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

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

>
> 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:
>
> """
> […]
> """

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

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

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

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

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

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

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

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