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

On 11/06/18 17:08, Elijah Newren wrote:

Hi Phillip,

On Mon, Jun 11, 2018 at 8:16 AM, Phillip Wood [off-list ref] wrote:

quoted

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

quoted

quoted

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).

quoted

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.

quoted

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.