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

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

[Sami Boukortt]

Hi,

Using git 2.26.0, I just tried using `git rebase` to strip empty
commits from a branch, but it leaves them as-is, even with
`--empty=drop`. With the “apply” backend, it removes them properly. Am
I holding it wrong?

`git rebase -i` also doesn’t pre-comment them like it used to.

Thanks.

Best,
Sami

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

[Elijah Newren]

On Tue, Apr 7, 2020 at 9:33 AM Sami Boukortt <sami@boukortt.com> wrote:
>
> Hi,
>
> Using git 2.26.0, I just tried using `git rebase` to strip empty
> commits from a branch, but it leaves them as-is, even with
> `--empty=drop`. With the “apply” backend, it removes them properly. Am
> I holding it wrong?
>
> `git rebase -i` also doesn’t pre-comment them like it used to.

Yes, from the manpage:

"""
--empty={drop,keep,ask}::
        How to handle commits that are not empty to start and are not
        clean cherry-picks of any upstream commit, but which become
        empty after rebasing (because they contain a subset of already
        upstream changes).  With drop (the default), commits that
        become empty are dropped....
"""

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.

The merge backend keeps intentionally empty commits.  Similar to the
apply backend, by default the merge backend drops commits that become
empty unless -i/--interactive is specified (in which case it stops and
asks the user what to do).  The merge backend also has an
--empty={drop,keep,ask} option for changing the behavior of handling
commits that become empty.
"""

To remove previously intentional commits, whether empty or not, use -i
and remove the lines corresponding to the commits you don't want.

Hope that helps,
Elijah

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

[Sami Boukortt]

Thank you for the very prompt response.

Le mar. 7 avr. 2020 à 18:54, Elijah Newren <newren@gmail.com> a écrit :
>
> On Tue, Apr 7, 2020 at 9:33 AM Sami Boukortt <sami@boukortt.com> wrote:
> >
> > Hi,
> >
> > Using git 2.26.0, I just tried using `git rebase` to strip empty
> > commits from a branch, but it leaves them as-is, even with
> > `--empty=drop`. With the “apply” backend, it removes them properly. Am
> > I holding it wrong?
> >
> > `git rebase -i` also doesn’t pre-comment them like it used to.
>
> Yes, from the manpage:
>
> """
> […]
> """

D’oh, not sure how I missed this. :) Thanks.

> To remove previously intentional commits, whether empty or not, use -i
> and remove the lines corresponding to the commits you don't want.

Sadly, that is somewhat inconvenient, as those commits are not
actually “intentional” from my viewpoint (though I understand that git
has no way of knowing this), but rather created by another tool
(git-imerge), which means that I have to check each commit
individually and risk mistakes. The old `rebase -i` behavior, where
such commits were automatically commented out, would be an acceptable
compromise, or even a comment added at the end of the commit line (so
that they are still kept if the editor is closed without changing the
rebase list). If there are plans to eventually remove the “apply”
backend, could that workaround be considered?

Alternatively, I could also use `git filter-branch` (with
`--prune-empty`), but apparently, its use is heavily discouraged.

Thanks again,
Sami

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

[Junio C Hamano]

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

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.

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

[Elijah Newren]

On Tue, Apr 7, 2020 at 10:28 AM Sami Boukortt <sami@boukortt.com> wrote:
>
> Thank you for the very prompt response.
>
> Le mar. 7 avr. 2020 à 18:54, Elijah Newren <newren@gmail.com> a écrit :
> >
> > On Tue, Apr 7, 2020 at 9:33 AM Sami Boukortt <sami@boukortt.com> wrote:
> > >
> > > Hi,
> > >
> > > Using git 2.26.0, I just tried using `git rebase` to strip empty
> > > commits from a branch, but it leaves them as-is, even with
> > > `--empty=drop`. With the “apply” backend, it removes them properly. Am
> > > I holding it wrong?
> > >
> > > `git rebase -i` also doesn’t pre-comment them like it used to.
> >
> > Yes, from the manpage:
> >
> > """
> > […]
> > """
>
> D’oh, not sure how I missed this. :) Thanks.
>
> > To remove previously intentional commits, whether empty or not, use -i
> > and remove the lines corresponding to the commits you don't want.
>
> Sadly, that is somewhat inconvenient, as those commits are not
> actually “intentional” from my viewpoint (though I understand that git
> has no way of knowing this), but rather created by another tool
> (git-imerge), which means that I have to check each commit

git-imerge creates non-merge commits?  Is this in the case when it is
acting like rebase?  If so, is this possibly a bug in git-imerge (in
that it doesn't drop commits which become empty)?

> individually and risk mistakes. The old `rebase -i` behavior, where
> such commits were automatically commented out, would be an acceptable
> compromise, or even a comment added at the end of the commit line (so
> that they are still kept if the editor is closed without changing the
> rebase list). If there are plans to eventually remove the “apply”
> backend, could that workaround be considered?

Automatically commenting them out is bad; that causes frustration for
people having to uncomment all the commits they intended to add.

But we could add some kind of option.

> Alternatively, I could also use `git filter-branch` (with
> `--prune-empty`), but apparently, its use is heavily discouraged.

You could use
   git filter-repo --prune-empty always

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

[Sami Boukortt]

Le mar. 7 avr. 2020 à 20:03, Elijah Newren <newren@gmail.com> a écrit :
>
> On Tue, Apr 7, 2020 at 10:28 AM Sami Boukortt <sami@boukortt.com> wrote:
> >
> > […]
> >
> > Sadly, that is somewhat inconvenient, as those commits are not
> > actually “intentional” from my viewpoint (though I understand that git
> > has no way of knowing this), but rather created by another tool
> > (git-imerge), which means that I have to check each commit
>
> git-imerge creates non-merge commits?  Is this in the case when it is
> acting like rebase?  If so, is this possibly a bug in git-imerge (in
> that it doesn't drop commits which become empty)?

It is indeed with `git imerge rebase`. I don’t know enough about
git-imerge’s internals to know how easy that would be to fix, but it
does seem as though that would be the ideal approach.

> > individually and risk mistakes. The old `rebase -i` behavior, where
> > such commits were automatically commented out, would be an acceptable
> > compromise, or even a comment added at the end of the commit line (so
> > that they are still kept if the editor is closed without changing the
> > rebase list). If there are plans to eventually remove the “apply”
> > backend, could that workaround be considered?
>
> Automatically commenting them out is bad; that causes frustration for
> people having to uncomment all the commits they intended to add.
>
> But we could add some kind of option.

Instead of automatically commenting them out, how about automatically
annotating them while leaving them in the rebase list, like so:

    pick 8441f42 Commit A
    pick e3fcaf8 Commit B  # empty
    pick af34c53 Commit C

> > Alternatively, I could also use `git filter-branch` (with
> > `--prune-empty`), but apparently, its use is heavily discouraged.
>
> You could use
>    git filter-repo --prune-empty always

That does seem like it would work, but wouldn’t it process the entire
repository (as opposed to filter-branch which can take a list of
revisions)?

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

[Elijah Newren]

On Tue, Apr 7, 2020 at 11:16 AM Sami Boukortt <sami@boukortt.com> wrote:
>
> Le mar. 7 avr. 2020 à 20:03, Elijah Newren <newren@gmail.com> a écrit :
> >
> > On Tue, Apr 7, 2020 at 10:28 AM Sami Boukortt <sami@boukortt.com> wrote:
> > >
> > > […]
> > >
> > > Sadly, that is somewhat inconvenient, as those commits are not
> > > actually “intentional” from my viewpoint (though I understand that git
> > > has no way of knowing this), but rather created by another tool
> > > (git-imerge), which means that I have to check each commit
> >
> > git-imerge creates non-merge commits?  Is this in the case when it is
> > acting like rebase?  If so, is this possibly a bug in git-imerge (in
> > that it doesn't drop commits which become empty)?
>
> It is indeed with `git imerge rebase`. I don’t know enough about
> git-imerge’s internals to know how easy that would be to fix, but it
> does seem as though that would be the ideal approach.

I don't either; maybe Michael Haggerty (cc'ed) can chime in on this
side of things.

> > > individually and risk mistakes. The old `rebase -i` behavior, where
> > > such commits were automatically commented out, would be an acceptable
> > > compromise, or even a comment added at the end of the commit line (so
> > > that they are still kept if the editor is closed without changing the
> > > rebase list). If there are plans to eventually remove the “apply”
> > > backend, could that workaround be considered?
> >
> > Automatically commenting them out is bad; that causes frustration for
> > people having to uncomment all the commits they intended to add.
> >
> > But we could add some kind of option.
>
> Instead of automatically commenting them out, how about automatically
> annotating them while leaving them in the rebase list, like so:
>
>     pick 8441f42 Commit A
>     pick e3fcaf8 Commit B  # empty
>     pick af34c53 Commit C

That seems reasonable.  Of course, that would make it specific to -i;
I'm curious if that's good enough or if there are other cases out
there that need more.  We could at least start with this, though.

> > > Alternatively, I could also use `git filter-branch` (with
> > > `--prune-empty`), but apparently, its use is heavily discouraged.
> >
> > You could use
> >    git filter-repo --prune-empty always
>
> That does seem like it would work, but wouldn’t it process the entire
> repository (as opposed to filter-branch which can take a list of
> revisions)?

By default, yes it processes the entire repository.  You can pass
revisions to filter-repo with the --refs option.

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

[Bryan Turner]

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

I wonder if the existing --empty could be extended, such that "drop"
was treated as a (deprecated?) synonym for "drop-new" (a new entry in
the list), with a new "drop-all". That way end users could pass
--empty=drop-all to get the old "apply" behavior with "merge".
Something like "--empty={drop-all,drop-new[,drop],keep,ask}::". A name
like "drop-all" seems more obvious/intuitive than simply "all" or
"always".

Just a thought from someone else who was also bitten by this behavior change.

>
> Thanks.

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

[Elijah Newren]

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

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

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

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

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

Thanks.