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

> [+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`