rebase-interactive
RFC PATCH v4 1/1 rebase-interactive
This is v4 of the first 2 patches of "[RFC PATCH vV n/9] rebase-interactive", looking forward to any additional comments.
Wink Saville (2):
rebase-interactive: Simplify pick_on_preserving_merges
rebase: Update invocation of rebase dot-sourced scripts
git-rebase--am.sh | 11 ----------- git-rebase--interactive.sh | 28 +++++++--------------------- git-rebase--merge.sh | 11 ----------- git-rebase.sh | 2 ++ 4 files changed, 9 insertions(+), 43 deletions(-)
Signature · 1 line
2.16.2
RFC PATCH v4 1/1 rebase-interactive: Simplify pick_on_preserving_merges
Use compound if statement instead of nested if statements to simplify pick_on_preserving_merges.
--- git-rebase--interactive.sh | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
git-rebase--interactive.sh +7 -10
@@ -307,17 +307,14 @@ pick_one_preserving_merges () { esac sha1=$(git rev-parse $sha1) - if test -f "$state_dir"/current-commit + if test -f "$state_dir"/current-commit && test "$fast_forward" = t then - if test "$fast_forward" = t - then - while read current_commit - do - git rev-parse HEAD > "$rewritten"/$current_commit - done <"$state_dir"/current-commit - rm "$state_dir"/current-commit || - die "$(gettext "Cannot write current commit's replacement sha1")" - fi + while read current_commit + do + git rev-parse HEAD > "$rewritten"/$current_commit + done <"$state_dir"/current-commit + rm "$state_dir"/current-commit || + die "$(gettext "Cannot write current commit's replacement sha1")" fi echo $sha1 >> "$state_dir"/current-commit
Signature · 1 line
2.16.2
Re: [RFC PATCH v4] rebase-interactive: Simplify pick_on_preserving_merges
Hi Wink,
Quoted 7 lines · Wink Saville
Use compound if statement instead of nested if statements to simplify
pick_on_preserving_merges.Signed-off-by Wink Saville <[email protected]>Reviewed-by Junio C Hamano <[email protected]>--- git-rebase--interactive.sh | 17 +++++++----------
The patch is obviously correct.
Thanks,
Johannes
RFC PATCH v4 1/1 rebase: Update invocation of rebase dot-sourced scripts
In reply to Wink SavilleThe backend scriptlets for "git rebase" were structured in a bit unusual way for historical reasons. Originally, it was designed in such a way that dot-sourcing them from "git rebase" would be sufficient to invoke the specific backend.
When it was discovered that some shell implementations (e.g. FreeBSD 9.x) misbehaved when exiting with a "return" is executed at the top level of a dot-sourced script (the original was expecting that the control returns to the next command in "git rebase" after dot-sourcing the scriptlet).
To fix this issue the whole body of git-rebase--$backend.sh was made into a shell function git_rebase__$backend and then the last statement of the scriptlet would invoke the function.
Here the call is moved to "git rebase" side, instead of at the end of each scriptlet. This give us a more normal arrangement where the scriptlet function library and allows multiple functions to be implemented in a scriptlet.
--- git-rebase--am.sh | 11 ----------- git-rebase--interactive.sh | 11 ----------- git-rebase--merge.sh | 11 ----------- git-rebase.sh | 2 ++ 4 files changed, 2 insertions(+), 33 deletions(-)
git-rebase--am.sh +0 -11
@@ -4,15 +4,6 @@ # Copyright (c) 2010 Junio C Hamano. # -# The whole contents of this file is run by dot-sourcing it from -# inside a shell function. It used to be that "return"s we see -# below were not inside any function, and expected to return -# to the function that dot-sourced us. -# -# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a -# construct and continue to run the statements that follow such a "return". -# As a work-around, we introduce an extra layer of a function -# here, and immediately call it after defining it. git_rebase__am () { case "$action" in
@@ -105,5 +96,3 @@ fi move_to_original_branch } -# ... and then we call the whole thing. -git_rebase__am
git-rebase--interactive.sh +0 -11
@@ -740,15 +740,6 @@ get_missing_commit_check_level () { printf '%s' "$check_level" | tr 'A-Z' 'a-z' } -# The whole contents of this file is run by dot-sourcing it from -# inside a shell function. It used to be that "return"s we see -# below were not inside any function, and expected to return -# to the function that dot-sourced us. -# -# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a -# construct and continue to run the statements that follow such a "return". -# As a work-around, we introduce an extra layer of a function -# here, and immediately call it after defining it. git_rebase__interactive () { case "$action" in
@@ -1029,5 +1020,3 @@ fi do_rest } -# ... and then we call the whole thing. -git_rebase__interactive
git-rebase--merge.sh +0 -11
@@ -104,15 +104,6 @@ finish_rb_merge () { say All done. } -# The whole contents of this file is run by dot-sourcing it from -# inside a shell function. It used to be that "return"s we see -# below were not inside any function, and expected to return -# to the function that dot-sourced us. -# -# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a -# construct and continue to run the statements that follow such a "return". -# As a work-around, we introduce an extra layer of a function -# here, and immediately call it after defining it. git_rebase__merge () { case "$action" in
@@ -171,5 +162,3 @@ done finish_rb_merge } -# ... and then we call the whole thing. -git_rebase__merge
git-rebase.sh +2 -0
@@ -196,7 +196,9 @@ run_specific_rebase () { export GIT_EDITOR autosquash= fi + # Source the code and invoke it . git-rebase--$type + git_rebase__$type ret=$? if test $ret -eq 0 then
Signature · 1 line
2.16.2
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Thanks for splitting these changes into smaller, more manageable chunks. A couple non-code comments below apply to both patches in this 2-patch series even though I'm responding only to this patch. (The actual code changes in the other patch looked fine and the patch was easily digested.)
Quoted 1 line · Wink Saville
rebase: Update invocation of rebase dot-sourced scripts
Nit: On this project, the summary line is not capitalized, so: s/Update/update/
Quoted 1 line
The backend scriptlets for "git rebase" were structured in a
On this project, commit messages are written in imperative mood. The commit message Junio suggested[1] said "are structured", which makes for a better imperative mood fit.
Quoted 9 lines
bit unusual way for historical reasons. Originally, it was designed in such a way that dot-sourcing them from "git rebase" would be sufficient to invoke the specific backend.
When it was discovered that some shell implementations (e.g. FreeBSD 9.x) misbehaved when exiting with a "return" is executed at the top level of a dot-sourced script (the original was expecting that the control returns to the next command in "git rebase" after dot-sourcing the scriptlet).
ECANTPARSE: This paragraph is grammatically corrupt.
? "When {something}..." but then what?
? "...when exiting with a "return" is executed"
Quoted 3 lines
To fix this issue the whole body of git-rebase--$backend.sh was made into a shell function git_rebase__$backend and then the last statement of the scriptlet would invoke the function.
Junio's proposed commit message[1] called this a "workaround", not a "fix", and, indeed, "workaround" better characterizes that change. If anything, _this_ patch is a (more correct) "fix" for that workaround.
Quoted 1 line
Here the call is moved to "git rebase" side, instead of at the
Junio's version, using imperative mood, said "Move the call...".
Quoted 3 lines
end of each scriptlet. This give us a more normal arrangement where the scriptlet function library and allows multiple functions to be implemented in a scriptlet.
ECANTPARSE: Grammatically corrupt.
? "where the ... library and allows..."
Overall, Junio's proposed message followed project practice (imperative mood) more closely and felt somewhat more coherent (despite the run-on sentence in the first paragraph and the apparent incorrect explanation of top-level "return" misbehavior -- the in-code comment says top-level "return" was essentially a no-op in broken shells, whereas he said it exited the shell).
Perhaps the following re-write addresses the above concerns:
Due to historical reasons, the backend scriptlets for "git rebase" are structured a bit unusually. As originally designed, dot-sourcing them from "git rebase" was sufficient to invoke the specific backend.
However, it was later discovered that some shell implementations (e.g. FreeBSD 9.x) misbehaved by continuing to execute statements following a top-level "return" rather than returning control to the next statement in "git rebase" after dot-sourcing the scriptlet. To work around this shortcoming, the whole body of git-rebase--$backend.sh was made into a shell function git_rebase__$backend, and then the very last line of the scriptlet called that function.
A more normal architecture is for a dot-sourced scriptlet merely to define functions (thus acting as a function library), and for those functions to be called by the script doing the dot-sourcing. Migrate to this arrangement by moving the git_rebase__$backend call from the end of a scriptlet into "git rebase" itself.
While at it, remove the large comment block from each scriptlet explaining this historic anomaly since it serves no purpose under the new normalized architecture in which a scriptlet is merely a function library.
Quoted 3 lines
Signed-off-by Wink Saville <[email protected]>Reviewed-by Junio C Hamano <[email protected]>Reviewed-by Eric Sunsine <[email protected]>
Despite its name, on this project, a Reviewed-by: does not mean merely that a person looked at and commented on a patch. Rather, it is a way for a person to say "I have studied and understood the patch and feel that it is ready for inclusion in the project." Reviewed-by:'s are therefore always given explicitly by the reviewer and Junio adds them to a patch when queuing. (Reviewed-by:'s are not always given, though, even when a reviewer has not found problems with a patch. For instance, even if I review and comment on this or subsequent patches, I will not give a Reviewed-by: since I'm not an area expert, thus wouldn't feel comfortable stating that the patch is correct.)
Consequently, these Reviewed-by: lines should be dropped. (You can, on the other hand, add Helped-by:'s when appropriate.)
The patch itself makes sense and seems straightforward. See one minor comment below...
[1]: https://public-inbox.org/git/[email protected]/
git-rebase.sh run_specific_rebase () { · line 201
Quoted 12 lines
---
git-rebase.sh +2
-0@@ -196,7 +196,9 @@ run_specific_rebase () { export GIT_EDITOR autosquash= fi + # Source the code and invoke it . git-rebase--$type + git_rebase__$type
The new comment merely repeats what the two lines of code themselves already state clearly, thus the comment is an unnecessary distraction; it does not aid in understanding the code.
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Quoted 10 lines · Eric Sunshine
Quoted 1 line
When it was discovered that some shell implementations
...
ECANTPARSE: This paragraph is grammatically corrupt.ECANTPARSE: Grammatically corrupt.
...
(despite the run-on sentence in the first paragraph and the apparent incorrect explanation of top-level "return" misbehavior -- the in-code comment says top-level "return" was essentially a no-op in broken shells, whereas he said it exited the shell).
My bad, almost entirely. Sorry.
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Eric Sunshine
(despite the run-on sentence in the first paragraph and the apparent incorrect explanation of top-level "return" misbehavior -- the in-code comment says top-level "return" was essentially a no-op in broken shells, whereas he said it exited the shell).
My bad, almost entirely. Sorry.
No apology necessary. That minor error aside, your proposed commit message gave just the right amount of detail for a person (me) not at all familiar with the topic to be able to understand it fully and intuit the patch content before even reading the patch proper. That's a good commit message.
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
In reply to Wink SavilleHi Wink,
Quoted 20 lines · Wink Saville
The backend scriptlets for "git rebase" were structured in a bit unusual way for historical reasons. Originally, it was designed in such a way that dot-sourcing them from "git rebase" would be sufficient to invoke the specific backend.
When it was discovered that some shell implementations (e.g. FreeBSD 9.x) misbehaved when exiting with a "return" is executed at the top level of a dot-sourced script (the original was expecting that the control returns to the next command in "git rebase" after dot-sourcing the scriptlet).
To fix this issue the whole body of git-rebase--$backend.sh was made into a shell function git_rebase__$backend and then the last statement of the scriptlet would invoke the function.
Here the call is moved to "git rebase" side, instead of at the end of each scriptlet. This give us a more normal arrangement where the scriptlet function library and allows multiple functions to be implemented in a scriptlet.
Quoted 8 lines
Signed-off-by Wink Saville <[email protected]>Reviewed-by Junio C Hamano <[email protected]>Reviewed-by Eric Sunsine <[email protected]>--- git-rebase--am.sh | 11 ----------- git-rebase--interactive.sh | 11 ----------- git-rebase--merge.sh | 11 ----------- git-rebase.sh | 2 ++
The patch makes sense to me.
Thanks,
Johannes
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Quoted 33 lines · Johannes Schindelin
Hi Wink,
Quoted 20 lines · Wink Saville
The backend scriptlets for "git rebase" were structured in a bit unusual way for historical reasons. Originally, it was designed in such a way that dot-sourcing them from "git rebase" would be sufficient to invoke the specific backend.
When it was discovered that some shell implementations (e.g. FreeBSD 9.x) misbehaved when exiting with a "return" is executed at the top level of a dot-sourced script (the original was expecting that the control returns to the next command in "git rebase" after dot-sourcing the scriptlet).
To fix this issue the whole body of git-rebase--$backend.sh was made into a shell function git_rebase__$backend and then the last statement of the scriptlet would invoke the function.
Here the call is moved to "git rebase" side, instead of at the end of each scriptlet. This give us a more normal arrangement where the scriptlet function library and allows multiple functions to be implemented in a scriptlet.
Quoted 8 lines
Signed-off-by Wink Saville <[email protected]>Reviewed-by Junio C Hamano <[email protected]>Reviewed-by Eric Sunsine <[email protected]>--- git-rebase--am.sh | 11 ----------- git-rebase--interactive.sh | 11 ----------- git-rebase--merge.sh | 11 ----------- git-rebase.sh | 2 ++
Quoted 4 lines
The patch makes sense to me.
Thanks,
Johannes
Junio, Eric and Johannes, thanks for the help!!!
I've created v5 with the two patches, what is the suggested format-patch/send-email command(s)?
Here is one possibility:
git format-patch --cover-letter --rfc --thread -v 5 [email protected] [email protected] [email protected] -o patches/v5 master..v5-2
If this was the first version then the above would seem to be a reasonable choice.
But this is version 5 and maybe I don't need --cover-letter which, I think means I
don't want to use --thread. If that's the case should I add --in-reply-to? But that leads to the question. from which message should I get the Message-Id?
More likely I'm totally wrong and should do something completely different, advice appreciated.
-- Wink
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Quoted 5 lines · Wink Saville
Here is one possibility:
git format-patch --cover-letter --rfc --thread -v 5 [email protected] [email protected] [email protected] -o patches/v5 master..v5-2
Sounds sensible.
Quoted 2 lines
If this was the first version then the above would seem to be a reasonable choice.
My personal preference (both as a reviewer and an occasional multi-patch series submitter) is to use a cover letter for a larger series (e.g. more than 3-5 patches), regardless of the iteration. In fact, a submitter tends to have _more_ things to say in the cover letter for v2 and subsequent iteration than the original iteration.
The motivation behind the series may not change so greatly but will be refined as iterations go on, and you want help those who missed the earlier iteration understand what you are doing with the updated cover letter. Also cover letter is the ideal place to outline where to find older iterations and their discussion and summarize what changed since these earlier attempts in this round.
Quoted 4 lines
But this is version 5 and maybe I don't need --cover-letter which, I think means I
don't want to use --thread. If that's the case should I add --in-reply-to? But that leads to the question. from which message should I get the Message-Id?
The most typical practice I've seen around here is that v5's cover is made in-reply-to v4's cover.
Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Quoted 26 lines · Junio C Hamano
Quoted 5 lines · Wink Saville
Here is one possibility:
git format-patch --cover-letter --rfc --thread -v 5 [email protected] [email protected] [email protected] -o patches/v5 master..v5-2
Sounds sensible.
Quoted 2 lines
If this was the first version then the above would seem to be a reasonable choice.
My personal preference (both as a reviewer and an occasional multi-patch series submitter) is to use a cover letter for a larger series (e.g. more than 3-5 patches), regardless of the iteration. In fact, a submitter tends to have _more_ things to say in the cover letter for v2 and subsequent iteration than the original iteration.
The motivation behind the series may not change so greatly but will be refined as iterations go on, and you want help those who missed the earlier iteration understand what you are doing with the updated cover letter. Also cover letter is the ideal place to outline where to find older iterations and their discussion and summarize what changed since these earlier attempts in this round.
Quoted 8 lines
Quoted 4 lines
But this is version 5 and maybe I don't need --cover-letter which, I think means I
don't want to use --thread. If that's the case should I add --in-reply-to? But that leads to the question. from which message should I get the Message-Id?The most typical practice I've seen around here is that v5's cover is made in-reply-to v4's cover.
Make sense
v5 cover rebase-interactive
In reply to Wink SavilleReworked patch 1 so that all of the backend scriptlets used by git-rebase use a normal function style invocation.
Merged the previous patch 2 and 3 have been squashed which provides reviewers a little easier time to detect any changes during extraction of the functions.
Wink Saville (8):
rebase-interactive: simplify pick_on_preserving_merges
rebase: update invocation of rebase dot-sourced scripts Indent function git_rebase__interactive
Extract functions out of git_rebase__interactive
Add and use git_rebase__interactive__preserve_merges Remove unused code paths from git_rebase__interactive Remove unused code paths from git_rebase__interactive__preserve_merges Remove merges_option and a blank line
git-rebase--am.sh | 11 -- git-rebase--interactive.sh | 407 ++++++++++++++++++++++++--------------------- git-rebase--merge.sh | 11 -- git-rebase.sh | 1 + 4 files changed, 216 insertions(+), 214 deletions(-)
Signature · 1 line
2.16.2
RFC PATCH v5 1/8 rebase-interactive: simplify pick_on_preserving_merges
Use compound if statement instead of nested if statements to simplify pick_on_preserving_merges.
--- git-rebase--interactive.sh | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
git-rebase--interactive.sh +7 -10
@@ -307,17 +307,14 @@ pick_one_preserving_merges () { esac sha1=$(git rev-parse $sha1) - if test -f "$state_dir"/current-commit + if test -f "$state_dir"/current-commit && test "$fast_forward" = t then - if test "$fast_forward" = t - then - while read current_commit - do - git rev-parse HEAD > "$rewritten"/$current_commit - done <"$state_dir"/current-commit - rm "$state_dir"/current-commit || - die "$(gettext "Cannot write current commit's replacement sha1")" - fi + while read current_commit + do + git rev-parse HEAD > "$rewritten"/$current_commit + done <"$state_dir"/current-commit + rm "$state_dir"/current-commit || + die "$(gettext "Cannot write current commit's replacement sha1")" fi echo $sha1 >> "$state_dir"/current-commit
Signature · 1 line
2.16.2
RFC PATCH v5 2/8 rebase: update invocation of rebase dot-sourced scripts
In reply to Wink SavilleDue to historical reasons, the backend scriptlets for "git rebase" are structured a bit unusually. As originally designed, dot-sourcing them from "git rebase" was sufficient to invoke the specific backend.
However, it was later discovered that some shell implementations (e.g. FreeBSD 9.x) misbehaved by continuing to execute statements following a top-level "return" rather than returning control to the next statement in "git rebase" after dot-sourcing the scriptlet. To work around this shortcoming, the whole body of git-rebase--$backend.sh was made into a shell function git_rebase__$backend, and then the very last line of the scriptlet called that function.
A more normal architecture is for a dot-sourced scriptlet merely to define functions (thus acting as a function library), and for those functions to be called by the script doing the dot-sourcing. Migrate to this arrangement by moving the git_rebase__$backend call from the end of a scriptlet into "git rebase" itself.
While at it, remove the large comment block from each scriptlet explaining this historic anomaly since it serves no purpose under the new normalized architecture in which a scriptlet is merely a function library.
--- git-rebase--am.sh | 11 ----------- git-rebase--interactive.sh | 11 ----------- git-rebase--merge.sh | 11 ----------- git-rebase.sh | 1 + 4 files changed, 1 insertion(+), 33 deletions(-)
git-rebase--am.sh +0 -11
@@ -4,15 +4,6 @@ # Copyright (c) 2010 Junio C Hamano. # -# The whole contents of this file is run by dot-sourcing it from -# inside a shell function. It used to be that "return"s we see -# below were not inside any function, and expected to return -# to the function that dot-sourced us. -# -# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a -# construct and continue to run the statements that follow such a "return". -# As a work-around, we introduce an extra layer of a function -# here, and immediately call it after defining it. git_rebase__am () { case "$action" in
@@ -105,5 +96,3 @@ fi move_to_original_branch } -# ... and then we call the whole thing. -git_rebase__am
git-rebase--interactive.sh +0 -11
@@ -740,15 +740,6 @@ get_missing_commit_check_level () { printf '%s' "$check_level" | tr 'A-Z' 'a-z' } -# The whole contents of this file is run by dot-sourcing it from -# inside a shell function. It used to be that "return"s we see -# below were not inside any function, and expected to return -# to the function that dot-sourced us. -# -# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a -# construct and continue to run the statements that follow such a "return". -# As a work-around, we introduce an extra layer of a function -# here, and immediately call it after defining it. git_rebase__interactive () { case "$action" in
@@ -1029,5 +1020,3 @@ fi do_rest } -# ... and then we call the whole thing. -git_rebase__interactive
git-rebase--merge.sh +0 -11
@@ -104,15 +104,6 @@ finish_rb_merge () { say All done. } -# The whole contents of this file is run by dot-sourcing it from -# inside a shell function. It used to be that "return"s we see -# below were not inside any function, and expected to return -# to the function that dot-sourced us. -# -# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a -# construct and continue to run the statements that follow such a "return". -# As a work-around, we introduce an extra layer of a function -# here, and immediately call it after defining it. git_rebase__merge () { case "$action" in
@@ -171,5 +162,3 @@ done finish_rb_merge } -# ... and then we call the whole thing. -git_rebase__merge
git-rebase.sh +1 -0
@@ -197,6 +197,7 @@ run_specific_rebase () { autosquash= fi . git-rebase--$type + git_rebase__$type ret=$? if test $ret -eq 0 then
Signature · 1 line
2.16.2
RFC PATCH v5 3/8 Indent function git_rebase__interactive
In reply to Wink Saville--- git-rebase--interactive.sh | 432 ++++++++++++++++++++++----------------------- 1 file changed, 215 insertions(+), 217 deletions(-)
git-rebase--interactive.sh +215 -217
@@ -741,27 +741,26 @@ get_missing_commit_check_level () { } git_rebase__interactive () { - -case "$action" in -continue) - if test ! -d "$rewritten" - then - exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \ - --continue - fi - # do we have anything to commit? - if git diff-index --cached --quiet HEAD -- - then - # Nothing to commit -- skip this commit - - test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD || - rm "$GIT_DIR"/CHERRY_PICK_HEAD || - die "$(gettext "Could not remove CHERRY_PICK_HEAD")" - else - if ! test -f "$author_script" + case "$action" in + continue) + if test ! -d "$rewritten" then - gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} - die "$(eval_gettext "\ + exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \ + --continue + fi + # do we have anything to commit? + if git diff-index --cached --quiet HEAD -- + then + # Nothing to commit -- skip this commit + + test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD || + rm "$GIT_DIR"/CHERRY_PICK_HEAD || + die "$(gettext "Could not remove CHERRY_PICK_HEAD")" + else + if ! test -f "$author_script" + then + gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} + die "$(eval_gettext "\ You have staged changes in your working tree. If these changes are meant to be squashed into the previous commit, run:
@@ -776,197 +775,197 @@ In both cases, once you're done, continue with: git rebase --continue ")" - fi - . "$author_script" || - die "$(gettext "Error trying to find the author identity to amend commit")" - if test -f "$amend" - then - current_head=$(git rev-parse --verify HEAD) - test "$current_head" = $(cat "$amend") || - die "$(gettext "\ + fi + . "$author_script" || + die "$(gettext "Error trying to find the author identity to amend commit")" + if test -f "$amend" + then + current_head=$(git rev-parse --verify HEAD) + test "$current_head" = $(cat "$amend") || + die "$(gettext "\ You have uncommitted changes in your working tree. Please commit them first and then run 'git rebase --continue' again.")" - do_with_author git commit --amend --no-verify -F "$msg" -e \ - ${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message || - die "$(gettext "Could not commit staged changes.")" - else - do_with_author git commit --no-verify -F "$msg" -e \ - ${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message || - die "$(gettext "Could not commit staged changes.")" + do_with_author git commit --amend --no-verify -F "$msg" -e \ + ${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message || + die "$(gettext "Could not commit staged changes.")" + else + do_with_author git commit --no-verify -F "$msg" -e \ + ${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message || + die "$(gettext "Could not commit staged changes.")" + fi fi - fi - if test -r "$state_dir"/stopped-sha - then - record_in_rewritten "$(cat "$state_dir"/stopped-sha)" - fi + if test -r "$state_dir"/stopped-sha + then + record_in_rewritten "$(cat "$state_dir"/stopped-sha)" + fi - require_clean_work_tree "rebase" - do_rest - return 0 - ;; -skip) - git rerere clear + require_clean_work_tree "rebase" + do_rest + return 0 + ;; + skip) + git rerere clear - if test ! -d "$rewritten" - then - exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \ - --continue - fi - do_rest - return 0 - ;; -edit-todo) - git stripspace --strip-comments <"$todo" >"$todo".new - mv -f "$todo".new "$todo" - collapse_todo_ids - append_todo_help - gettext " + if test ! -d "$rewritten" + then + exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \ + --continue + fi + do_rest + return 0 + ;; + edit-todo) + git stripspace --strip-comments <"$todo" >"$todo".new + mv -f "$todo".new "$todo" + collapse_todo_ids + append_todo_help + gettext " You are editing the todo file of an ongoing interactive rebase. To continue rebase after editing, run: git rebase --continue " | git stripspace --comment-lines >>"$todo" - git_sequence_editor "$todo" || - die "$(gettext "Could not execute editor")" - expand_todo_ids - - exit - ;; -show-current-patch) - exec git show REBASE_HEAD -- - ;; -esac - -comment_for_reflog start + git_sequence_editor "$todo" || + die "$(gettext "Could not execute editor")" + expand_todo_ids -if test ! -z "$switch_to" -then - GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" - output git checkout "$switch_to" -- || - die "$(eval_gettext "Could not checkout \$switch_to")" + exit + ;; + show-current-patch) + exec git show REBASE_HEAD -- + ;; + esac comment_for_reflog start -fi - -orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")" -mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")" -rm -f "$(git rev-parse --git-path REBASE_HEAD)" -: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")" -write_basic_state -if test t = "$preserve_merges" -then - if test -z "$rebase_root" + if test ! -z "$switch_to" then - mkdir "$rewritten" && - for c in $(git merge-base --all $orig_head $upstream) - do - echo $onto > "$rewritten"/$c || - die "$(gettext "Could not init rewritten commits")" - done - else - mkdir "$rewritten" && - echo $onto > "$rewritten"/root || - die "$(gettext "Could not init rewritten commits")" + GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" + output git checkout "$switch_to" -- || + die "$(eval_gettext "Could not checkout \$switch_to")" + + comment_for_reflog start fi - # No cherry-pick because our first pass is to determine - # parents to rewrite and skipping dropped commits would - # prematurely end our probe - merges_option= -else - merges_option="--no-merges --cherry-pick" -fi - -shorthead=$(git rev-parse --short $orig_head) -shortonto=$(git rev-parse --short $onto) -if test -z "$rebase_root" - # this is now equivalent to ! -z "$upstream" -then - shortupstream=$(git rev-parse --short $upstream) - revisions=$upstream...$orig_head - shortrevisions=$shortupstream..$shorthead -else - revisions=$onto...$orig_head - shortrevisions=$shorthead -fi -if test t != "$preserve_merges" -then - git rebase--helper --make-script ${keep_empty:+--keep-empty} \ - $revisions ${restrict_revision+^$restrict_revision} >"$todo" || - die "$(gettext "Could not generate todo list")" -else - format=$(git config --get rebase.instructionFormat) - # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse - git rev-list $merges_option --format="%m%H ${format:-%s}" \ - --reverse --left-right --topo-order \ - $revisions ${restrict_revision+^$restrict_revision} | \ - sed -n "s/^>//p" | - while read -r sha1 rest - do - if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1 - then - comment_out="$comment_char " - else - comment_out= - fi + orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")" + mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")" + rm -f "$(git rev-parse --git-path REBASE_HEAD)" + : > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")" + write_basic_state + if test t = "$preserve_merges" + then if test -z "$rebase_root" then - preserve=t - for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-) + mkdir "$rewritten" && + for c in $(git merge-base --all $orig_head $upstream) do - if test -f "$rewritten"/$p - then - preserve=f - fi + echo $onto > "$rewritten"/$c || + die "$(gettext "Could not init rewritten commits")" done else - preserve=f - fi - if test f = "$preserve" - then - touch "$rewritten"/$sha1 - printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo" - fi - done -fi - -# Watch for commits that been dropped by --cherry-pick -if test t = "$preserve_merges" -then - mkdir "$dropped" - # Save all non-cherry-picked changes - git rev-list $revisions --left-right --cherry-pick | \ - sed -n "s/^>//p" > "$state_dir"/not-cherry-picks - # Now all commits and note which ones are missing in - # not-cherry-picks and hence being dropped - git rev-list $revisions | - while read rev - do - if test -f "$rewritten"/$rev && - ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null - then - # Use -f2 because if rev-list is telling us this commit is - # not worthwhile, we don't want to track its multiple heads, - # just the history of its first-parent for others that will - # be rebasing on top of it - git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev - sha1=$(git rev-list -1 $rev) - sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo" - rm "$rewritten"/$rev + mkdir "$rewritten" && + echo $onto > "$rewritten"/root || + die "$(gettext "Could not init rewritten commits")" fi - done -fi + # No cherry-pick because our first pass is to determine + # parents to rewrite and skipping dropped commits would + # prematurely end our probe + merges_option= + else + merges_option="--no-merges --cherry-pick" + fi + + shorthead=$(git rev-parse --short $orig_head) + shortonto=$(git rev-parse --short $onto) + if test -z "$rebase_root" + # this is now equivalent to ! -z "$upstream" + then + shortupstream=$(git rev-parse --short $upstream) + revisions=$upstream...$orig_head + shortrevisions=$shortupstream..$shorthead + else + revisions=$onto...$orig_head + shortrevisions=$shorthead + fi + if test t != "$preserve_merges" + then + git rebase--helper --make-script ${keep_empty:+--keep-empty} \ + $revisions ${restrict_revision+^$restrict_revision} >"$todo" || + die "$(gettext "Could not generate todo list")" + else + format=$(git config --get rebase.instructionFormat) + # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse + git rev-list $merges_option --format="%m%H ${format:-%s}" \ + --reverse --left-right --topo-order \ + $revisions ${restrict_revision+^$restrict_revision} | \ + sed -n "s/^>//p" | + while read -r sha1 rest + do + + if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1 + then + comment_out="$comment_char " + else + comment_out= + fi + + if test -z "$rebase_root" + then + preserve=t + for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-) + do + if test -f "$rewritten"/$p + then + preserve=f + fi + done + else + preserve=f + fi + if test f = "$preserve" + then + touch "$rewritten"/$sha1 + printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo" + fi + done + fi + + # Watch for commits that been dropped by --cherry-pick + if test t = "$preserve_merges" + then + mkdir "$dropped" + # Save all non-cherry-picked changes + git rev-list $revisions --left-right --cherry-pick | \ + sed -n "s/^>//p" > "$state_dir"/not-cherry-picks + # Now all commits and note which ones are missing in + # not-cherry-picks and hence being dropped + git rev-list $revisions | + while read rev + do + if test -f "$rewritten"/$rev && + ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null + then + # Use -f2 because if rev-list is telling us this commit is + # not worthwhile, we don't want to track its multiple heads, + # just the history of its first-parent for others that will + # be rebasing on top of it + git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev + sha1=$(git rev-list -1 $rev) + sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo" + rm "$rewritten"/$rev + fi + done + fi -test -s "$todo" || echo noop >> "$todo" -test -z "$autosquash" || git rebase--helper --rearrange-squash || exit -test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd" + test -s "$todo" || echo noop >> "$todo" + test -z "$autosquash" || git rebase--helper --rearrange-squash || exit + test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd" -todocount=$(git stripspace --strip-comments <"$todo" | wc -l) -todocount=${todocount##* } + todocount=$(git stripspace --strip-comments <"$todo" | wc -l) + todocount=${todocount##* } cat >>"$todo" <<EOF
@@ -975,48 +974,47 @@ $comment_char $(eval_ngettext \ "Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \ "$todocount") EOF -append_todo_help -gettext " -However, if you remove everything, the rebase will be aborted. - -" | git stripspace --comment-lines >>"$todo" + append_todo_help + gettext " + However, if you remove everything, the rebase will be aborted. -if test -z "$keep_empty" -then - printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo" -fi + " | git stripspace --comment-lines >>"$todo" + if test -z "$keep_empty" + then + printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo" + fi -has_action "$todo" || - return 2 -cp "$todo" "$todo".backup -collapse_todo_ids -git_sequence_editor "$todo" || - die_abort "$(gettext "Could not execute editor")" + has_action "$todo" || + return 2 -has_action "$todo" || - return 2 + cp "$todo" "$todo".backup + collapse_todo_ids + git_sequence_editor "$todo" || + die_abort "$(gettext "Could not execute editor")" -git rebase--helper --check-todo-list || { - ret=$? - checkout_onto - exit $ret -} + has_action "$todo" || + return 2 -expand_todo_ids + git rebase--helper --check-todo-list || { + ret=$? + checkout_onto + exit $ret + } -test -d "$rewritten" || test -n "$force_rebase" || -onto="$(git rebase--helper --skip-unnecessary-picks)" || -die "Could not skip unnecessary pick commands" + expand_todo_ids -checkout_onto -if test -z "$rebase_root" && test ! -d "$rewritten" -then - require_clean_work_tree "rebase" - exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \ - --continue -fi -do_rest + test -d "$rewritten" || test -n "$force_rebase" || + onto="$(git rebase--helper --skip-unnecessary-picks)" || + die "Could not skip unnecessary pick commands" + checkout_onto + if test -z "$rebase_root" && test ! -d "$rewritten" + then + require_clean_work_tree "rebase" + exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \ + --continue + fi + do_rest }
Signature · 1 line
2.16.2
Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive
Quoted 4 lines · Wink Saville
Signed-off-by Wink Saville <[email protected]>--- git-rebase--interactive.sh | 432 ++++++++++++++++++++++----------------------- 1 file changed, 215 insertions(+), 217 deletions(-)
Thanks for separating this step out. "git show -w --stat -p" tells us that this is a pure re-indent patch pretty easily ;-).
Overlong lines might want to get rewrapped at some point, and it is OK to do that either in this step or in a separate step.
Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Wink Saville
Signed-off-by Wink Saville <[email protected]>--- git-rebase--interactive.sh | 432 ++++++++++++++++++++++----------------------- 1 file changed, 215 insertions(+), 217 deletions(-)
Quoted 6 lines
Thanks for separating this step out. "git show -w --stat -p" tells us that this is a pure re-indent patch pretty easily ;-).
Overlong lines might want to get rewrapped at some point, and it is OK to do that either in this step or in a separate step.
The longest line in the file before this change was line 532 which is 108 characters, now there are three lines longer because of the indentation. Line 762 is 112, line 957 is 110 and 985 110.
My initial reaction is to leave these long lines as is, but if you want them shorter what is the maximum line length? At 80 characters per line I count about 25 lines will need to be shortened.
Also, I assume you want me to only change lines in git_rebase__interactive.
-- Wink
Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive
Quoted 2 lines · Wink Saville
Also, I assume you want me to only change lines in
git_rebase__interactive.
I actually do not care if line-wrapping is done; it is perfectly fine to leave it for future clean-up and leave it outside the scope of this series. If you are going to do as a part of the series, yes, I do prefer you limit yourself to those lines that are involved in the series in some other way.
Thanks.
Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive
Quoted 5 lines
I actually do not care if line-wrapping is done; it is perfectly fine to leave it for future clean-up and leave it outside the scope of this series. If you are going to do as a part of the series, yes, I do prefer you limit yourself to those lines that are involved in the series in some other way.
Then lets leave the long lines alone for this series.
RFC PATCH v5 4/8 Extract functions out of git_rebase__interactive
In reply to Wink SavilleThe extracted functions are:
- initiate_action
- setup_reflog_action
- init_basic_state
- init_revisions_and_shortrevisions
- complete_action
Used by git_rebase__interactive
--- git-rebase--interactive.sh | 182 +++++++++++++++++++++++++++------------------ 1 file changed, 111 insertions(+), 71 deletions(-)
git-rebase--interactive.sh +111 -71
@@ -740,8 +740,20 @@ get_missing_commit_check_level () { printf '%s' "$check_level" | tr 'A-Z' 'a-z' } -git_rebase__interactive () { - case "$action" in +# Initiate an action. If the cannot be any +# further action it may exec a command +# or exit and not return. +# +# TODO: Consider a cleaner return model so it +# never exits and always return 0 if process +# is complete. +# +# Parameter 1 is the action to initiate. +# +# Returns 0 if the action was able to complete +# and if 1 if further processing is required. +initiate_action () { + case "$1" in continue) if test ! -d "$rewritten" then
@@ -836,8 +848,13 @@ To continue rebase after editing, run: show-current-patch) exec git show REBASE_HEAD -- ;; + *) + return 1 # continue + ;; esac +} +setup_reflog_action () { comment_for_reflog start if test ! -z "$switch_to"
@@ -848,13 +865,102 @@ To continue rebase after editing, run: comment_for_reflog start fi +} +init_basic_state () { orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")" mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")" rm -f "$(git rev-parse --git-path REBASE_HEAD)" : > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")" write_basic_state +} + +init_revisions_and_shortrevisions () { + shorthead=$(git rev-parse --short $orig_head) + shortonto=$(git rev-parse --short $onto) + if test -z "$rebase_root" + # this is now equivalent to ! -z "$upstream" + then + shortupstream=$(git rev-parse --short $upstream) + revisions=$upstream...$orig_head + shortrevisions=$shortupstream..$shorthead + else + revisions=$onto...$orig_head + shortrevisions=$shorthead + fi +} + +complete_action() { + test -s "$todo" || echo noop >> "$todo" + test -z "$autosquash" || git rebase--helper --rearrange-squash || exit + test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd" + + todocount=$(git stripspace --strip-comments <"$todo" | wc -l) + todocount=${todocount##* } + +cat >>"$todo" <<EOF + +$comment_char $(eval_ngettext \ + "Rebase \$shortrevisions onto \$shortonto (\$todocount command)" \ + "Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \ + "$todocount") +EOF + append_todo_help + gettext " + However, if you remove everything, the rebase will be aborted. + + " | git stripspace --comment-lines >>"$todo" + + if test -z "$keep_empty" + then + printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo" + fi + + + has_action "$todo" || + return 2 + + cp "$todo" "$todo".backup + collapse_todo_ids + git_sequence_editor "$todo" || + die_abort "$(gettext "Could not execute editor")" + + has_action "$todo" || + return 2 + + git rebase--helper --check-todo-list || { + ret=$? + checkout_onto + exit $ret + } + + expand_todo_ids + + test -d "$rewritten" || test -n "$force_rebase" || + onto="$(git rebase--helper --skip-unnecessary-picks)" || + die "Could not skip unnecessary pick commands" + + checkout_onto + if test -z "$rebase_root" && test ! -d "$rewritten" + then + require_clean_work_tree "rebase" + exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \ + --continue + fi + do_rest +} + +git_rebase__interactive () { + initiate_action "$action" + ret=$? + if test $ret = 0; then + return 0 + fi + + setup_reflog_action + init_basic_state + if test t = "$preserve_merges" then if test -z "$rebase_root"
@@ -878,18 +984,8 @@ To continue rebase after editing, run: merges_option="--no-merges --cherry-pick" fi - shorthead=$(git rev-parse --short $orig_head) - shortonto=$(git rev-parse --short $onto) - if test -z "$rebase_root" - # this is now equivalent to ! -z "$upstream" - then - shortupstream=$(git rev-parse --short $upstream) - revisions=$upstream...$orig_head - shortrevisions=$shortupstream..$shorthead - else - revisions=$onto...$orig_head - shortrevisions=$shorthead - fi + init_revisions_and_shortrevisions + if test t != "$preserve_merges" then git rebase--helper --make-script ${keep_empty:+--keep-empty} \
@@ -960,61 +1056,5 @@ To continue rebase after editing, run: done fi - test -s "$todo" || echo noop >> "$todo" - test -z "$autosquash" || git rebase--helper --rearrange-squash || exit - test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd" - - todocount=$(git stripspace --strip-comments <"$todo" | wc -l) - todocount=${todocount##* } - -cat >>"$todo" <<EOF - -$comment_char $(eval_ngettext \ - "Rebase \$shortrevisions onto \$shortonto (\$todocount command)" \ - "Rebase \$shortrevisions onto \$shortonto (\$todocount commands)" \ - "$todocount") -EOF - append_todo_help - gettext " - However, if you remove everything, the rebase will be aborted. - - " | git stripspace --comment-lines >>"$todo" - - if test -z "$keep_empty" - then - printf '%s\n' "$comment_char $(gettext "Note that empty commits are commented out")" >>"$todo" - fi - - - has_action "$todo" || - return 2 - - cp "$todo" "$todo".backup - collapse_todo_ids - git_sequence_editor "$todo" || - die_abort "$(gettext "Could not execute editor")" - - has_action "$todo" || - return 2 - - git rebase--helper --check-todo-list || { - ret=$? - checkout_onto - exit $ret - } - - expand_todo_ids - - test -d "$rewritten" || test -n "$force_rebase" || - onto="$(git rebase--helper --skip-unnecessary-picks)" || - die "Could not skip unnecessary pick commands" - - checkout_onto - if test -z "$rebase_root" && test ! -d "$rewritten" - then - require_clean_work_tree "rebase" - exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \ - --continue - fi - do_rest + complete_action }
Signature · 1 line
2.16.2
Re: [RFC PATCH v5 4/8] Extract functions out of git_rebase__interactive
Quoted 7 lines · Wink Saville
The extracted functions are:
-initiate_action
-setup_reflog_action
-init_basic_state
-init_revisions_and_shortrevisions
-complete_action
Quoted 6 lines
Used by
git_rebase__interactiveSigned-off-by Wink Saville <[email protected]>Helped-by Junio C Hamano <[email protected]>Helped-by Johannes Schindelin <[email protected]>---
I checked the correspondence of lines between the verison before and after the patch, and did not spot anything suspicious. The fact that we do not use "local" and stick to POSIX shell helps a bit, as we do not have to worry about "does this $variable in the split-out function refer to the same data as the original?" ;-)
Will queue.
Re: [RFC PATCH v5 4/8] Extract functions out of git_rebase__interactive
In reply to Wink Savillegit-rebase--interactive.sh get_missing_commit_check_level () { · line 740
Quoted 7 lines · Wink Saville
The extracted functions are:
[...]Signed-off-by Wink Saville <[email protected]>---
git-rebase--interactive.sh +1
-0@@ -740,8 +740,20 @@ get_missing_commit_check_level () { +# Initiate an action. If the cannot be any
s/the/there/
git-rebase--interactive.sh get_missing_commit_check_level () { · line 744
Quoted 1 line
git-rebase--interactive.sh +1
-0+# further action it may exec a command
s/\s+/ /
Quoted 11 lines
git-rebase--interactive.sh +11
-0+# or exit and not return. +# +# TODO: Consider a cleaner return model so it +# never exits and always return 0 if process +# is complete. +# +# Parameter 1 is the action to initiate. +# +# Returns 0 if the action was able to complete +# and if 1 if further processing is required. +initiate_action () {
RFC PATCH v5 5/8 Add and use git_rebase__interactive__preserve_merges
In reply to Wink SavilleAt the moment it's an exact copy of git_rebase__interactive except the name has changed.
--- git-rebase--interactive.sh | 108 +++++++++++++++++++++++++++++++++++++++++++++ git-rebase.sh | 2 +- 2 files changed, 109 insertions(+), 1 deletion(-)
git-rebase--interactive.sh +108 -0
@@ -1058,3 +1058,111 @@ git_rebase__interactive () { complete_action } + +git_rebase__interactive__preserve_merges () { + initiate_action "$action" + ret=$? + if test $ret = 0; then + return 0 + fi + + setup_reflog_action + init_basic_state + + if test t = "$preserve_merges" + then + if test -z "$rebase_root" + then + mkdir "$rewritten" && + for c in $(git merge-base --all $orig_head $upstream) + do + echo $onto > "$rewritten"/$c || + die "$(gettext "Could not init rewritten commits")" + done + else + mkdir "$rewritten" && + echo $onto > "$rewritten"/root || + die "$(gettext "Could not init rewritten commits")" + fi + # No cherry-pick because our first pass is to determine + # parents to rewrite and skipping dropped commits would + # prematurely end our probe + merges_option= + else + merges_option="--no-merges --cherry-pick" + fi + + init_revisions_and_shortrevisions + + if test t != "$preserve_merges" + then + git rebase--helper --make-script ${keep_empty:+--keep-empty} \ + $revisions ${restrict_revision+^$restrict_revision} >"$todo" || + die "$(gettext "Could not generate todo list")" + else + format=$(git config --get rebase.instructionFormat) + # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse + git rev-list $merges_option --format="%m%H ${format:-%s}" \ + --reverse --left-right --topo-order \ + $revisions ${restrict_revision+^$restrict_revision} | \ + sed -n "s/^>//p" | + while read -r sha1 rest + do + + if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1 + then + comment_out="$comment_char " + else + comment_out= + fi + + if test -z "$rebase_root" + then + preserve=t + for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-) + do + if test -f "$rewritten"/$p + then + preserve=f + fi + done + else + preserve=f + fi + if test f = "$preserve" + then + touch "$rewritten"/$sha1 + printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo" + fi + done + fi + + # Watch for commits that been dropped by --cherry-pick + if test t = "$preserve_merges" + then + mkdir "$dropped" + # Save all non-cherry-picked changes + git rev-list $revisions --left-right --cherry-pick | \ + sed -n "s/^>//p" > "$state_dir"/not-cherry-picks + # Now all commits and note which ones are missing in + # not-cherry-picks and hence being dropped + git rev-list $revisions | + while read rev + do + if test -f "$rewritten"/$rev && + ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null + then + # Use -f2 because if rev-list is telling us this commit is + # not worthwhile, we don't want to track its multiple heads, + # just the history of its first-parent for others that will + # be rebasing on top of it + git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev + sha1=$(git rev-list -1 $rev) + sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo" + rm "$rewritten"/$rev + fi + done + fi + + complete_action +}
git-rebase.sh +1 -1
@@ -197,7 +197,7 @@ run_specific_rebase () { autosquash= fi . git-rebase--$type - git_rebase__$type + git_rebase__$type${preserve_merges:+__preserve_merges} ret=$? if test $ret -eq 0 then
Signature · 1 line
2.16.2
RFC PATCH v5 6/8 Remove unused code paths from git_rebase__interactive
In reply to Wink SavilleSince git_rebase__interactive is now never called with $preserve_merges = t we can remove those code paths.
--- git-rebase--interactive.sh | 95 ++-------------------------------------------- 1 file changed, 4 insertions(+), 91 deletions(-)
git-rebase--interactive.sh +4 -91
@@ -961,100 +961,13 @@ git_rebase__interactive () { setup_reflog_action init_basic_state - if test t = "$preserve_merges" - then - if test -z "$rebase_root" - then - mkdir "$rewritten" && - for c in $(git merge-base --all $orig_head $upstream) - do - echo $onto > "$rewritten"/$c || - die "$(gettext "Could not init rewritten commits")" - done - else - mkdir "$rewritten" && - echo $onto > "$rewritten"/root || - die "$(gettext "Could not init rewritten commits")" - fi - # No cherry-pick because our first pass is to determine - # parents to rewrite and skipping dropped commits would - # prematurely end our probe - merges_option= - else - merges_option="--no-merges --cherry-pick" - fi + merges_option="--no-merges --cherry-pick" init_revisions_and_shortrevisions - if test t != "$preserve_merges" - then - git rebase--helper --make-script ${keep_empty:+--keep-empty} \ - $revisions ${restrict_revision+^$restrict_revision} >"$todo" || - die "$(gettext "Could not generate todo list")" - else - format=$(git config --get rebase.instructionFormat) - # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse - git rev-list $merges_option --format="%m%H ${format:-%s}" \ - --reverse --left-right --topo-order \ - $revisions ${restrict_revision+^$restrict_revision} | \ - sed -n "s/^>//p" | - while read -r sha1 rest - do - - if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1 - then - comment_out="$comment_char " - else - comment_out= - fi - - if test -z "$rebase_root" - then - preserve=t - for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-) - do - if test -f "$rewritten"/$p - then - preserve=f - fi - done - else - preserve=f - fi - if test f = "$preserve" - then - touch "$rewritten"/$sha1 - printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo" - fi - done - fi - - # Watch for commits that been dropped by --cherry-pick - if test t = "$preserve_merges" - then - mkdir "$dropped" - # Save all non-cherry-picked changes - git rev-list $revisions --left-right --cherry-pick | \ - sed -n "s/^>//p" > "$state_dir"/not-cherry-picks - # Now all commits and note which ones are missing in - # not-cherry-picks and hence being dropped - git rev-list $revisions | - while read rev - do - if test -f "$rewritten"/$rev && - ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null - then - # Use -f2 because if rev-list is telling us this commit is - # not worthwhile, we don't want to track its multiple heads, - # just the history of its first-parent for others that will - # be rebasing on top of it - git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev - sha1=$(git rev-list -1 $rev) - sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo" - rm "$rewritten"/$rev - fi - done - fi + git rebase--helper --make-script ${keep_empty:+--keep-empty} \ + $revisions ${restrict_revision+^$restrict_revision} >"$todo" || + die "$(gettext "Could not generate todo list")" complete_action }
Signature · 1 line
2.16.2
RFC PATCH v5 7/8 Remove unused code paths from git_rebase__interactive__preserve_merges
In reply to Wink SavilleSince git_rebase__interactive__preserve_merges is now always called with $preserve_merges = t we can remove the unused code paths.
--- git-rebase--interactive.sh | 152 ++++++++++++++++++++------------------------- 1 file changed, 69 insertions(+), 83 deletions(-)
git-rebase--interactive.sh +69 -83
@@ -982,100 +982,86 @@ git_rebase__interactive__preserve_merges () { setup_reflog_action init_basic_state - if test t = "$preserve_merges" + if test -z "$rebase_root" then - if test -z "$rebase_root" - then - mkdir "$rewritten" && - for c in $(git merge-base --all $orig_head $upstream) - do - echo $onto > "$rewritten"/$c || - die "$(gettext "Could not init rewritten commits")" - done - else - mkdir "$rewritten" && - echo $onto > "$rewritten"/root || + mkdir "$rewritten" && + for c in $(git merge-base --all $orig_head $upstream) + do + echo $onto > "$rewritten"/$c || die "$(gettext "Could not init rewritten commits")" - fi - # No cherry-pick because our first pass is to determine - # parents to rewrite and skipping dropped commits would - # prematurely end our probe - merges_option= + done else - merges_option="--no-merges --cherry-pick" + mkdir "$rewritten" && + echo $onto > "$rewritten"/root || + die "$(gettext "Could not init rewritten commits")" fi + # No cherry-pick because our first pass is to determine + # parents to rewrite and skipping dropped commits would + # prematurely end our probe + merges_option= + init_revisions_and_shortrevisions - if test t != "$preserve_merges" - then - git rebase--helper --make-script ${keep_empty:+--keep-empty} \ - $revisions ${restrict_revision+^$restrict_revision} >"$todo" || - die "$(gettext "Could not generate todo list")" - else - format=$(git config --get rebase.instructionFormat) - # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse - git rev-list $merges_option --format="%m%H ${format:-%s}" \ - --reverse --left-right --topo-order \ - $revisions ${restrict_revision+^$restrict_revision} | \ - sed -n "s/^>//p" | - while read -r sha1 rest - do + format=$(git config --get rebase.instructionFormat) + # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse + git rev-list $merges_option --format="%m%H ${format:-%s}" \ + --reverse --left-right --topo-order \ + $revisions ${restrict_revision+^$restrict_revision} | \ + sed -n "s/^>//p" | + while read -r sha1 rest + do - if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1 - then - comment_out="$comment_char " - else - comment_out= - fi + if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1 + then + comment_out="$comment_char " + else + comment_out= + fi - if test -z "$rebase_root" - then - preserve=t - for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-) - do - if test -f "$rewritten"/$p - then - preserve=f - fi - done - else - preserve=f - fi - if test f = "$preserve" - then - touch "$rewritten"/$sha1 - printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo" - fi - done - fi + if test -z "$rebase_root" + then + preserve=t + for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-) + do + if test -f "$rewritten"/$p + then + preserve=f + fi + done + else + preserve=f + fi + if test f = "$preserve" + then + touch "$rewritten"/$sha1 + printf '%s\n' "${comment_out}pick $sha1 $rest" >>"$todo" + fi + done # Watch for commits that been dropped by --cherry-pick - if test t = "$preserve_merges" - then - mkdir "$dropped" - # Save all non-cherry-picked changes - git rev-list $revisions --left-right --cherry-pick | \ - sed -n "s/^>//p" > "$state_dir"/not-cherry-picks - # Now all commits and note which ones are missing in - # not-cherry-picks and hence being dropped - git rev-list $revisions | - while read rev - do - if test -f "$rewritten"/$rev && - ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null - then - # Use -f2 because if rev-list is telling us this commit is - # not worthwhile, we don't want to track its multiple heads, - # just the history of its first-parent for others that will - # be rebasing on top of it - git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev - sha1=$(git rev-list -1 $rev) - sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo" - rm "$rewritten"/$rev - fi - done - fi + mkdir "$dropped" + # Save all non-cherry-picked changes + git rev-list $revisions --left-right --cherry-pick | \ + sed -n "s/^>//p" > "$state_dir"/not-cherry-picks + # Now all commits and note which ones are missing in + # not-cherry-picks and hence being dropped + git rev-list $revisions | + while read rev + do + if test -f "$rewritten"/$rev && + ! sane_grep "$rev" "$state_dir"/not-cherry-picks >/dev/null + then + # Use -f2 because if rev-list is telling us this commit is + # not worthwhile, we don't want to track its multiple heads, + # just the history of its first-parent for others that will + # be rebasing on top of it + git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev + sha1=$(git rev-list -1 $rev) + sane_grep -v "^[a-z][a-z]* $sha1" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo" + rm "$rewritten"/$rev + fi + done complete_action }
Signature · 1 line
2.16.2
RFC PATCH v5 8/8 Remove merges_option and a blank line
In reply to Wink Savillemerges_option is unused in git_rebase__interactive and always empty in git_rebase__interactive__preserve_merges so it can be removed.
--- git-rebase--interactive.sh | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
git-rebase--interactive.sh +1 -9
@@ -961,8 +961,6 @@ git_rebase__interactive () { setup_reflog_action init_basic_state - merges_option="--no-merges --cherry-pick" - init_revisions_and_shortrevisions git rebase--helper --make-script ${keep_empty:+--keep-empty} \
@@ -996,22 +994,16 @@ git_rebase__interactive__preserve_merges () { die "$(gettext "Could not init rewritten commits")" fi - # No cherry-pick because our first pass is to determine - # parents to rewrite and skipping dropped commits would - # prematurely end our probe - merges_option= - init_revisions_and_shortrevisions format=$(git config --get rebase.instructionFormat) # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse - git rev-list $merges_option --format="%m%H ${format:-%s}" \ + git rev-list --format="%m%H ${format:-%s}" \ --reverse --left-right --topo-order \ $revisions ${restrict_revision+^$restrict_revision} | \ sed -n "s/^>//p" | while read -r sha1 rest do - if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit $sha1 then comment_out="$comment_char "
Signature · 1 line
2.16.2
Quoted 17 lines · Wink Saville
Reworked patch 1 so that all of the backend scriptlets used by git-rebase use a normal function style invocation.
Merged the previous patch 2 and 3 have been squashed which provides reviewers a little easier time to detect any changes during extraction of the functions.
Wink Saville (8):
rebase-interactive: simplifypick_on_preserving_merges
rebase: update invocation of rebase dot-sourced scripts Indent functiongit_rebase__interactive
Extract functions out ofgit_rebase__interactive
Add and usegit_rebase__interactive__preserve_mergesRemove unused code paths fromgit_rebase__interactiveRemove unused code paths fromgit_rebase__interactive__preserve_mergesRemovemerges_optionand a blank line
Quoted 9 lines
git-rebase--am.sh | 11 -- git-rebase--interactive.sh | 407 ++++++++++++++++++++++++--------------------- git-rebase--merge.sh | 11 -- git-rebase.sh | 1 + 4 files changed, 216 insertions(+), 214 deletions(-)--
2.16.2
Argh, I misspelled Junio's email address, so when you reply-all try to remember to remove "[email protected]" from the cc: list.
Sorry,
Wink
Quoted 24 lines · Wink Saville
Quoted 23 lines · Wink Saville
Reworked patch 1 so that all of the backend scriptlets used by git-rebase use a normal function style invocation.
Merged the previous patch 2 and 3 have been squashed which provides reviewers a little easier time to detect any changes during extraction of the functions.
Wink Saville (8):
rebase-interactive: simplifypick_on_preserving_merges
rebase: update invocation of rebase dot-sourced scripts Indent functiongit_rebase__interactive
Extract functions out ofgit_rebase__interactive
Add and usegit_rebase__interactive__preserve_mergesRemove unused code paths fromgit_rebase__interactiveRemove unused code paths fromgit_rebase__interactive__preserve_mergesRemovemerges_optionand a blank linegit-rebase--am.sh | 11 -- git-rebase--interactive.sh | 407 ++++++++++++++++++++++++--------------------- git-rebase--merge.sh | 11 -- git-rebase.sh | 1 + 4 files changed, 216 insertions(+), 214 deletions(-)
Quoted 6 lines
Quoted 3 lines
--
2.16.2Argh, I misspelled Junio's email address, so when you reply-all try to remember to remove "[email protected]" from the cc: list.
Heh, too late ;-)
I queued everything (with all patch 3-8/8 retitled to share a common prefix, so that "git shortlog" output would stay sane) and I think I resolved the conflicts with Dscho's recreate-merges topic correctly. Please double check what will appear on 'pu' later today.
Thanks.
Quoted 35 lines · Junio C Hamano
Quoted 24 lines · Wink Saville
Quoted 23 lines · Wink Saville
Reworked patch 1 so that all of the backend scriptlets used by git-rebase use a normal function style invocation.
Merged the previous patch 2 and 3 have been squashed which provides reviewers a little easier time to detect any changes during extraction of the functions.
Wink Saville (8):
rebase-interactive: simplifypick_on_preserving_merges
rebase: update invocation of rebase dot-sourced scripts Indent functiongit_rebase__interactive
Extract functions out ofgit_rebase__interactive
Add and usegit_rebase__interactive__preserve_mergesRemove unused code paths fromgit_rebase__interactiveRemove unused code paths fromgit_rebase__interactive__preserve_mergesRemovemerges_optionand a blank linegit-rebase--am.sh | 11 -- git-rebase--interactive.sh | 407 ++++++++++++++++++++++++--------------------- git-rebase--merge.sh | 11 -- git-rebase.sh | 1 + 4 files changed, 216 insertions(+), 214 deletions(-)Quoted 6 lines
Quoted 3 lines
--
2.16.2Argh, I misspelled Junio's email address, so when you reply-all try to remember to remove "[email protected]" from the cc: list.
Heh, too late ;-)
Quoted 8 lines
I queued everything (with all patch 3-8/8 retitled to share a common prefix, so that "git shortlog" output would stay sane) and I think I resolved the conflicts with Dscho's recreate-merges topic correctly. Please double check what will appear on 'pu' later today.
Thanks.
OK, thank you!
Quoted 10 lines
Quoted 8 lines
I queued everything (with all patch 3-8/8 retitled to share a common prefix, so that "git shortlog" output would stay sane) and I think I resolved the conflicts with Dscho's recreate-merges topic correctly. Please double check what will appear on 'pu' later today.
Thanks.
OK, thank you!
I looked at 'pu' and it LGTM, so I pushed 'pu' as a branch to my github account to test with Travis-CI. All the linux builds are green, but the 2 OSX builds are red[1] and the logs show compile errors:
CC ident.o
CC json-writer.o
json-writer.c:123:38: error: format specifies type 'uintmax_t' (aka 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned long long') [-Werror,-Wformat]
strbuf_addf(&jw->json, ":%"PRIuMAX, value);
~~ ^~~~~json-writer.c:228:37: error: format specifies type 'uintmax_t' (aka 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned long long') [-Werror,-Wformat] [0m
strbuf_addf(&jw->json, "%"PRIuMAX, value);
~~ ^~~~~2 errors generated.
make: *** [json-writer.o] Error 1
make: *** Waiting for unfinished jobs....
[RFC Patch 1/1] changes the PRIuMax to PRIu64 to correct the compile error and now Travis-CI is green [2].
[1]: https://travis-ci.org/winksaville/git/builds/357660624
[2]: https://travis-ci.org/winksaville/git/builds/357681929
-- Wink
Quoted 10 lines · Wink Saville
json-writer.c:123:38: error: format specifies type '
uintmax_t' (aka 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned long long') [-Werror,-Wformat]strbuf_addf(&jw->json, ":%"PRIuMAX, value); ~~ ^~~~~json-writer.c:228:37: error: format specifies type '
uintmax_t' (aka 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned long long') [-Werror,-Wformat] [0m
Quoted 5 lines
strbuf_addf(&jw->json, "%"PRIuMAX, value); ~~ ^~~~~2 errors generated.
make: *** [json-writer.o] Error 1
make: *** Waiting for unfinished jobs....
For whatever reason, our codebase seems to shy away from PRIu64, even though there are liberal uses of PRIu32. Showing the value casted to uintmax_t with PRIuMAX seems to be our preferred way to say "We cannot say how wide this type is on different platforms, and are playing safe by using widest-possible int type" (e.g. showing a pid_t value from daemon.c).
In this codepath, the actual values are specified to be uint64_t, so the use of PRIu64 may be OK, but I have to wonder why the codepath is not dealing with uintmax_t in the first place. When even larger than present archs are prevalent in N years and 64-bit starts to feel a tad small (like we feel for 16-bit ints these days), it will feel a bit silly to have a subsystem that is limited to such a "fixed and a tad small these days" types and pretend it to be be a generic seriealizer, I suspect.
Quoted 25 lines · Junio C Hamano
Quoted 10 lines · Wink Saville
json-writer.c:123:38: error: format specifies type '
uintmax_t' (aka 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned long long') [-Werror,-Wformat]strbuf_addf(&jw->json, ":%"PRIuMAX, value); ~~ ^~~~~json-writer.c:228:37: error: format specifies type '
uintmax_t' (aka 'unsigned long') but the argument has type 'uint64_t' (aka 'unsigned long long') [-Werror,-Wformat] [0mQuoted 5 lines
strbuf_addf(&jw->json, "%"PRIuMAX, value); ~~ ^~~~~2 errors generated.
make: *** [json-writer.o] Error 1
make: *** Waiting for unfinished jobs....For whatever reason, our codebase seems to shy away from PRIu64, even though there are liberal uses of PRIu32. Showing the value casted to
uintmax_twith PRIuMAX seems to be our preferred way to say "We cannot say how wide this type is on different platforms, and are playing safe by using widest-possible int type" (e.g. showing apid_tvalue fromdaemon.c).
Quoted 9 lines
In this codepath, the actual values are specified to be
uint64_t, so the use of PRIu64 may be OK, but I have to wonder why the codepath is not dealing withuintmax_tin the first place. When even larger than present archs are prevalent in N years and 64-bit starts to feel a tad small (like we feel for 16-bit ints these days), it will feel a bit silly to have a subsystem that is limited to such a "fixed and a tad small these days" types and pretend it to be be a generic seriealizer, I suspect.
I defined that routine to take a uint64_t because I wanted to pass a nanosecond value received from getnanotime() and that's what it returns.
My preference would be to change the PRIuMAX to PRIu64, but there aren't any other references in the code to that symbol and I didn't want to start a new trend here.
I am concerned that the above compiler error message says that uintmax_t is defined as an "unsigned long" (which is defined as *at least* 32 bits, but not necessarily 64. But a uint64_t is defined as a "unsigned long long" and guaranteed as a 64 bit value.
So while I'm not really worried about 128 bit integers right now, I'm more concerned about 32 bit compilers truncating that value without any warnings.
Jeff
Quoted 3 lines · Jeff Hostetler
I defined that routine to take a
uint64_tbecause I wanted to pass a nanosecond value received fromgetnanotime()and that's what it returns.
Hmph, but the target format does not have different representation of inttypes in different sizes, no?
I personally doubt that we would benefit from having a group of functions (i.e. format_int{8,16,32,64}_to_json()) that callers have to choose from, depending on the exact size of the integer they want to serialize. The de-serializing side would be the same story.
Even if the variable a potential caller of the formetter is a sized type that is different from uintmax_t, the caller shouldn't have to add an extra cast.
Am I missing some obvious merit for having these separate functions for explicit sizes?
Quoted 14 lines · Junio C Hamano
Quoted 3 lines · Jeff Hostetler
I defined that routine to take a
uint64_tbecause I wanted to pass a nanosecond value received fromgetnanotime()and that's what it returns.Hmph, but the target format does not have different representation of inttypes in different sizes, no?
I personally doubt that we would benefit from having a group of functions (i.e. format_int{8,16,32,64}_to_json()) that callers have to choose from, depending on the exact size of the integer they want to serialize. The de-serializing side would be the same story.
Quoted 7 lines
Even if the variable a potential caller of the formetter is a sized type that is different from
uintmax_t, the caller shouldn't have to add an extra cast.Am I missing some obvious merit for having these separate functions for explicit sizes?
I did the uint64_t for the unsigned ns times.
I did the other one for the usual signed ints.
I could convert them both to a single signed 64 bit typed function if we only want to have one function.
Jeff
Quoted 6 lines · Jeff Hostetler
I did the
uint64_tfor the unsigned ns times.I did the other one for the usual signed ints.
I could convert them both to a single signed 64 bit typed function if we only want to have one function.
I still think having sized version is a horrible idea, and recommend instrad to use "the widest possible on the platform" type, for the same reason why you would only have variant for double but not for float.
intmax and uintmax are by definition wide enough to hold any value that would fit in any integral type the platform supports, so if a caller that wants to handle 64-bit unsigned timestamp for example uses uint64_t variable to pass such a timestamp around, and the platform is capable of groking that code, you should be able to safely pass that to json serializer you are writing that takes uintmax_t just fine, and (1) your caller that passes around uint64_t timestamps won't compile on a platform that is incapable of doing 64-bit and you have bigger problem than uintmax_t being narrower than 64-bit on such a platform, and (2) your caller can just pass uint64_t value to your JSON formatter that expects uintmax_t without explicit casting, as normal integral type promotion rule would apply.
So I would think it is most sensible to have double, uintmax_t and intmax_t variants. If you do not care about the extra value range that unsigned integral types afford, a single intmax_t variant would also be fine.
Quoted 1 line · Junio C Hamano
Jeff Hostetler <[email protected]> writes:
[...]
Quoted 4 lines
So I would think it is most sensible to have double,
uintmax_tandintmax_tvariants. If you do not care about the extra value range that unsigned integral types afford, a singleintmax_tvariant would also be fine.
I'll reroll with just the double and intmax_t variants. Thanks for the feedback and sorry for all the noise.
Jeff
Quoted 4 lines · Jeff Hostetler
I am concerned that the above compiler error message says that
uintmax_tis defined as an "unsigned long" (which is defined as *at least* 32 bits, but not necessarily 64. But auint64_tis defined as a "unsigned long long" and guaranteed as a 64 bit value.
On a platform whose uintmax_t is u32, is it realistic to expect that we would be able to use u64, even if we explicitly ask for it, in the first place?
In other words, on a platform that handles uint64_t, I would expect uintmax_t to be wide enough to hold an uint64_t value without truncation.
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Jeff Hostetler
I am concerned that the above compiler error message says that
uintmax_tis defined as an "unsigned long" (which is defined as *at least* 32 bits, but not necessarily 64. But auint64_tis defined as a "unsigned long long" and guaranteed as a 64 bit value.
Quoted 8 lines
On a platform whose
uintmax_tis u32, is it realistic to expect that we would be able to use u64, even if we explicitly ask for it, in the first place?In other words, on a platform that handles
uint64_t, I would expectuintmax_tto be wide enough to hold anuint64_tvalue without truncation.
I was just going by what the reported compiler error message was. It said that "unsigned long" didn't match the uint64_t variable. And that made me nervous.
If all of the platforms we build on define uintmax_t >= 64 bits, then it doesn't matter.
If we do have a platform where uintmax_t is u32, then we'll have a lot more breakage than in just the new function I added.
Thanks,
Jeff
Quoted 4 lines
I was just going by what the reported compiler error message was. It said that "unsigned long" didn't match the
uint64_tvariable. And that made me nervous.
Quoted 8 lines
If all of the platforms we build on define
uintmax_t>= 64 bits, then it doesn't matter.If we do have a platform where
uintmax_tis u32, then we'll have a lot more breakage than in just the new function I added.Thanks,
Jeff
Should we add a "_Static_assert" that sizeof(uintmax_t) >= sizeof(uint64_t) ?
Quoted 1 line · Wink Saville
Should we add a "
_Static_assert" thatsizeof(uintmax_t)>=sizeof(uint64_t)?
If that expression compiles, then both types are understood by the platform. Because
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html
tells us:
Greatest-width integer types
The following type designates a signed integer type capable of representing any value of any signed integer type: intmax_t
The following type designates an unsigned integer type capable of representing any value of any unsigned integer type: uintmax_t
These types are required.
we know what that expression evaluates to, no?
Hi all,
Quoted 7 lines · Wink Saville
Quoted 7 lines
I was just going by what the reported compiler error message was. It said that "unsigned long" didn't match the
uint64_tvariable. And that made me nervous.If all of the platforms we build on define
uintmax_t>= 64 bits, then it doesn't matter.
Quoted 7 lines
Quoted 5 lines
If we do have a platform where
uintmax_tis u32, then we'll have a lot more breakage than in just the new function I added.Thanks,
JeffShould we add a "
_Static_assert" thatsizeof(uintmax_t)>=sizeof(uint64_t)?
To come back to the subject that all of the mails in this here mail thread of you gentle people bear: Wink's patches look good to me, and I would like to have them be bumped down the next/master waterfall.
Ciao,
Dscho
Quoted 9 lines · Wink Saville
Wink Saville (8):
rebase-interactive: simplifypick_on_preserving_merges
rebase: update invocation of rebase dot-sourced scripts Indent functiongit_rebase__interactive
Extract functions out ofgit_rebase__interactive
Add and usegit_rebase__interactive__preserve_mergesRemove unused code paths fromgit_rebase__interactiveRemove unused code paths fromgit_rebase__interactive__preserve_mergesRemovemerges_optionand a blank line
I felt that the structure of steps 5-7 that adds an identical copy first and then removes irrelevant parts from both copies was a bit unusual, but I do not think of a better structure I would use if I were doing this series myself, and more importantly, the entire series was a pleasant and straight-forward read.
Will queue and wait for input from others.
Thanks.