Re: [RFC PATCH v2 1/1] rebase-interactive: Add git_rebase__interactive__preserve_merges

> Wink Saville <wink@saville.com> 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 () {
>>       pick|p)
>>               comment_for_reflog pick
>>
>> -             mark_action_done
>> +             mark_action_done $sha1 "$rest"
>
> This demands more attention from the readers than all the changes we
> have seen so far which were just line wrapping and whitespace
> changes.  That is why it is better to leave these changes to a
> separate patch after preliminary clean-up.  It allows reviewers'
> eyes coast over the clean-up step, and then lets them focus on the
> more "meaningful" changes  like this one.
>
>> @@ -637,7 +652,7 @@ you are able to reword the commit.")"
>>               read -r command rest < "$todo"
>>               mark_action_done
>>               eval_gettextln "Executing: \$rest"
>> -             "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
>> +             "${SHELL:-/bin/sh}" -c "$rest" # Actual execution
> Why?  This change is not justified in the proposed log message.