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

["Brian Lyles" <brianmlyles@gmail.com>]

On Mon, Jan 29, 2024 at 4:55 AM Phillip Wood <phillip.wood123@gmail.com>
wrote:

>>>> 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`
> 
> Oh dear, that's a pain. I haven't checked but suspect we already hit 
> this when running
> 
>      git cherry-pick --allow-empty
> 
> on an orphan checkout. In do_pick_commit() we treat an error reading 
> HEAD as an unborn branch so I think we could do the same here. If the 
> branch is unborn then we can use the_hash_algo->empty_tree as the tree 
> to compare to.

You're correct that `git cherry-pick --allow-empty` on an unborn branch
hits the same error today, and that using `empty_tree` seems to resolve
it. Thank you for this tip, I'll include a fix and corresponding test as
a separate commit in v2.

-- 
Thank you,
Brian Lyles