Hi Brian
On 28/01/2024 16:36, Brian Lyles wrote:
[+cc Junio]
On Sat, Jan 27, 2024 at 5:30 PM Brian Lyles <brianmlyles@xxxxxxxxx> 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`
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.
Best Wishes
Phillip
