Wink SavilleThis 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(-)
-- 2.16.2
Wink Saville[RFC PATCH v4] rebase-interactive: Simplify pick_on_preserving_merges
Use compound if statement instead of nested if statements to simplify pick_on_preserving_merges.
Signed-off-by: Wink Saville <wink@saville.com> Reviewed-by: Junio C Hamano <gister@pobox.com> --- git-rebase--interactive.sh | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 331c8dfea..561e2660e 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -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
-- 2.16.2
Johannes SchindelinRe: [RFC PATCH v4] rebase-interactive: Simplify pick_on_preserving_merges
Hi Wink,
On Thu, 22 Mar 2018, Wink Saville wrote:
> Use compound if statement instead of nested if statements to > simplify pick_on_preserving_merges. > > Signed-off-by: Wink Saville <wink@saville.com> > Reviewed-by: Junio C Hamano <gister@pobox.com> > --- > git-rebase--interactive.sh | 17 +++++++----------
The patch is obviously correct.
Thanks, Johannes
Wink Saville[RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
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.
Signed-off-by: Wink Saville <wink@saville.com> Reviewed-by: Junio C Hamano <gitster@pobox.com> Reviewed-by: Eric Sunsine <sunsine@sunshineco.com> --- 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, git-rebase--interactive.sh, git-rebase--merge.sh, git-rebase.sh
diff --git a/git-rebase--am.sh b/git-rebase--am.sh index be3f06892..e5fd6101d 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -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 diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 561e2660e..213d75f43 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -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 diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh index ceb715453..685f48ca4 100644 --- a/git-rebase--merge.sh +++ b/git-rebase--merge.sh @@ -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 diff --git a/git-rebase.sh b/git-rebase.sh index a1f6e5de6..4595a316a 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -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
-- 2.16.2
Eric SunshineRe: [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.)
On Fri, Mar 23, 2018 at 12:39 AM, Wink Saville <wink@saville.com> wrote:
> rebase: Update invocation of rebase dot-sourced scripts
Nit: On this project, the summary line is not capitalized, so: s/Update/update/
> 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.
> 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"
> 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.
> Here the call is moved to "git rebase" side, instead of at the
Junio's version, using imperative mood, said "Move the call...".
> 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.> Signed-off-by: Wink Saville <wink@saville.com> > Reviewed-by: Junio C Hamano <gitster@pobox.com> > Reviewed-by: Eric Sunsine <sunsine@sunshineco.com>
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/xmqqefkbltxv.fsf@gitster-ct.c.googlers.com/
> ---
> diff --git a/git-rebase.sh b/git-rebase.sh
> index a1f6e5de6..4595a316a 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -196,7 +196,9 @@ run_specific_rebase () {
> export GIT_EDITOR
> autosquash=
> fi
> + # Source the code and invoke it
> . git-rebase--$type
> + git_rebase__$typeThe 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.
Junio C HamanoRe: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Eric Sunshine <sunshine@sunshineco.com> writes:
>> 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.
Eric SunshineRe: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
On Fri, Mar 23, 2018 at 5:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes: >> (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.
Johannes SchindelinRe: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Hi Wink,
On Thu, 22 Mar 2018, Wink Saville wrote:
> 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. > > Signed-off-by: Wink Saville <wink@saville.com> > Reviewed-by: Junio C Hamano <gitster@pobox.com> > Reviewed-by: Eric Sunsine <sunsine@sunshineco.com> > --- > 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
Wink SavilleRe: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
On Fri, Mar 23, 2018 at 10:12 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi Wink, > > On Thu, 22 Mar 2018, Wink Saville wrote: > >> 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. >> >> Signed-off-by: Wink Saville <wink@saville.com> >> Reviewed-by: Junio C Hamano <gitster@pobox.com> >> Reviewed-by: Eric Sunsine <sunsine@sunshineco.com> >> --- >> 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
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 --to=git@vger.kernel.org --cc=sunshine@sunshineco.com --cc=Johannes.Schindelin@gmx.de -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
Junio C HamanoRe: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
Wink Saville <wink@saville.com> writes:
> Here is one possibility: > > git format-patch --cover-letter --rfc --thread -v 5 > --to=git@vger.kernel.org --cc=sunshine@sunshineco.com > --cc=Johannes.Schindelin@gmx.de -o patches/v5 master..v5-2
Sounds sensible.
> 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.
> 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.
Wink SavilleRe: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts
On Fri, Mar 23, 2018 at 1:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Wink Saville <wink@saville.com> writes: > >> Here is one possibility: >> >> git format-patch --cover-letter --rfc --thread -v 5 >> --to=git@vger.kernel.org --cc=sunshine@sunshineco.com >> --cc=Johannes.Schindelin@gmx.de -o patches/v5 master..v5-2 > > Sounds sensible. > >> 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. > >> 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
Wink Saville[RFC PATCH v5 0/8] rebase-interactive
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: 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(-)
-- 2.16.2
Wink Saville[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.
Signed-off-by: Wink Saville <wink@saville.com> --- git-rebase--interactive.sh | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 331c8dfea..561e2660e 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -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
-- 2.16.2
Wink Saville[RFC PATCH v5 2/8] rebase: update invocation of rebase dot-sourced scripts
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.
Signed-off-by: Wink Saville <wink@saville.com> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> --- 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, git-rebase--interactive.sh, git-rebase--merge.sh, git-rebase.sh
diff --git a/git-rebase--am.sh b/git-rebase--am.sh index be3f06892..e5fd6101d 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -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 diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 561e2660e..213d75f43 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -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 diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh index ceb715453..685f48ca4 100644 --- a/git-rebase--merge.sh +++ b/git-rebase--merge.sh @@ -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 diff --git a/git-rebase.sh b/git-rebase.sh index a1f6e5de6..6edf8c5b1 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -197,6 +197,7 @@ run_specific_rebase () { autosquash= fi . git-rebase--$type + git_rebase__$type ret=$? if test $ret -eq 0 then
-- 2.16.2
Wink Saville[RFC PATCH v5 3/8] Indent function git_rebase__interactive
Signed-off-by: Wink Saville <wink@saville.com> --- git-rebase--interactive.sh | 432 ++++++++++++++++++++++----------------------- 1 file changed, 215 insertions(+), 217 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 213d75f43..a79330f45 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -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 }
-- 2.16.2
Junio C HamanoRe: [RFC PATCH v5 3/8] Indent function git_rebase__interactive
Wink Saville <wink@saville.com> writes:
> Signed-off-by: Wink Saville <wink@saville.com> > --- > 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.
Wink SavilleRe: [RFC PATCH v5 3/8] Indent function git_rebase__interactive
On Fri, Mar 23, 2018 at 3:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Wink Saville <wink@saville.com> writes: > >> Signed-off-by: Wink Saville <wink@saville.com> >> --- >> 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. >
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
Junio C HamanoRe: [RFC PATCH v5 3/8] Indent function git_rebase__interactive
Wink Saville <wink@saville.com> writes:
> 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.
Wink SavilleRe: [RFC PATCH v5 3/8] Indent function 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.
Then lets leave the long lines alone for this series.
Wink Saville[RFC PATCH v5 4/8] Extract functions out of git_rebase__interactive
The extracted functions are: - initiate_action - setup_reflog_action - init_basic_state - init_revisions_and_shortrevisions - complete_action
Used by git_rebase__interactive
Signed-off-by: Wink Saville <wink@saville.com> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> --- git-rebase--interactive.sh | 182 +++++++++++++++++++++++++++------------------ 1 file changed, 111 insertions(+), 71 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index a79330f45..2c10a7f1a 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -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 }
-- 2.16.2
Junio C HamanoRe: [RFC PATCH v5 4/8] Extract functions out of git_rebase__interactive
Wink Saville <wink@saville.com> writes:
> The extracted functions are: > - initiate_action > - setup_reflog_action > - init_basic_state > - init_revisions_and_shortrevisions > - complete_action > > Used by git_rebase__interactive > > Signed-off-by: Wink Saville <wink@saville.com> > Helped-by: Junio C Hamano <gitster@pobox.com> > Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > ---
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.
Eric SunshineRe: [RFC PATCH v5 4/8] Extract functions out of git_rebase__interactive
On Fri, Mar 23, 2018 at 5:25 PM, Wink Saville <wink@saville.com> wrote:
> The extracted functions are:
> [...]
> Signed-off-by: Wink Saville <wink@saville.com>
> ---
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> @@ -740,8 +740,20 @@ get_missing_commit_check_level () {
> +# Initiate an action. If the cannot be anys/the/there/
> +# further action it may exec a command
s/\s+/ /
> +# 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 () {Wink Saville[RFC PATCH v5 5/8] Add and use git_rebase__interactive__preserve_merges
At the moment it's an exact copy of git_rebase__interactive except the name has changed.
Signed-off-by: Wink Saville <wink@saville.com> --- git-rebase--interactive.sh | 108 +++++++++++++++++++++++++++++++++++++++++++++ git-rebase.sh | 2 +- 2 files changed, 109 insertions(+), 1 deletion(-)
git-rebase--interactive.sh, git-rebase.sh
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 2c10a7f1a..ab5513d80 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -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 +} diff --git a/git-rebase.sh b/git-rebase.sh index 6edf8c5b1..fb64ee1fe 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -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
-- 2.16.2
Wink Saville[RFC PATCH v5 6/8] Remove unused code paths from git_rebase__interactive
Since git_rebase__interactive is now never called with $preserve_merges = t we can remove those code paths.
Signed-off-by: Wink Saville <wink@saville.com> --- git-rebase--interactive.sh | 95 ++-------------------------------------------- 1 file changed, 4 insertions(+), 91 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index ab5513d80..346da0f67 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -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 }
-- 2.16.2
Wink Saville[RFC PATCH v5 7/8] Remove unused code paths from git_rebase__interactive__preserve_merges
Since git_rebase__interactive__preserve_merges is now always called with $preserve_merges = t we can remove the unused code paths.
Signed-off-by: Wink Saville <wink@saville.com> --- git-rebase--interactive.sh | 152 ++++++++++++++++++++------------------------- 1 file changed, 69 insertions(+), 83 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 346da0f67..ddbd126f2 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -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 }
-- 2.16.2
Wink Saville[RFC PATCH v5 8/8] Remove merges_option and a blank line
merges_option is unused in git_rebase__interactive and always empty in git_rebase__interactive__preserve_merges so it can be removed.
Signed-off-by: Wink Saville <wink@saville.com> --- git-rebase--interactive.sh | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index ddbd126f2..50323fc27 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -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 "
-- 2.16.2
Wink SavilleRe: [RFC PATCH v5 0/8] rebase-interactive
On Fri, Mar 23, 2018 at 2:25 PM, Wink Saville <wink@saville.com> wrote:
> 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: 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(-) > > -- > 2.16.2 >
Argh, I misspelled Junio's email address, so when you reply-all try to remember to remove "gister@pobox.com" from the cc: list.
Sorry, Wink
Junio C HamanoRe: [RFC PATCH v5 0/8] rebase-interactive
Wink Saville <wink@saville.com> writes:
> On Fri, Mar 23, 2018 at 2:25 PM, Wink Saville <wink@saville.com> wrote: >> 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: 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(-) >> >> -- >> 2.16.2 >> > > Argh, I misspelled Junio's email address, so when you reply-all try > to remember to remove "gister@pobox.com" 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.
Wink SavilleRe: [RFC PATCH v5 0/8] rebase-interactive
On Fri, Mar 23, 2018 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Wink Saville <wink@saville.com> writes: > >> On Fri, Mar 23, 2018 at 2:25 PM, Wink Saville <wink@saville.com> wrote: >>> 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: 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(-) >>> >>> -- >>> 2.16.2 >>> >> >> Argh, I misspelled Junio's email address, so when you reply-all try >> to remember to remove "gister@pobox.com" 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. >
OK, thank you!
Wink SavilleRe: [RFC PATCH v5 0/8] rebase-interactive
>> 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.ojson-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
Junio C HamanoRe: [RFC PATCH v5 0/8] rebase-interactive
Wink Saville <wink@saville.com> writes:
> 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....
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.
Re: [RFC PATCH v5 0/8] rebase-interactive
On 3/26/2018 11:56 AM, Junio C Hamano wrote:
> Wink Saville <wink@saville.com> writes: > >> 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.... > > 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. >
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
Junio C HamanoRe: [RFC PATCH v5 0/8] rebase-interactive
Jeff Hostetler <git@jeffhostetler.com> writes:
> 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.
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?
Re: [RFC PATCH v5 0/8] rebase-interactive
On 3/26/2018 1:57 PM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> writes:
>
>> 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.
>
> 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?
> 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
Junio C HamanoRe: [RFC PATCH v5 0/8] rebase-interactive
Jeff Hostetler <git@jeffhostetler.com> writes:
> 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.
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.
Re: [RFC PATCH v5 0/8] rebase-interactive
On 3/27/2018 1:07 AM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> writes:
[...]
> 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.
I'll reroll with just the double and intmax_t variants. Thanks for the feedback and sorry for all the noise.
Jeff
Junio C HamanoRe: [RFC PATCH v5 0/8] rebase-interactive
Jeff Hostetler <git@jeffhostetler.com> writes:
> 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.
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.
Re: [RFC PATCH v5 0/8] rebase-interactive
On 3/26/2018 2:00 PM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> writes: > >> 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. > > 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. >
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
Wink SavilleRe: [RFC PATCH v5 0/8] rebase-interactive
> 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
Should we add a "_Static_assert" that sizeof(uintmax_t) >= sizeof(uint64_t) ?
Junio C HamanoRe: [RFC PATCH v5 0/8] rebase-interactive
Wink Saville <wink@saville.com> writes:
> Should we add a "_Static_assert" that sizeof(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_tThese types are required.
we know what that expression evaluates to, no?
Johannes SchindelinRe: [RFC PATCH v5 0/8] rebase-interactive
Hi all,
On Mon, 26 Mar 2018, Wink Saville wrote:
> > 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 > > Should we add a "_Static_assert" that sizeof(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
Junio C HamanoRe: [RFC PATCH v5 0/8] rebase-interactive
Wink Saville <wink@saville.com> writes:
> 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
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.