On Mon, Jan 29, 2024 at 4:55 AM Phillip Wood <phillip.wood123@xxxxxxxxx> 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
