[RFC PATCH v4] rebase-interactive

[RFC PATCH v4] rebase-interactive

[Wink Saville]

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(-)

-- 
2.16.2

[RFC PATCH v4] rebase-interactive: Simplify pick_on_preserving_merges

[Wink Saville]

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

Re: [RFC PATCH v4] rebase-interactive: Simplify pick_on_preserving_merges

[Johannes Schindelin]

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

[RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts

[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.

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(-)

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

Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts

[Eric Sunshine]

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__$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

[Junio C Hamano]

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.

Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts

[Eric Sunshine]

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.

Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts

[Johannes Schindelin]

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

Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts

[Wink Saville]

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

Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts

[Junio C Hamano]

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.

Re: [RFC PATCH v4] rebase: Update invocation of rebase dot-sourced scripts

[Wink Saville]

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

[RFC PATCH v5 0/8] rebase-interactive

[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: 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

Re: [RFC PATCH v5 0/8] rebase-interactive

[Wink Saville]

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

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

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.

Re: [RFC PATCH v5 0/8] rebase-interactive

[Wink Saville]

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!

Re: [RFC PATCH v5 0/8] rebase-interactive

[Wink Saville]

>> 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

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

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

[Jeff Hostetler]

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

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

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

[Jeff Hostetler]

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

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

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

[Jeff Hostetler]

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

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

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

[Jeff Hostetler]

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

Re: [RFC PATCH v5 0/8] rebase-interactive

[Wink Saville]

> 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) ?

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

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_t

    These types are required.

we know what that expression evaluates to, no?

Re: [RFC PATCH v5 0/8] rebase-interactive

[Johannes Schindelin]

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

Re: [RFC PATCH v5 0/8] rebase-interactive

[Junio C Hamano]

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.

[RFC PATCH v5 1/8] rebase-interactive: simplify pick_on_preserving_merges

[Wink Saville]

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

[RFC PATCH v5 2/8] rebase: update invocation of rebase dot-sourced scripts

[Wink Saville]

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(-)

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

[RFC PATCH v5 3/8] Indent function git_rebase__interactive

[Wink Saville]

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

Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive

[Junio C Hamano]

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.

Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive

[Wink Saville]

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

Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive

[Junio C Hamano]

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.

Re: [RFC PATCH v5 3/8] Indent function git_rebase__interactive

[Wink Saville]

> 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

[Wink Saville]

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

Re: [RFC PATCH v5 4/8] Extract functions out of git_rebase__interactive

[Junio C Hamano]

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.

Re: [RFC PATCH v5 4/8] Extract functions out of git_rebase__interactive

[Eric Sunshine]

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 any

s/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 () {

[RFC PATCH v5 5/8] Add and use git_rebase__interactive__preserve_merges

[Wink Saville]

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(-)

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

[RFC PATCH v5 6/8] Remove unused code paths from git_rebase__interactive

[Wink Saville]

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

[RFC PATCH v5 7/8] Remove unused code paths from git_rebase__interactive__preserve_merges

[Wink Saville]

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

[RFC PATCH v5 8/8] Remove merges_option and a blank line

[Wink Saville]

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