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

[Phillip Wood <phillip.wood@talktalk.net>]

On 11/06/18 17:08, Elijah Newren wrote:
> 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:
> 
>>> 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.

I'm not really sure either, on the one hand most of the time it is
convenient for rebase just to skip over it, on the other if you have
some important information in the commit message or a note then maybe
you want rebase to stop so you can selvage that information.

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

On path forwards is to always stop, and implement
--keep-redundant-commits for rebase. That would be very easy for
interactive rebases as it shares the same code as cherry-pick. I've just
had a quick look at builtin/am.c and I think it would be fairly
straightforward to add some code to check if it's rebasing and stop if
the patch is already applied.

Best Wishes

Phillip

> Thanks for the pointer.
>