Re: [PATCH] git-rebase.sh: handle keep-empty like all other options

From: Elijah Newren <newren@gmail.com>
Date: Jun 11, 2018, 16:08 UTC
Message-ID: <CABPp-BEuZDHDAxixrUUXyzbpBXoVfWhfdJR1g9eFN9MSAMjh_Q@mail.gmail.com>
In-Reply-To: <7e3d71e6-ae97-f47b-382c-085f6a281792@talktalk.net>

Hi Phillip,

On Mon, Jun 11, 2018 at 8:16 AM, Phillip Wood <phillip.wood@talktalk.net> wrote:

> On 11/06/18 15:42, Elijah Newren wrote:

Show 16 quoted lines

>> However, we have a bigger problem with empty commits, in that there
>> are really three modes rather than two:
>>    * Automatically drop empty commits (default for am-based rebase)
>>    * Automatically keep empty commits (as done with --keep-empty)
>>    * Halt the rebase and tell the user how to specify if they want to
>> keep it (default for interactive rebases)
>>
>> Currently, only the first option is available to am-based rebases, and
>> only the second two options are available to interactive-based
>> rebases.
>
>
> I'm not sure that's the case, my understanding is that for an interactive
> rebase unless you give '--keep-empty' then any empty commits will be
> commented out in the todo list and therefore dropped unless they're
> uncommented when editing the list.

Ah, yes, you are right; I was implicitly thinking about cases where the commit became empty rather than when the commit started empty...and mis-read --keep-empty (which as I learned now is only for started-empty cases).

> The third case happens when a commit
> becomes empty when it's rebased (i.e. the original commit is not empty), I'm
> not sure what the am backend does for this.

The am backend does not halt the rebase; it simply drops the commit and says "No changes -- Patch already applied."

It has always seemed jarring and confusing to me that one rebase mode stops and asks the user what to do and the other assumes. I think both should have the same default (and have options to pick the alternate behavior).

I'm less certain what the default should be, though.

> cherry-pick has
> '--keep-redundant-commits' for this case but that has never been added to
> rebase.

Thanks for the pointer.