| Message ID | 20241118151755.756265-6-bence@ferdinandy.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | set-head/fetch remote/HEAD updates | expand |
Bence Ferdinandy <bence@ferdinandy.com> writes: > +static void report_set_head_auto(const char *remote, const char *head_name, > + struct strbuf *b_local_head, int updateres) { "updateres" was too mysterious a name. "res" stands for what, "resource"? Looking at the way the parameter is used by the code, it seems to indicate that the remote HEAD originally was in a detached state, so "was_detached" may be a better name, perhaps? > + else if (!!updateres && b_local_head->len) > + printf(_("'%s/HEAD' was detached at '%s' and now points to '%s'\n"), > + remote, b_local_head->buf, head_name); There is no need for !!; any non-zero integer is true. !! is useful only in a context that takes only 0 and 1 (like when you are making an assignment to a variable or a structure member that takes only 0 or 1). > static int set_head(int argc, const char **argv, const char *prefix) > { > - int i, opt_a = 0, opt_d = 0, result = 0; > - struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT; > + int i, opt_a = 0, opt_d = 0, result = 0, updateres; > + struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT, > + b_local_head = STRBUF_INIT; > @@ -1440,20 +1468,27 @@ static int set_head(int argc, const char **argv, const char *prefix) > } else > usage_with_options(builtin_remote_sethead_usage, options); > > - if (head_name) { > - strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", argv[0], head_name); > - /* make sure it's valid */ > - if (!refs_ref_exists(refs, b_remote_head.buf)) > - result |= error(_("Not a valid ref: %s"), b_remote_head.buf); > - else if (refs_update_symref(refs, b_head.buf, b_remote_head.buf, "remote set-head")) > - result |= error(_("Could not setup %s"), b_head.buf); > - else if (opt_a) > - printf("%s/HEAD set to %s\n", argv[0], head_name); > - free(head_name); > + if (!head_name) > + goto cleanup; > + strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", argv[0], head_name); > + if (!refs_ref_exists(refs, b_remote_head.buf)) { > + result |= error(_("Not a valid ref: %s"), b_remote_head.buf); > + goto cleanup; > + } OK, we refuse to allow a manual "remote set-head" to create a dangling symref, which is a faithful rewrite from the original. > + updateres = refs_update_symref_extended(refs, b_head.buf, b_remote_head.buf, > + "remote set-head", &b_local_head); > + if (updateres == -2) { Where does this -2 come from? It is not the "you asked to read it as a symref but it wasn't a symref" thing, which was mapped to -1 with [PATCH 3/9]. It is an unusual way to construct an extensible API function to say "all different kinds of errors we happen to know when this particular caller was written return -2, but some special cases are not -2". Rather, "all negatives, other than these selected few values we special-case and handle, are errors" is more natural, isn't it? Maybe I am misreading the code and missing where the -2 comes from or the significance of the value? I dunno.
On Tue Nov 19, 2024 at 03:27, Junio C Hamano <gitster@pobox.com> wrote: > Bence Ferdinandy <bence@ferdinandy.com> writes: > >> +static void report_set_head_auto(const char *remote, const char *head_name, >> + struct strbuf *b_local_head, int updateres) { > > "updateres" was too mysterious a name. "res" stands for what, > "resource"? > > Looking at the way the parameter is used by the code, it seems to > indicate that the remote HEAD originally was in a detached state, so > "was_detached" may be a better name, perhaps? "res" wanted to be short for result, but "was_detached" is definitely more readable. > >> + else if (!!updateres && b_local_head->len) >> + printf(_("'%s/HEAD' was detached at '%s' and now points to '%s'\n"), >> + remote, b_local_head->buf, head_name); > > There is no need for !!; any non-zero integer is true. !! is useful > only in a context that takes only 0 and 1 (like when you are making > an assignment to a variable or a structure member that takes only 0 > or 1). > >> static int set_head(int argc, const char **argv, const char *prefix) >> { >> - int i, opt_a = 0, opt_d = 0, result = 0; >> - struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT; >> + int i, opt_a = 0, opt_d = 0, result = 0, updateres; >> + struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT, >> + b_local_head = STRBUF_INIT; > >> @@ -1440,20 +1468,27 @@ static int set_head(int argc, const char **argv, const char *prefix) >> } else >> usage_with_options(builtin_remote_sethead_usage, options); >> >> - if (head_name) { >> - strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", argv[0], head_name); >> - /* make sure it's valid */ >> - if (!refs_ref_exists(refs, b_remote_head.buf)) >> - result |= error(_("Not a valid ref: %s"), b_remote_head.buf); >> - else if (refs_update_symref(refs, b_head.buf, b_remote_head.buf, "remote set-head")) >> - result |= error(_("Could not setup %s"), b_head.buf); >> - else if (opt_a) >> - printf("%s/HEAD set to %s\n", argv[0], head_name); >> - free(head_name); >> + if (!head_name) >> + goto cleanup; >> + strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", argv[0], head_name); >> + if (!refs_ref_exists(refs, b_remote_head.buf)) { >> + result |= error(_("Not a valid ref: %s"), b_remote_head.buf); >> + goto cleanup; >> + } > > OK, we refuse to allow a manual "remote set-head" to create a > dangling symref, which is a faithful rewrite from the original. > >> + updateres = refs_update_symref_extended(refs, b_head.buf, b_remote_head.buf, >> + "remote set-head", &b_local_head); > >> + if (updateres == -2) { > > Where does this -2 come from? It is not the "you asked to read it > as a symref but it wasn't a symref" thing, which was mapped to -1 > with [PATCH 3/9]. No, it is not, but it's also a mistake. It should be `updateres == 1`. refs_update_symref_extended outputs -1 for "not a symref" and 1 for any other error currently. Before I touched the code it was 1 for any error, so I left that as is. So we want to error out on set_head if we get a 1 and continue if we get 0 or -1 (and handle the difference in the report_set_head_auto). Thanks for noticing, I'll get that fixed in v14.
"Bence Ferdinandy" <bence@ferdinandy.com> writes: > No, it is not, but it's also a mistake. It should be `updateres == 1`. > refs_update_symref_extended outputs -1 for "not a symref" and 1 for any other > error currently. Before I touched the code it was 1 for any error, so I left > that as is. So we want to error out on set_head if we get a 1 and continue if > we get 0 or -1 (and handle the difference in the report_set_head_auto). > > Thanks for noticing, I'll get that fixed in v14. It is good that somebody noticed it (and it may have happened to be me), but if it is a "mistake" as you said, I wonder why none of your tests caught it. Do we have a gap in test coverage? Thanks.
On Tue Nov 19, 2024 at 11:54, Junio C Hamano <gitster@pobox.com> wrote: > "Bence Ferdinandy" <bence@ferdinandy.com> writes: > >> No, it is not, but it's also a mistake. It should be `updateres == 1`. >> refs_update_symref_extended outputs -1 for "not a symref" and 1 for any other >> error currently. Before I touched the code it was 1 for any error, so I left >> that as is. So we want to error out on set_head if we get a 1 and continue if >> we get 0 or -1 (and handle the difference in the report_set_head_auto). >> >> Thanks for noticing, I'll get that fixed in v14. > > It is good that somebody noticed it (and it may have happened to be > me), but if it is a "mistake" as you said, I wonder why none of your > tests caught it. Do we have a gap in test coverage? I think there is no test that is testing this branch: updateres = refs_update_symref_extended(refs, b_head.buf, b_remote_head.buf, "remote set-head", &b_local_head, 0); if (updateres == 1) { result |= error(_("Could not setup %s"), b_head.buf); goto cleanup; Running this in t/ grep -r "Could not setup" also yield nothing, so that's probably true. I'm wondering what would be the best way to trigger this error, refs_update_symref needs to fail for this.
On Tue Nov 19, 2024 at 11:54, Junio C Hamano <gitster@pobox.com> wrote: > "Bence Ferdinandy" <bence@ferdinandy.com> writes: > >> No, it is not, but it's also a mistake. It should be `updateres == 1`. >> refs_update_symref_extended outputs -1 for "not a symref" and 1 for any other >> error currently. Before I touched the code it was 1 for any error, so I left >> that as is. So we want to error out on set_head if we get a 1 and continue if >> we get 0 or -1 (and handle the difference in the report_set_head_auto). >> >> Thanks for noticing, I'll get that fixed in v14. > > It is good that somebody noticed it (and it may have happened to be > me), but if it is a "mistake" as you said, I wonder why none of your > tests caught it. Do we have a gap in test coverage? > > Thanks. I've been looking into how I can force an error on remote set-head to test the error branch. For the files backend it seems to be pretty easy touch .git/refs/remotes/origin/HEAD.lock git remote set-head [something] rm .git/refs/remotes/origin/HEAD.lock My problem is that in this case, since I want to force an error on a user command, I don't see a way that is possible with git commands and I'm not sure how I could manipulate the reftable file in a way that can be reversed and only errors out the particular thing I need. I've been looking through the functions called here and it seems other errors all depend passing wrong parameters but that is not possible (and should not be possible) to trigger with remote set-head, so I think something "manual" needs to be done, like the above. Since this particular test just wants to test what happens if `refs_update_symref_extended` returns with 1 and not testing correct behaviour of backends and such, would it be acceptable if this particular test case would check for the backend and if it is reftables it will just pass without actually checking and do the manually locking thing above for the files backend? Thanks, Bence
"Bence Ferdinandy" <bence@ferdinandy.com> writes: > Since this particular test just wants to test what happens if > `refs_update_symref_extended` returns with 1 and not testing correct behaviour > of backends and such, would it be acceptable if this particular test case would > check for the backend and if it is reftables it will just pass without actually > checking and do the manually locking thing above for the files backend? I think we have some pre-made test prerequisite to skip tests unless run with a particular ref backend, exactly for that. Perhaps test_expect_success REFFILES 'block update to check error' ' ... manually block update and see how the operation ... errors out ' or something?
diff --git a/builtin/remote.c b/builtin/remote.c index 1d68c5b2ba..a682ef5df2 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1399,10 +1399,38 @@ static int show(int argc, const char **argv, const char *prefix) return result; } +static void report_set_head_auto(const char *remote, const char *head_name, + struct strbuf *b_local_head, int updateres) { + struct strbuf buf_prefix = STRBUF_INIT; + const char *prev_head = NULL; + + strbuf_addf(&buf_prefix, "refs/remotes/%s/", remote); + skip_prefix(b_local_head->buf, buf_prefix.buf, &prev_head); + + if (prev_head && !strcmp(prev_head, head_name)) + printf(_("'%s/HEAD' is unchanged and points to '%s'\n"), + remote, head_name); + else if (prev_head) + printf(_("'%s/HEAD' has changed from '%s' and now points to '%s'\n"), + remote, prev_head, head_name); + else if (!b_local_head->len) + printf(_("'%s/HEAD' is now created and points to '%s'\n"), + remote, head_name); + else if (!!updateres && b_local_head->len) + printf(_("'%s/HEAD' was detached at '%s' and now points to '%s'\n"), + remote, b_local_head->buf, head_name); + else + printf(_("'%s/HEAD' used to point to '%s' " + "(which is not a remote branch), but now points to '%s'\n"), + remote, b_local_head->buf, head_name); + strbuf_release(&buf_prefix); +} + static int set_head(int argc, const char **argv, const char *prefix) { - int i, opt_a = 0, opt_d = 0, result = 0; - struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT; + int i, opt_a = 0, opt_d = 0, result = 0, updateres; + struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT, + b_local_head = STRBUF_INIT; char *head_name = NULL; struct ref_store *refs = get_main_ref_store(the_repository); @@ -1440,20 +1468,27 @@ static int set_head(int argc, const char **argv, const char *prefix) } else usage_with_options(builtin_remote_sethead_usage, options); - if (head_name) { - strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", argv[0], head_name); - /* make sure it's valid */ - if (!refs_ref_exists(refs, b_remote_head.buf)) - result |= error(_("Not a valid ref: %s"), b_remote_head.buf); - else if (refs_update_symref(refs, b_head.buf, b_remote_head.buf, "remote set-head")) - result |= error(_("Could not setup %s"), b_head.buf); - else if (opt_a) - printf("%s/HEAD set to %s\n", argv[0], head_name); - free(head_name); + if (!head_name) + goto cleanup; + strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", argv[0], head_name); + if (!refs_ref_exists(refs, b_remote_head.buf)) { + result |= error(_("Not a valid ref: %s"), b_remote_head.buf); + goto cleanup; + } + updateres = refs_update_symref_extended(refs, b_head.buf, b_remote_head.buf, + "remote set-head", &b_local_head); + if (updateres == -2) { + result |= error(_("Could not setup %s"), b_head.buf); + goto cleanup; } + if (opt_a) + report_set_head_auto(argv[0], head_name, &b_local_head, updateres); +cleanup: + free(head_name); strbuf_release(&b_head); strbuf_release(&b_remote_head); + strbuf_release(&b_local_head); return result; } diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 9b50276646..807df00ba7 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -432,12 +432,63 @@ test_expect_success 'set-head --auto' ' ) ' +test_expect_success 'set-head --auto detects creation' ' + ( + cd test && + git update-ref --no-deref -d refs/remotes/origin/HEAD && + git remote set-head --auto origin >output && + echo "${SQ}origin/HEAD${SQ} is now created and points to ${SQ}main${SQ}" >expect && + test_cmp expect output + ) +' + +test_expect_success 'set-head --auto to update a non symbolic ref' ' + ( + cd test && + git update-ref --no-deref -d refs/remotes/origin/HEAD && + git update-ref refs/remotes/origin/HEAD HEAD && + HEAD=$(git log --pretty="%H") && + git remote set-head --auto origin >output && + echo "${SQ}origin/HEAD${SQ} was detached at ${SQ}${HEAD}${SQ} and now points to ${SQ}main${SQ}" >expect && + test_cmp expect output + ) +' + +test_expect_success 'set-head --auto detects no change' ' + ( + cd test && + git remote set-head --auto origin >output && + echo "${SQ}origin/HEAD${SQ} is unchanged and points to ${SQ}main${SQ}" >expect && + test_cmp expect output + ) +' + +test_expect_success 'set-head --auto detects change' ' + ( + cd test && + git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/ahead && + git remote set-head --auto origin >output && + echo "${SQ}origin/HEAD${SQ} has changed from ${SQ}ahead${SQ} and now points to ${SQ}main${SQ}" >expect && + test_cmp expect output + ) +' + +test_expect_success 'set-head --auto detects strange ref' ' + ( + cd test && + git symbolic-ref refs/remotes/origin/HEAD refs/heads/main && + git remote set-head --auto origin >output && + echo "${SQ}origin/HEAD${SQ} used to point to ${SQ}refs/heads/main${SQ} (which is not a remote branch), but now points to ${SQ}main${SQ}" >expect && + test_cmp expect output + ) +' + test_expect_success 'set-head --auto has no problem w/multiple HEADs' ' ( cd test && git fetch two "refs/heads/*:refs/remotes/two/*" && git remote set-head --auto two >output 2>&1 && - echo "two/HEAD set to main" >expect && + echo "${SQ}two/HEAD${SQ} is now created and points to ${SQ}main${SQ}" >expect && test_cmp expect output ) ' @@ -456,6 +507,16 @@ test_expect_success 'set-head explicit' ' ) ' +test_expect_success 'set-head --auto reports change' ' + ( + cd test && + git remote set-head origin side2 && + git remote set-head --auto origin >output 2>&1 && + echo "${SQ}origin/HEAD${SQ} has changed from ${SQ}side2${SQ} and now points to ${SQ}main${SQ}" >expect && + test_cmp expect output + ) +' + cat >test/expect <<EOF Pruning origin URL: $(pwd)/one
Currently, set-head --auto will print a message saying "remote/HEAD set to branch", which implies something was changed. Change the output of --auto, so the output actually reflects what was done: a) set a previously unset HEAD, b) change HEAD because remote changed or c) no updates. As edge cases, if HEAD is changed from a previous symbolic reference that was not a remote branch, explicitly call attention to this fact, and also notify the user if the previous reference was not a symbolic reference. Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> --- Notes: v1-v2: was RFC in https://lore.kernel.org/git/20240910203835.2288291-1-bence@ferdinandy.com/ v3: This patch was originally sent along when I thought set-head was going to be invoked by fetch, but the discussion on the RFC concluded that it should be not. This opened the possibility to make it more explicit. Note: although I feel both things the patch does are really just cosmetic, an argument could be made for breaking it into two, one for the no-op part and one for the --auto print update. Was sent in: https://lore.kernel.org/git/20240915221055.904107-1-bence@ferdinandy.com/ v4: - changes are now handled atomically via the ref update transaction - outputs have changed along the lines of Junio's suggestion - minor refactor to set_head for improved legibility v5: - the minor refactor has been split out into its own patch v6: - fixed uninitialized prev_head - fixed case of unusual previous target - fixed a test that would have been actually broken at this patch (the output was only correct with the later update to fetch) - added 4 tests for the 4 output cases v7: - change commit prefix to be more in line with project standards - fixed tests to also work with the reftable backend - renamed report function, fixed style issue with checking buf len - fixed not releasing an strbuf v8: no change v9: - mark output strings in report_set_head_auto as translatable - rename buf_prev to b_local_head for consistency - use ${SQ} in tests instead of '\'' v10: no change v11: no change v12: no change v13: added handling the edge case of previous remote/HEAD being a detached HEAD builtin/remote.c | 59 +++++++++++++++++++++++++++++++++++--------- t/t5505-remote.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 13 deletions(-)