| Message ID | 20240310184602.539656-5-brianmlyles@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Brian Lyles <brianmlyles@gmail.com> writes: > 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. When cherry-picking on top of an unborn branch without the option, how does the code figure out that we are on an unborn branch? We must be doing the detection, as we'd need to at least know the fact that we are on an unborn in order to make the resulting commit a root commit and also in order to apply the cherry-picked changes against an empty tree. > 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. It is not a good code hygiene to assume that a failure to read HEAD always means we are on an unborn branch, even if that is the most likely cause of the failure. We may instead want to positively determine that we are on an unborn state, by seeing if the HEAD is a symbolic ref that points at a ref in refs/heads/* hierarchy, and that ref does not exist. But if the existing sequencer code is littered with such loose logic everywhere, perhaps imitating the looseness for now with a NEEDSWORK comment to later clean them all up would be the least we could do right now. If we want to be more ambitious, a few clean-up patches that refactors existing code paths that detect that we are on an unborn branch into a call to a single helper function, and then tightens its implementation, before doing this step would be even better (but I offhand do not know how bad the current code is, so I would understand it if it may turn out to be a lot more work than you would want to invest in).
Junio C Hamano <gitster@pobox.com> writes: > It is not a good code hygiene to assume that a failure to read HEAD > always means we are on an unborn branch, even if that is the most > likely cause of the failure. We may instead want to positively > determine that we are on an unborn state, by seeing if the HEAD is a > symbolic ref that points at a ref in refs/heads/* hierarchy, and > that ref does not exist. I suspect that you are almost there. + if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) { + /* + * Treat an error reading HEAD as an unborn branch. + */ After we see this error, if we make a call to resolve_ref_unsafe() with RESOLVE_REF_NO_RECURSE, the call should return the branch that we are on but is not yet born, and &oid will get the null_oid. I am not sure if there is a way to combine the two calls into one, but because the failure case (i.e. doing anything on an unborn branch) is a rare case that happens only once before actually giving birth to a new branch, it probably is not worth spending extra brain cycles on it and just use a simple and stupid "when resolving fails, see if we are in a rare salvageable case with extra code" approach.
Hi Junio, On Mon, Mar 11, 2024 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> It is not a good code hygiene to assume that a failure to read HEAD >> always means we are on an unborn branch, even if that is the most >> likely cause of the failure. We may instead want to positively >> determine that we are on an unborn state, by seeing if the HEAD is a >> symbolic ref that points at a ref in refs/heads/* hierarchy, and >> that ref does not exist. > > I suspect that you are almost there. > > + if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) { > + /* > + * Treat an error reading HEAD as an unborn branch. > + */ > > After we see this error, if we make a call to resolve_ref_unsafe() > with RESOLVE_REF_NO_RECURSE, the call should return the branch that > we are on but is not yet born, and &oid will get the null_oid. I am > not sure if there is a way to combine the two calls into one, but > because the failure case (i.e. doing anything on an unborn branch) > is a rare case that happens only once before actually giving birth > to a new branch, it probably is not worth spending extra brain > cycles on it and just use a simple and stupid "when resolving fails, > see if we are in a rare salvageable case with extra code" approach. If I'm understanding you correctly, it sounds like you're hinting at something like this: if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) { /* * Check to see if this is an unborn branch */ if (resolve_ref_unsafe("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) && is_null_oid(&head_oid)) { head_tree_oid = the_hash_algo->empty_tree; } else { return error(_("could not resolve HEAD commit")); } } else { ... } This does in fact seem to have the same effect as the original patch in this case. If this is in line with what you are looking for, I can include this in v4. The commit message would be adjusted slightly to be: sequencer: handle unborn branch with `--allow-empty` 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. Detect unborn branches in `is_index_unchanged`. When on an unborn branch, use the `empty_tree` as the tree to compare against. ... Do these changes look good?
"Brian Lyles" <brianmlyles@gmail.com> writes: > If I'm understanding you correctly, it sounds like you're hinting at > something like this: You may also want to add _READING (I do not know offhand). Also you'd want to make sure resolve_ref_unsafe() returned a plausible looking refname (i.e. passes starts_with("refs/heads/"). But other than that, yeah, doing these as "extra checks" only after we see the primary resolve_ref("HEAD") fails was what I had in mind. Thanks.
Hi Brian On 10/03/2024 18:42, Brian Lyles wrote: > - Avoid using `--quiet` in the `git diff` call to make debugging easier > in the event of a failure Sorry, I forgot when I was reviewing v2 that we need to replace --quiet with --exit-code otherwise the diff will never fail. Apart from that I don't have anything to add to Junio's comments on this patch. Best Wishes Phillip
On Tue, Mar 12, 2024 at 5:25 PM Junio C Hamano <gitster@pobox.com> wrote: > "Brian Lyles" <brianmlyles@gmail.com> writes: > >> If I'm understanding you correctly, it sounds like you're hinting at >> something like this: > > You may also want to add _READING (I do not know offhand). Also > you'd want to make sure resolve_ref_unsafe() returned a plausible > looking refname (i.e. passes starts_with("refs/heads/"). > > But other than that, yeah, doing these as "extra checks" only after > we see the primary resolve_ref("HEAD") fails was what I had in mind. > > Thanks. Makes sense to me. From reading the documentation for RESOLVE_REF_READING, I think we do want that as well, and the starts_with("refs/heads/") check works as expected too. I'll incorporate those into v4.
Hi Phillip On Wed, Mar 13, 2024 at 10:10 AM <phillip.wood123@gmail.com> wrote: > Hi Brian > > On 10/03/2024 18:42, Brian Lyles wrote: >> - Avoid using `--quiet` in the `git diff` call to make debugging easier >> in the event of a failure > > Sorry, I forgot when I was reviewing v2 that we need to replace --quiet > with --exit-code otherwise the diff will never fail. Apart from that I > don't have anything to add to Junio's comments on this patch. Ah, of course -- thank you for catching that. I'll include that in v4.
diff --git a/sequencer.c b/sequencer.c index f49a871ac0..a62ce244c1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -770,29 +770,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; + const struct object_id *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..8a1d154ca6 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -104,11 +104,19 @@ test_expect_success 'revert forbidden on dirty working tree' ' ' test_expect_success 'cherry-pick on unborn branch' ' - git checkout --orphan unborn && + git switch --orphan unborn && git rm --cached -r . && - rm -rf * && git cherry-pick initial && - git diff --quiet 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 '
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. Add a new test to cover this scenario. While modelled off of the existing 'cherry-pick on unborn branch' test, some improvements can be made: - Use `git switch --orphan unborn` instead of `git checkout --orphan unborn` to avoid the need for a separate `rm -rf *` call - Avoid using `--quiet` in the `git diff` call to make debugging easier in the event of a failure Make these improvements to the existing test as well as the new test. Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Brian Lyles <brianmlyles@gmail.com> --- Differences from v2: - Minor code and test cleanup per [1] and the other replies in that thread [1]: https://lore.kernel.org/git/62247a1c-0249-4ce1-8626-fe97b89c23dc@gmail.com/ sequencer.c | 35 +++++++++++++++++++++-------------- t/t3501-revert-cherry-pick.sh | 14 +++++++++++--- 2 files changed, 32 insertions(+), 17 deletions(-)