Re: [PATCH v2 02/23] rebase -i: allow squashing empty commits without complaints

[Fabian Ruch <bafain@gmail.com>]

Hi Eric,

Eric Sunshine writes:
> On Wed, Aug 6, 2014 at 7:59 PM, Fabian Ruch <bafain@gmail.com> wrote:
>> The to-do list commands `squash` and `fixup` apply the changes
>> introduced by the named commit to the tree but instead of creating
>> a new commit on top of the current head it replaces the previous
>> commit with a new commit that records the updated tree. If the
>> result is an empty commit git-rebase stops with the error message
>>
>>    You asked to amend the most recent commit, but doing so would make
>>    it empty. You can repeat your command with --allow-empty, or you can
>>    remove the commit entirely with "git reset HEAD^".
>>
>> This message is not very helpful because neither does git-rebase
>> support an option `--allow-empty` nor does the messages say how to
>> resume the rebase. Firstly, change the error message to
>>
>>    The squash result is empty and --keep-empty was not specified.
>>
>>    You can remove the squash commit now with
>>
>>      git reset HEAD^
>>
>>    Once you are down, run
> 
> I guess you meant: s/down/done
> 
> Same issue with the actually message in the code (below).

Fixed.

>>      git rebase --continue
>>
>> If the user wishes to squash a sequence of commits into one
>> commit, f. i.
>>
>>    pick A
>>    squash Revert "A"
>>    squash A'
>>
>> , it does not matter for the end result that the first squash
>> result, or any sub-sequence in general, is going to be empty. The
>> squash message is not affected at all by which commits are created
>> and only the commit created by the last line in the sequence will
>> end up in the final history. Secondly, print the error message
>> only if the whole squash sequence produced an empty commit.
>>
>> Lastly, since an empty squash commit is not a failure to rewrite
>> the history as planned, issue the message above as a mere warning
>> and interrupt the rebase with the return value zero. The
>> interruption should be considered as a notification with the
>> chance to undo it on the spot. Specifying the `--keep-empty`
>> option tells git-rebase to keep empty squash commits in the
>> rebased history without notification.
>>
>> Add tests.
>>
>> Reported-by: Peter Krefting <peter@softwolves.pp.se>
>> Signed-off-by: Fabian Ruch <bafain@gmail.com>
>> ---
>> Hi,
>>
>> Peter Krefting is cc'd as the author of the bug report "Confusing
>> error message in rebase when commit becomes empty" discussed on the
>> mailing list in June. Phil Hord and Jeff King both participated in
>> the problem discussion which ended with two proposals by Jeff.
>>
>> Jeff King writes:
>>>   1. Always keep such empty commits. A user who is surprised by them
>>>      being empty can then revisit them. Or drop them by doing another
>>>      rebase without --keep-empty.
>>>
>>>   2. Notice ourselves that the end-result of the whole squash is an
>>>      empty commit, and stop to let the user deal with it.
>>
>> This patch chooses the second alternative. Either way seems OK. The
>> crucial consensus of the discussion was to silently throw away empty
>> interim commits.
>>
>>    Fabian
>>
>>  git-rebase--interactive.sh    | 20 +++++++++++---
>>  t/t3404-rebase-interactive.sh | 62 +++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 79 insertions(+), 3 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 3222bf6..8820eac 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -549,7 +549,7 @@ do_next () {
>>                 squash|s|fixup|f)
>>                         # This is an intermediate commit; its message will only be
>>                         # used in case of trouble.  So use the long version:
>> -                       do_with_author output git commit --allow-empty-message \
>> +                       do_with_author output git commit --allow-empty-message --allow-empty \
>>                                 --amend --no-verify -F "$squash_msg" \
>>                                 ${gpg_sign_opt:+"$gpg_sign_opt"} ||
>>                                 die_failed_squash $sha1 "$rest"
>> @@ -558,18 +558,32 @@ do_next () {
>>                         # This is the final command of this squash/fixup group
>>                         if test -f "$fixup_msg"
>>                         then
>> -                               do_with_author git commit --allow-empty-message \
>> +                               do_with_author git commit --allow-empty-message --allow-empty \
>>                                         --amend --no-verify -F "$fixup_msg" \
>>                                         ${gpg_sign_opt:+"$gpg_sign_opt"} ||
>>                                         die_failed_squash $sha1 "$rest"
>>                         else
>>                                 cp "$squash_msg" "$GIT_DIR"/SQUASH_MSG || exit
>>                                 rm -f "$GIT_DIR"/MERGE_MSG
>> -                               do_with_author git commit --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
>> +                               do_with_author git commit --allow-empty --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
>>                                         ${gpg_sign_opt:+"$gpg_sign_opt"} ||
>>                                         die_failed_squash $sha1 "$rest"
>>                         fi
>>                         rm -f "$squash_msg" "$fixup_msg"
>> +                       if test -z "$keep_empty" && is_empty_commit HEAD
>> +                       then
>> +                               echo "$sha1" >"$state_dir"/stopped-sha
>> +                               warn "The squash result is empty and --keep-empty was not specified."
>> +                               warn
>> +                               warn "You can remove the squash commit now with"
>> +                               warn
>> +                               warn "  git reset HEAD^"
>> +                               warn
>> +                               warn "Once you are down, run"
> 
> s/down/done/

Thanks for the thorough reading.

   Fabian