| Message ID | 20240320233724.214369-5-brianmlyles@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Brian Lyles <brianmlyles@gmail.com> writes: > + if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) { > + /* > + * Check to see if this is an unborn branch > + */ > + head_name = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, &head_oid, NULL); > + if (!head_name || !starts_with(head_name, "refs/heads/") || !is_null_oid(&head_oid)) > + return error(_("could not resolve HEAD commit")); > + 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; Not that I am qualified to do a review of your changes but I am in a situation where I am trying to understand Git code in general and (perhaps normal for this situation) wondering about varying styles of commenting code -- could be that I am just too new to the code base and do not yet understand the obvious things that don't need comments. In the above example, there is a short but outstanding comment that announces a check (and if I understood correctly by [1] it is a kind of trick that could deserve some more information) and it does _not_ comment on the result. Of course, I have an idea where the correct place for a comment /* This is an unborn branch -- handle it as if... */ could be, but I'm not sure. So, my intention is by no means to trigger another spin of this series -- it is just a view from someone trying to understand not just this code ;-) Dirk [1] https://lore.kernel.org/git/xmqqh6hcu2tg.fsf@gitster.g/
Dirk Gouders <dirk@gouders.net> writes: > Brian Lyles <brianmlyles@gmail.com> writes: > >> + if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) { >> + /* >> + * Check to see if this is an unborn branch >> + */ > In the above example, there is a short but outstanding comment that > announces a check (and if I understood correctly by [1] it is a kind of > trick that could deserve some more information) and it does _not_ > comment on the result. Of course, I have an idea where the correct > place for a comment /* This is an unborn branch -- handle it as if... */ > could be, but I'm not sure. You mean "Check to see if this is an unborn branch, and if so, use an empty tree to compare against, instead of the tree of the HEAD that does not yet exist"? I think that is possible, but the use of the_hash_algo->empty_tree indicates that clearly enough. But we need to stop somewhere and what we see above may be a reasonable place to do so. If anything, we may want to say why we want to continue as if we had an empty tree (as opposed to fail and return with an error()), or the tree to compare with is computed here for what purpose. But the name of the function may tell what this whole computation and comparison is for, so it probably is not needed, either.
Junio C Hamano <gitster@pobox.com> writes: > Dirk Gouders <dirk@gouders.net> writes: > >> Brian Lyles <brianmlyles@gmail.com> writes: >> >>> + if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) { >>> + /* >>> + * Check to see if this is an unborn branch >>> + */ >> In the above example, there is a short but outstanding comment that >> announces a check (and if I understood correctly by [1] it is a kind of >> trick that could deserve some more information) and it does _not_ >> comment on the result. Of course, I have an idea where the correct >> place for a comment /* This is an unborn branch -- handle it as if... */ >> could be, but I'm not sure. > > You mean "Check to see if this is an unborn branch, and if so, use > an empty tree to compare against, instead of the tree of the HEAD > that does not yet exist"? > > I think that is possible, but the use of the_hash_algo->empty_tree > indicates that clearly enough. But we need to stop somewhere and > what we see above may be a reasonable place to do so. > > If anything, we may want to say why we want to continue as if we had > an empty tree (as opposed to fail and return with an error()), or > the tree to compare with is computed here for what purpose. But the > name of the function may tell what this whole computation and > comparison is for, so it probably is not needed, either. Thank you for the reply. I guess, the hidden question in my comment was: "Do experienced Git developers understand the code as something obvious?". And I read your answer as a "Yes, no problem.". Now, I can put this subject aside and later, after more reading, check if my understanding improved sufficiently to now understand that code without additional comments, as you already do. Dirk
diff --git a/sequencer.c b/sequencer.c index f49a871ac0..f31d71ebad 100644 --- a/sequencer.c +++ b/sequencer.c @@ -770,29 +770,40 @@ 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; + const char *head_name; - 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)) { + /* + * Check to see if this is an unborn branch + */ + head_name = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, &head_oid, NULL); + if (!head_name || !starts_with(head_name, "refs/heads/") || !is_null_oid(&head_oid)) + return error(_("could not resolve HEAD commit")); + 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..af73227512 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 --exit-code 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 --exit-code 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. Detect unborn branches in `is_index_unchanged`. 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. Use simply `--exit-code` instead. Make these improvements to the existing test as well as the new test. Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Brian Lyles <brianmlyles@gmail.com> --- Changes from v3: - More robustly validate that we are on an unborn branch, rather than assuming that an error while reading the HEAD implies an unborn branch - Replace `--quiet` with `--exit-code` in the tests rather than just removing `--quiet`, to ensure that the test still fails appropriately if any differences are found. sequencer.c | 39 ++++++++++++++++++++++------------- t/t3501-revert-cherry-pick.sh | 14 ++++++++++--- 2 files changed, 36 insertions(+), 17 deletions(-)