| Message ID | 20240210074859.552497-5-brianmlyles@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Hi Brian On 10/02/2024 07:43, Brian Lyles wrote: > When using git-cherry-pick(1) with `--allow-empty` while on an unborn > branch, an error is thrown. This is inconsistent with the same > cherry-pick when `--allow-empty` is not specified. > > Treat a failure reading HEAD as an unborn branch in > `is_index_unchanged`. This is consistent with other sequencer logic such > as `do_pick_commit`. When on an unborn branch, use the `empty_tree` as > the tree to compare against. > > Signed-off-by: Brian Lyles <brianmlyles@gmail.com> > Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > > This is another new commit that was not present in v1. > > See this comment[1] from Phillip for context. > > [1]: https://lore.kernel.org/git/b5213705-4cd6-40ef-8c5f-32b214534b8b@gmail.com/ Thanks for fixing this and adding a test, I've left a few small comments below. > static int is_index_unchanged(struct repository *r) > { > - struct object_id head_oid, *cache_tree_oid; > + struct object_id head_oid, *cache_tree_oid, head_tree_oid; I think we can make `head_tree_oid` a pointer like cache_tree_oid and avoid de-referencing `the_hash_algo->empty_tree` and the return value of `get_commit_tree_oid()`. I think the only reason to copy it would be if the underlying object had a shorter lifetime than `head_tree_oid` but I don't think that's the case. > +test_expect_success 'cherry-pick on unborn branch with --allow-empty' ' > + git checkout main && I'm a bit confused by this - are we already on the branch "unborn" and so need to move away from it to delete it? > + git branch -D unborn && > + git checkout --orphan unborn && > + git rm --cached -r . && > + rm -rf * && "git switch --orphan" leaves us with an empty index and working copy without having to remove the files ourselves. > + git cherry-pick initial --allow-empty && > + git diff --quiet initial && I'd drop "--quiet" here as it makes debugging easier if we can see the diff if the test fails. > + test_cmp_rev ! initial HEAD > +' Best Wishes Phillip
On Thu, Feb 22, 2024 at 10:34 AM <phillip.wood123@gmail.com> wrote: >> static int is_index_unchanged(struct repository *r) >> { >> - struct object_id head_oid, *cache_tree_oid; >> + struct object_id head_oid, *cache_tree_oid, head_tree_oid; > > I think we can make `head_tree_oid` a pointer like cache_tree_oid and > avoid de-referencing `the_hash_algo->empty_tree` and the return value of > `get_commit_tree_oid()`. I think the only reason to copy it would be if > the underlying object had a shorter lifetime than `head_tree_oid` but I > don't think that's the case. Makes sense -- I'll update this in v3. >> +test_expect_success 'cherry-pick on unborn branch with --allow-empty' ' >> + git checkout main && > > I'm a bit confused by this - are we already on the branch "unborn" and > so need to move away from it to delete it? Yes, the previous test leaves us on that branch. In v3, I will update this to instead just use `git checkout --detach`, as that does seem a little less confusing than switching to some other branch that is only relevant because it's not `unborn`. If there is a cleaner way to do this, I'd be happy to switch to it. >> + git branch -D unborn && >> + git checkout --orphan unborn && >> + git rm --cached -r . && >> + rm -rf * && > > "git switch --orphan" leaves us with an empty index and working copy > without having to remove the files ourselves. Thanks for pointing this out. Using git-switch(1) here instead of git-checkout(1) allows us to drop the `rm -rf *` call form both the existing 'cherry-pick on unborn branch' test as well as my new test. It appears that the `git rm --cached -r .` call is still necessary in the existing test. >> + git cherry-pick initial --allow-empty && >> + git diff --quiet initial && > > I'd drop "--quiet" here as it makes debugging easier if we can see the > diff if the test fails. This makes sense. In v3, I will update this new test as well as the existing test to not use `--quiet`. Combining the above suggestions, here's the version of the existing and new tests that I intend to use in v3. Let me know if this isn't what you had in mind! test_expect_success 'cherry-pick on unborn branch' ' git switch --orphan unborn && git rm --cached -r . && git cherry-pick initial && git diff initial && test_cmp_rev ! initial HEAD ' test_expect_success 'cherry-pick on unborn branch with --allow-empty' ' git checkout --detach && git branch -D unborn && git switch --orphan unborn && git cherry-pick initial --allow-empty && git diff initial && test_cmp_rev ! initial HEAD '
Hi Brian On 23/02/2024 05:28, Brian Lyles wrote: > On Thu, Feb 22, 2024 at 10:34 AM <phillip.wood123@gmail.com> wrote: >>> +test_expect_success 'cherry-pick on unborn branch with --allow-empty' ' >>> + git checkout main && >> >> I'm a bit confused by this - are we already on the branch "unborn" and >> so need to move away from it to delete it? > > Yes, the previous test leaves us on that branch. In v3, I will update > this to instead just use `git checkout --detach`, as that does seem a > little less confusing than switching to some other branch that is only > relevant because it's not `unborn`. If there is a cleaner way to do > this, I'd be happy to switch to it. I think "git checkout --detach" is probably the best we can do. It would be nice to be able to do "git switch -C --orphan unborn" but "-C" does not work with "--orphan" >>> + git branch -D unborn && >>> + git checkout --orphan unborn && >>> + git rm --cached -r . && >>> + rm -rf * && >> >> "git switch --orphan" leaves us with an empty index and working copy >> without having to remove the files ourselves. > > Thanks for pointing this out. Using git-switch(1) here instead of > git-checkout(1) allows us to drop the `rm -rf *` call form both the > existing 'cherry-pick on unborn branch' test as well as my new test. It > appears that the `git rm --cached -r .` call is still necessary in the > existing test. It looks like the previous test 'revert forbidden on dirty working tree' fails to clean up properly and so "git switch" is carrying the uncommitted changes across to the new orphan branch. I think that "git switch --discard-changes --orphan unborn" ought to clean the worktree and index but it doesn't seem to work. >>> + git cherry-pick initial --allow-empty && >>> + git diff --quiet initial && >> >> I'd drop "--quiet" here as it makes debugging easier if we can see the >> diff if the test fails. > > This makes sense. In v3, I will update this new test as well as the > existing test to not use `--quiet`. > > Combining the above suggestions, here's the version of the existing and > new tests that I intend to use in v3. Let me know if this isn't what you > had in mind! > > test_expect_success 'cherry-pick on unborn branch' ' > git switch --orphan unborn && > git rm --cached -r . && > git cherry-pick initial && > git diff initial && > test_cmp_rev ! initial HEAD > ' > > test_expect_success 'cherry-pick on unborn branch with --allow-empty' ' > git checkout --detach && > git branch -D unborn && > git switch --orphan unborn && > git cherry-pick initial --allow-empty && > git diff initial && > test_cmp_rev ! initial HEAD > ' These look good Thanks Phillip
diff --git a/sequencer.c b/sequencer.c index 3cc88d8a80..b1b19512de 100644 --- a/sequencer.c +++ b/sequencer.c @@ -769,30 +769,36 @@ static struct object_id *get_cache_tree_oid(struct index_state *istate) static int is_index_unchanged(struct repository *r) { - struct object_id head_oid, *cache_tree_oid; + struct object_id head_oid, *cache_tree_oid, head_tree_oid; struct commit *head_commit; struct index_state *istate = r->index; - if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) - return error(_("could not resolve HEAD commit")); + if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) { + /* + * Treat an error reading HEAD as an unborn branch. + */ + head_tree_oid = *the_hash_algo->empty_tree; + } else { + head_commit = lookup_commit(r, &head_oid); - head_commit = lookup_commit(r, &head_oid); + /* + * If head_commit is NULL, check_commit, called from + * lookup_commit, would have indicated that head_commit is not + * a commit object already. repo_parse_commit() will return failure + * without further complaints in such a case. Otherwise, if + * the commit is invalid, repo_parse_commit() will complain. So + * there is nothing for us to say here. Just return failure. + */ + if (repo_parse_commit(r, head_commit)) + return -1; - /* - * If head_commit is NULL, check_commit, called from - * lookup_commit, would have indicated that head_commit is not - * a commit object already. repo_parse_commit() will return failure - * without further complaints in such a case. Otherwise, if - * the commit is invalid, repo_parse_commit() will complain. So - * there is nothing for us to say here. Just return failure. - */ - if (repo_parse_commit(r, head_commit)) - return -1; + head_tree_oid = *get_commit_tree_oid(head_commit); + } if (!(cache_tree_oid = get_cache_tree_oid(istate))) return -1; - return oideq(cache_tree_oid, get_commit_tree_oid(head_commit)); + return oideq(cache_tree_oid, &head_tree_oid); } static int write_author_script(const char *message) diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index aeab689a98..390e0ed186 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -112,6 +112,17 @@ test_expect_success 'cherry-pick on unborn branch' ' test_cmp_rev ! initial HEAD ' +test_expect_success 'cherry-pick on unborn branch with --allow-empty' ' + git checkout main && + git branch -D unborn && + git checkout --orphan unborn && + git rm --cached -r . && + rm -rf * && + git cherry-pick initial --allow-empty && + git diff --quiet initial && + test_cmp_rev ! initial HEAD +' + test_expect_success 'cherry-pick "-" to pick from previous branch' ' git checkout unborn && test_commit to-pick actual content &&
When using git-cherry-pick(1) with `--allow-empty` while on an unborn branch, an error is thrown. This is inconsistent with the same cherry-pick when `--allow-empty` is not specified. Treat a failure reading HEAD as an unborn branch in `is_index_unchanged`. This is consistent with other sequencer logic such as `do_pick_commit`. When on an unborn branch, use the `empty_tree` as the tree to compare against. Signed-off-by: Brian Lyles <brianmlyles@gmail.com> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- This is another new commit that was not present in v1. See this comment[1] from Phillip for context. [1]: https://lore.kernel.org/git/b5213705-4cd6-40ef-8c5f-32b214534b8b@gmail.com/ sequencer.c | 36 ++++++++++++++++++++--------------- t/t3501-revert-cherry-pick.sh | 11 +++++++++++ 2 files changed, 32 insertions(+), 15 deletions(-)