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