Re: [PATCH 0/1] rebase: add --[no-]edit to --continue

[Junio C Hamano <gitster@pobox.com>]

Hugo Sales <hugo@hsal.es> writes:

> When a rebase stops for conflicts and the user runs `git rebase --continue`, the
> merge backend opens $EDITOR so the commit message can be revised. That is often
> useful, but not always: sometimes the user only wants to keep the message that
> is already there.
>
> This series adds:
>
> - `git rebase --continue --no-edit` to commit without opening an editor

Meh. "GIT_SEQUENCE_EDITOR=: git rebase --continue" is your friend ;-)

> - `rebase.noEdit` to make that the default on continue
> - `git rebase --continue --edit` to override `rebase.noEdit`

The new 'rebase.noEdit' configuration is especially concerning.  It
encourages users to assume by default that their rebase sessions
will not produce notable changes worth recording in the commit logs.

A few immediate edge cases come to mind:

 - What if 'rebase -i' stops to give control back to the user for
   reasons other than a merge conflict?  If the user chose 'edit',
   their original intent was to modify both the commit message and
   the content.  With 'rebase.noEdit' enabled, would they now have
   to remember to pass '--edit' when continuing?  Does the answer
   depend on whether the 'edit' step resulted in a merge conflict?

 - What if the user chose 'reword', which is an explicit signal to
   update the commit message, but 'rebase.noEdit' is enabled?  If
   the rebase does not stop with a conflict, it might open the
   editor regardless of the configuration.  But if a conflict does
   occur and requires manual resolution, will the user still need to
   remember to pass '--edit' when continuing?

The proposed tests only cover the code path where control returns to
the user due to a conflict.  This is understandable since that
scenario was your primary motivation.  However, they do not verify
what happens when there are no conflicts but the user explicitly
intended to edit the message from the start.  You may want to expand
the test coverage to address these scenarios (and potentially
others, as this is not an exhaustive list).

Thanks.