Re: New git-rebase backend: no way to drop already-empty commits

From: Elijah Newren <newren@gmail.com>
Date: Apr 7, 2020, 19:45 UTC
Message-ID: <CABPp-BF8eokQTVwgo80ffq3tn=NA=mPf7oymce5P4itDyZBiMg@mail.gmail.com>
In-Reply-To: <xmqqh7xvtby2.fsf@gitster.c.googlers.com>

On Tue, Apr 7, 2020 at 10:58 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > Yes, from the manpage:
> >
> > ...
> >
> > and
> >
> > """
> > Empty commits
> > ~~~~~~~~~~~~~
> >
> > The apply backend unfortunately drops intentionally empty commits, i.e.
> > commits that started empty, though these are rare in practice.  It
> > also drops commits that become empty and has no option for controlling
> > this behavior.
>
> This is a very good illustration that shows why "switch the default
> and retire the apply backend" deserves to be cooked for quite a long
> time.

Yep. I suspect it may be a long time before we have --whitespace=fix implemented anyway (because I'm not sure there are folks who want to dig into xdiff), but even if it was implemented soon, retiring a backend that has been the default for many, many years is the kind of thing that should wait a while.

Show 5 quoted lines

>  The 'apply' dropping empty commits may have looked like an
> 'unfortunate' thing to whoever wrote the above paragraph in the
> documentation, but it clearly shows that person (me included) did
> not think of the ramifications deeply enough that there may be valid
> workflows that _depend_ on the behaviour.

Guilty as charged. I did try to highlight the empty handling for reviewers when I posted the backend switch series (see e.g. https://lore.kernel.org/git/pull.679.git.git.1576861788.gitgitgadget@gmail.com/ and https://lore.kernel.org/git/pull.679.v3.git.git.1577217299.gitgitgadget@gmail.com/ and some of the emails between Phillip and I directly about that topic while discussing the series), but it's understandable that this could be overlooked by not just me but reviewers as well -- the workaround of running an interactive rebase and remove the lines they don't want probably seemed really simple and clear.

Show 10 quoted lines

> As we will be dropping 'apply' that could be used as an escape
> hatch, before we do so, we should teach the other backends an
> alternate escape hatch to help those who have been depending on the
> behaviour of 'apply' that discards the empty ones, whether they
> become empty, or they are empty from the beginning.  I think the
> "has contents originally but becomes empty" side is already taken
> care of, so we'd need to make sure it is easy to optionally discard
> the ones that are originally empty.
>
> Thanks.

I will take a look into it, using (or at least starting with) the suggestion Sami provided.