[RFC PATCH v2 1/1] rebase-interactive: Add git_rebase__interactive__preserve_merges
Refactor git_rebase__interactive__preserve_merges out of
git_rebase__interactive resulting in fewer conditionals making
both routines are simpler.
Changed run_specific_rebase in git-rebase to dispatch to the appropriate
function after sourcing git-rebase--interactive.
---
git-rebase--interactive.sh | 469 +
git-rebase.sh | 16 +-
2 files changed, 273 insertions(+), 212 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 331c8dfea..65ff75654 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1,5 +1,7 @@
-# This shell script fragment is sourced by git-rebase to implement
-# its interactive mode. "git rebase --interactive" makes it easy
+#!/bin/sh
+# This shell script fragment is sourced by git-rebase--interactive
+# and git-rebase--interactive--preserve-merges in support of the
+# interactive mode. "git rebase --interactive" makes it easy
# to fix up commits in the middle of a series and rearrange commits.
#
# Copyright (c) 2006 Johannes E. Schindelin
@@ -7,6 +9,7 @@
# The original idea comes from Eric W. Biederman, in
# https://public-inbox.org/git/m1odwkyuf5.fsf...@ebiederm.dsl.xmission.com/
#
+
# The file containing rebase commands, comments, and empty lines.
# This file is created by "git rebase -i" then edited by the user. As
# the lines are processed, they are removed from the front of this
@@ -125,6 +128,19 @@ comment_for_reflog () {
esac
}
+setup_reflog_action () {
+ comment_for_reflog start
+
+ 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")"
+
+ comment_for_reflog start
+ fi
+}
+
last_count=
mark_action_done () {
sed -e 1q < "$todo" >> "$done"
@@ -274,7 +290,8 @@ pick_one () {
case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
case "$force_rebase" in '') ;; ?*) ff= ;; esac
- output git rev-parse --verify $sha1 || die "$(eval_gettext "Invalid
commit name: \$sha1")"
+ output git rev-parse --verify $sha1 ||
+ die "$(eval_gettext "Invalid commit name: \$sha1")"
if is_empty_commit "$sha1"
then
@@ -287,8 +304,8 @@ pick_one () {
${gpg_sign_opt:+$(git rev-parse --sq-quote
"$gpg_sign_opt")} \
"$strategy_args" $empty_args $ff "$@"
- # If cherry-pick dies it leaves the to-be-picked commit unrecorded.
Reschedule
- # previous task so this commit is not lost.
+ # If cherry-pick dies it leaves the to-be-picked commit unrecorded.
+ # Reschedule previous task so this commit is not lost.
ret=$?
case "$ret" in [01]) ;; *) reschedule_last_action ;; esac
return $ret
@@ -307,17 +324,15 @@ 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
@@ -337,10 +352,10 @@ pick_one_preserving_merges () {
if test -f "$rewritten"/$p
then
new_p=$(cat "$rewritten"/$p)
-
- # If the todo reordered commits, and our parent is
marked for
- # rewriting, but hasn't been gotten to yet, assume the
user meant to
- # drop it on top of the current HEAD
+ # If the todo reordered commits, and our parent is
+ # marked for rewriting, but hasn't been gotten to yet,
+ # assume the user meant to drop it on top of the
+ # current HEAD
if test -z "$new_p"
then
new_p=$(git rev-parse HEAD)
@@ -379,7 +394,7 @@ pick_one_preserving_merges () {
then
# detach HEAD to current parent
Re: [RFC PATCH v2 1/1] rebase-interactive: Add git_rebase__interactive__preserve_merges
Wink Saville writes:
> Refactor git_rebase__interactive__preserve_merges out of
> git_rebase__interactive resulting in fewer conditionals making
> both routines are simpler.
>
> Changed run_specific_rebase in git-rebase to dispatch to the appropriate
> function after sourcing git-rebase--interactive.
> ---
I think this will become (more) reviewable if it is split into two
patches (at least). A preliminary patch that does the style changes
and line wrapping (left below) as step #1, and all the rest on top
as step #2.
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 331c8dfea..65ff75654 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -1,5 +1,7 @@
> -# This shell script fragment is sourced by git-rebase to implement
> -# its interactive mode. "git rebase --interactive" makes it easy
> +#!/bin/sh
Addition of #! implies that this might be invoked as the top-level
script; is that the case now? I did not get such an impression from
the proposed log message and it is still always dot-sourced (in
which case #! gives a wrong signal to the readers).
> +# This shell script fragment is sourced by git-rebase--interactive
> +# and git-rebase--interactive--preserve-merges in support of the
> +# interactive mode. "git rebase --interactive" makes it easy
> # to fix up commits in the middle of a series and rearrange commits.
> #
> # Copyright (c) 2006 Johannes E. Schindelin
> @@ -7,6 +9,7 @@
> # The original idea comes from Eric W. Biederman, in
> # https://public-inbox.org/git/m1odwkyuf5.fsf...@ebiederm.dsl.xmission.com/
> #
> +
> # The file containing rebase commands, comments, and empty lines.
Why?
> @@ -274,7 +290,8 @@ pick_one () {
>
> case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
> case "$force_rebase" in '') ;; ?*) ff= ;; esac
> - output git rev-parse --verify $sha1 || die "$(eval_gettext "Invalid
> commit name: \$sha1")"
> + output git rev-parse --verify $sha1 ||
> + die "$(eval_gettext "Invalid commit name: \$sha1")"
Just linewrapping.
> @@ -287,8 +304,8 @@ pick_one () {
> ${gpg_sign_opt:+$(git rev-parse --sq-quote
> "$gpg_sign_opt")} \
> "$strategy_args" $empty_args $ff "$@"
>
> - # If cherry-pick dies it leaves the to-be-picked commit unrecorded.
> Reschedule
> - # previous task so this commit is not lost.
> + # If cherry-pick dies it leaves the to-be-picked commit unrecorded.
> + # Reschedule previous task so this commit is not lost.
Ditto.
> @@ -307,17 +324,15 @@ 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
Good code simplification that turns
if A
if B
do this thing
fi
fi
into
if A & B
do this thing
fi
> @@ -337,10 +352,10 @@ pick_one_preserving_merges () {
> if test -f "$rewritten"/$p
> then
> new_p=$(cat "$rewritten"/$p)
> -
> - # If the todo reordered commits, and our parent is
> marked for
> - # rewriting, but hasn't been gotten to yet, assume the
> user meant to
> - # drop it on top of the current HEAD
> + # If the todo reordered commits, and our parent is
> + # marked for rewriting, but hasn't been gotten to yet,
> + # assume the user meant to drop it on top of the
> + # current HEAD
Just line wrapping.
> @@ -379,7 +394,7 @@ pick_one_preserving_merges () {
> then
> # detach HEAD to current parent
> output git checkout $first_parent 2> /dev/null ||
> - die "$(eval_gettext "Cannot move HEAD to
> \$first_parent")"
> +die "$(eval_gettext "Cannot move HEAD to
> \$first_parent")"
> fi
Ditto.
> @@ -553,7 +568,7 @@ do_next ()
Re: [RFC PATCH v2 1/1] rebase-interactive: Add git_rebase__interactive__preserve_merges
On Wed, Mar 21, 2018 at 12:42 PM, Junio C Hamano wrote:
> Wink Saville writes:
>
>> Refactor git_rebase__interactive__preserve_merges out of
>> git_rebase__interactive resulting in fewer conditionals making
>> both routines are simpler.
>>
>> Changed run_specific_rebase in git-rebase to dispatch to the appropriate
>> function after sourcing git-rebase--interactive.
>> ---
>
> I think this will become (more) reviewable if it is split into two
> patches (at least). A preliminary patch that does the style changes
> and line wrapping (left below) as step #1, and all the rest on top
> as step #2.
Yes, I will break this into several commits
>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 331c8dfea..65ff75654 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -1,5 +1,7 @@
>> -# This shell script fragment is sourced by git-rebase to implement
>> -# its interactive mode. "git rebase --interactive" makes it easy
>> +#!/bin/sh
>
> Addition of #! implies that this might be invoked as the top-level
> script; is that the case now? I did not get such an impression from
> the proposed log message and it is still always dot-sourced (in
> which case #! gives a wrong signal to the readers).
Will remove adding the shebang
>> +# This shell script fragment is sourced by git-rebase--interactive
>> +# and git-rebase--interactive--preserve-merges in support of the
>> +# interactive mode. "git rebase --interactive" makes it easy
>> # to fix up commits in the middle of a series and rearrange commits.
>> #
>> # Copyright (c) 2006 Johannes E. Schindelin
>> @@ -7,6 +9,7 @@
>> # The original idea comes from Eric W. Biederman, in
>> # https://public-inbox.org/git/m1odwkyuf5.fsf...@ebiederm.dsl.xmission.com/
>> #
>> +
>> # The file containing rebase commands, comments, and empty lines.
>
> Why?
Will remove the blank line.
>> @@ -274,7 +290,8 @@ pick_one () {
>>
>> case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
>> case "$force_rebase" in '') ;; ?*) ff= ;; esac
>> - output git rev-parse --verify $sha1 || die "$(eval_gettext "Invalid
>> commit name: \$sha1")"
>> + output git rev-parse --verify $sha1 ||
>> + die "$(eval_gettext "Invalid commit name: \$sha1")"
>
> Just linewrapping.
Will be leaving for now
>
>> @@ -287,8 +304,8 @@ pick_one () {
>> ${gpg_sign_opt:+$(git rev-parse --sq-quote
>> "$gpg_sign_opt")} \
>> "$strategy_args" $empty_args $ff "$@"
>>
>> - # If cherry-pick dies it leaves the to-be-picked commit unrecorded.
>> Reschedule
>> - # previous task so this commit is not lost.
>> + # If cherry-pick dies it leaves the to-be-picked commit unrecorded.
>> + # Reschedule previous task so this commit is not lost.
>
> Ditto.
Will be leaving for now
>
>> @@ -307,17 +324,15 @@ 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
>
> Good code simplification that turns
>
> if A
> if B
> do this thing
> fi
> fi
>
> into
>
> if A & B
> do this thing
> fi
Will be keeping this in a future commit
>
>> @@ -337,10 +352,10 @@ pick_one_preserving_merges () {
>> if test -f "$rewritten"/$p
>> then
>> new_p=$(cat "$rewritten"/$p)
>> -
>> - # If the todo reordered commits, and our parent is
>> marked for
>> - # rewriting, but hasn't been gotten to yet, assume the
>> user meant to
>> - # drop it on top of the current HEAD
>> + # If the todo reordered commits, and our parent is
>> + # marked for rewriting, but hasn't been gotten to yet,
>> + # assume the user meant to drop it on top of the
>> + # current HEAD
>
> Just line wrapping.
Will be leaving for now
>
>> @@ -379,7 +394,7 @@ p
Re: [RFC PATCH v2 1/1] rebase-interactive: Add git_rebase__interactive__preserve_merges
Wink Saville writes: >> Good code simplification that turns >> >> if A >> if B >> do this thing >> fi >> fi >> >> into >> >> if A & B >> do this thing >> fi > > Will be keeping this in a future commit I think this one is simple enough to be in the "prelim clean-up" change, so that you can reduce the size of the remaining stuff that really needs focused review. > But if that doesn't come to past, I believe my goal of simplication and > fixing: > "not ok 24 - exchange two commits with -p # TODO known breakage" > In t3404-rebase-interactive.sh is worth while. Oh, thanks for working on this.
