Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options

[Brian Lyles <brianmlyles@gmail.com>]

[+cc Junio]

On Sat, Jan 27, 2024 at 5:30 PM Brian Lyles <brianmlyles@gmail.com> wrote:

> > > For some amount of backwards compatibility with the existing code and
> > > tests, I have opted to preserve the behavior of returning 0 when:
> > >
> > > - `allow_empty` is specified, and
> > > - either `is_index_unchanged` or `is_original_commit_empty` indicates an
> > >    error
> >
> > I'm not sure that is a good idea as it is hiding an error that we didn't
> > hit before because we returned early.
>
> I think you're right -- Previously the error could not have been hit,
> but now it can. An error is still an error, and we should handle it
> regardless of how `allow_empty` was set. I'll address this in v2 by
> simply returning the error.

As I dig into this more, I'm noticing that this may have unintended side
effects that I'm unsure of. After making this change, I noticed a couple
of failures in the cherry-pick test suite. The others may be a knock-on
of this initial failure:

    expecting success of 3501.8 'cherry-pick on unborn branch':
            git checkout --orphan unborn &&
            git rm --cached -r . &&
            rm -rf * &&
            git cherry-pick initial &&
            git diff --quiet initial &&
            test_cmp_rev ! initial HEAD

    A       extra_file
    Switched to a new branch 'unborn'
    rm 'extra_file'
    rm 'spoo'
    error: could not resolve HEAD commit
    fatal: cherry-pick failed
    not ok 8 - cherry-pick on unborn branch
    #
    #               git checkout --orphan unborn &&
    #               git rm --cached -r . &&
    #               rm -rf * &&
    #               git cherry-pick initial &&
    #               git diff --quiet initial &&
    #               test_cmp_rev ! initial HEAD
    #

It looks like this is caused specifically by not hiding the error from
`index_unchanged`

    index_unchanged = is_index_unchanged(r);
    if (index_unchanged < 0) {
        return index_unchanged;
    }

At this point, my inexperience with the git codebase and these more edge
case scenarios starts to show. I'm unsure how to best approach this. It
seems that exposing these errors when `allow_empty` is not set causes
the entire cherry-pick to fail in situations where it would not
previously. Here is what that same test looks like prior to any of my
changes from this series:

    expecting success of 3501.8 'cherry-pick on unborn branch':
            git checkout --orphan unborn &&
            git rm --cached -r . &&
            rm -rf * &&
            git cherry-pick initial &&
            git diff --quiet initial &&
            test_cmp_rev ! initial HEAD

    A       extra_file
    Switched to a new branch 'unborn'
    rm 'extra_file'
    rm 'spoo'
    [unborn 38e6d75] initial
     Author: A U Thor <author@example.com>
     Date: Thu Apr 7 15:13:13 2005 -0700
     1 file changed, 15 insertions(+)
     create mode 100644 oops
    ok 8 - cherry-pick on unborn branch

Conceptually I definitely agree that it seems odd to hide these errors
just because `allow_empty` was not set, but I fear this to be a breaking
change for which I don't have a good understanding of the impact.

Any guidance here would be appreciated.

Thank you,
Brian Lyles