Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
- From
Brian Lyles <brianmlyles@gmail.com>
- Date
- Feb 10, 2024, 05:50 UTC
- Message-ID
- <17b26a6fdb07139f.70b1dd9aae081c6e.203dcd72f6563036@zivdesk>
- In-Reply-To
- <b5213705-4cd6-40ef-8c5f-32b214534b8b@gmail.com>
On Mon, Jan 29, 2024 at 4:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 49 quoted lines
>>>> 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