| Message ID | 20241010133022.1733542-3-bence@ferdinandy.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/6] refs_update_symref: atomically record overwritten ref | expand |
Bence Ferdinandy <bence@ferdinandy.com> writes: [snip] > diff --git a/builtin/remote.c b/builtin/remote.c > index 353ffd2c43..24f9caf149 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -1399,10 +1399,34 @@ static int show(int argc, const char **argv, const char *prefix) > return result; > } > > +static void report_auto(const char *remote, const char *head_name, > + struct strbuf *buf_prev) { Nit: would be nicer if this was called 'report_set_head_auto' > + struct strbuf buf_prefix = STRBUF_INIT; > + const char *prev_head = NULL; > + > + strbuf_addf(&buf_prefix, "refs/remotes/%s/", remote); > + skip_prefix(buf_prev->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 (buf_prev->len == 0) Nit: we tend to do `if (!buf_prev->len)` in this project > + printf("'%s/HEAD' is now created and points to '%s'\n", > + remote, head_name); > + else > + printf("'%s/HEAD' used to point to '%s' " > + "(which is unusual), but now points to '%s'\n", Isn't "which is unusual" a bit vague? Wouldn't be nicer to say why it is unusual? > + remote, buf_prev->buf, head_name); Let's clear up buf_prefix by calling `strbuf_release` here. > +} > + > static int set_head(int argc, const char **argv, const char *prefix) > { > int i, opt_a = 0, opt_d = 0, result = 0; > - struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT; > + struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, > + buf_prev = STRBUF_INIT; > char *head_name = NULL; > struct ref_store *refs = get_main_ref_store(the_repository); > > @@ -1445,15 +1469,17 @@ static int set_head(int argc, const char **argv, const char *prefix) > /* make sure it's valid */ > if (!refs_ref_exists(refs, buf2.buf)) > result |= error(_("Not a valid ref: %s"), buf2.buf); > - else if (refs_update_symref(refs, buf.buf, buf2.buf, "remote set-head", NULL)) > + else if (refs_update_symref(refs, buf.buf, buf2.buf, "remote set-head", &buf_prev)) > result |= error(_("Could not setup %s"), buf.buf); > - else if (opt_a) > - printf("%s/HEAD set to %s\n", argv[0], head_name); > + else if (opt_a) { > + report_auto(argv[0], head_name, &buf_prev); > + } > free(head_name); > } > > strbuf_release(&buf); > strbuf_release(&buf2); > + strbuf_release(&buf_prev); > return result; > } > > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh > index 532035933f..77c12b8709 100755 > --- a/t/t5505-remote.sh > +++ b/t/t5505-remote.sh > @@ -429,12 +429,51 @@ test_expect_success 'set-head --auto' ' > ) > ' > > +test_expect_success 'set-head --auto detects creation' ' > + ( > + cd test && > + rm .git/refs/remotes/origin/HEAD && does this work with reftables too? > + git remote set-head --auto origin >output && > + echo "'\''origin/HEAD'\'' is now created and points to '\''main'\''" >expect && Nit: might be cleaner to use `${SQ}` here and below [snip]
Thanks for the very helpful review on the series! Most of it is clear to me and I agree with, so for brevity I'll only comment where I have a question/comment. On Thu Oct 10, 2024 at 23:05, karthik nayak <karthik.188@gmail.com> wrote: [snip] > > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh > > index 532035933f..77c12b8709 100755 > > --- a/t/t5505-remote.sh > > +++ b/t/t5505-remote.sh > > @@ -429,12 +429,51 @@ test_expect_success 'set-head --auto' ' > > ) > > ' > > > > +test_expect_success 'set-head --auto detects creation' ' > > + ( > > + cd test && > > + rm .git/refs/remotes/origin/HEAD && > > does this work with reftables too? You got me there, probably not. I'm guessing I should use git update-ref or something similar to set these values manually instead of editing the file. On the other hand, is there a way to force the tests to run on a reftables backend? t/README does not mention anything about this and since in a previous round it already came up that I forgot to update the reftables backend, it would be nice if I could actually test everything with that as well. > > > + git remote set-head --auto origin >output && > > + echo "'\''origin/HEAD'\'' is now created and points to '\''main'\''" >expect && > > Nit: might be cleaner to use `${SQ}` here and below You mean something like this? git remote set-head --auto origin >output && HEAD="'\''origin/HEAD'\''" && echo "${HEAD} is now created and points to '\''main'\''" >expect && I tried a few variations and this is what I could get working, but I may be simply missing something with the backtick. Thanks, Bence
"Bence Ferdinandy" <bence@ferdinandy.com> writes: > Thanks for the very helpful review on the series! Most of it is clear to me and > I agree with, so for brevity I'll only comment where I have a question/comment. > > On Thu Oct 10, 2024 at 23:05, karthik nayak <karthik.188@gmail.com> wrote: > [snip] > >> > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh >> > index 532035933f..77c12b8709 100755 >> > --- a/t/t5505-remote.sh >> > +++ b/t/t5505-remote.sh >> > @@ -429,12 +429,51 @@ test_expect_success 'set-head --auto' ' >> > ) >> > ' >> > >> > +test_expect_success 'set-head --auto detects creation' ' >> > + ( >> > + cd test && >> > + rm .git/refs/remotes/origin/HEAD && >> >> does this work with reftables too? > > You got me there, probably not. I'm guessing I should use git update-ref or > something similar to set these values manually instead of editing the file. Yup, "git update-ref --no-deref -d refs/remotes/origin/HEAD" > On the other hand, is there a way to force the tests to run on a reftables > backend? t/README does not mention anything about this and since in a previous > round it already came up that I forgot to update the reftables backend, it > would be nice if I could actually test everything with that as well. $ git grep GIT_TEST_DEFAULT_REF_FORMAT t/README Thanks.
"Bence Ferdinandy" <bence@ferdinandy.com> writes: [snip] >> >> > + git remote set-head --auto origin >output && >> > + echo "'\''origin/HEAD'\'' is now created and points to '\''main'\''" >expect && >> >> Nit: might be cleaner to use `${SQ}` here and below > You mean something like this? > > git remote set-head --auto origin >output && > HEAD="'\''origin/HEAD'\''" && > echo "${HEAD} is now created and points to '\''main'\''" >expect && > > I tried a few variations and this is what I could get working, but I may be > simply missing something with the backtick. I mean simply this git remote set-head --auto origin >output && echo "${SQ}origin/HEAD${SQ} is now created and points to ${SQ}main${SQ}" >expect && - Karthik
On Fri Oct 11, 2024 at 22:43, karthik nayak <karthik.188@gmail.com> wrote: > "Bence Ferdinandy" <bence@ferdinandy.com> writes: > > [snip] > >>> >>> > + git remote set-head --auto origin >output && >>> > + echo "'\''origin/HEAD'\'' is now created and points to '\''main'\''" >expect && >>> >>> Nit: might be cleaner to use `${SQ}` here and below >> You mean something like this? >> >> git remote set-head --auto origin >output && >> HEAD="'\''origin/HEAD'\''" && >> echo "${HEAD} is now created and points to '\''main'\''" >expect && >> >> I tried a few variations and this is what I could get working, but I may be >> simply missing something with the backtick. > > I mean simply this > > git remote set-head --auto origin >output && > echo "${SQ}origin/HEAD${SQ} is now created and points to > ${SQ}main${SQ}" >expect && Ah, I see in other tests this is used, but not in this particular test file. It's a bit hard to decide which is more cryptic, but ${SQ} is nicer on the eyes. On the other hand I would either switch the entire file in a separate patch or leave in the '\'' here as well. Or I guess one could go through the entire test base and switch everything to either one or the other for consistency.
"Bence Ferdinandy" <bence@ferdinandy.com> writes: > On Fri Oct 11, 2024 at 22:43, karthik nayak <karthik.188@gmail.com> wrote: >> "Bence Ferdinandy" <bence@ferdinandy.com> writes: >> >> [snip] >> >>>> >>>> > + git remote set-head --auto origin >output && >>>> > + echo "'\''origin/HEAD'\'' is now created and points to '\''main'\''" >expect && >>>> >>>> Nit: might be cleaner to use `${SQ}` here and below >>> You mean something like this? >>> >>> git remote set-head --auto origin >output && >>> HEAD="'\''origin/HEAD'\''" && >>> echo "${HEAD} is now created and points to '\''main'\''" >expect && >>> >>> I tried a few variations and this is what I could get working, but I may be >>> simply missing something with the backtick. >> >> I mean simply this >> >> git remote set-head --auto origin >output && >> echo "${SQ}origin/HEAD${SQ} is now created and points to >> ${SQ}main${SQ}" >expect && > > Ah, I see in other tests this is used, but not in this particular test file. > It's a bit hard to decide which is more cryptic, but ${SQ} is nicer on the > eyes. On the other hand I would either switch the entire file in a separate > patch or leave in the '\'' here as well. Or I guess one could go through the > entire test base and switch everything to either one or the other for > consistency. I'm not sure I entirely agree with this sentiment. Consistency is a great goal to target, but it shouldn't hinder changes that are beneficial. In our case, if you make the first change, there is now reason for upcoming changes to the same file to also use '${SQ}' and eventually we can reach consistency of using '${SQ}' throughout the file. - Karthik
On 15/10/2024 08:51, karthik nayak wrote: > "Bence Ferdinandy" <bence@ferdinandy.com> writes: >> On Fri Oct 11, 2024 at 22:43, karthik nayak <karthik.188@gmail.com> wrote: >>> "Bence Ferdinandy" <bence@ferdinandy.com> writes: >>> I mean simply this >>> >>> git remote set-head --auto origin >output && >>> echo "${SQ}origin/HEAD${SQ} is now created and points to >>> ${SQ}main${SQ}" >expect && >> >> Ah, I see in other tests this is used, but not in this particular test file. >> It's a bit hard to decide which is more cryptic, but ${SQ} is nicer on the >> eyes. On the other hand I would either switch the entire file in a separate >> patch or leave in the '\'' here as well. Or I guess one could go through the >> entire test base and switch everything to either one or the other for >> consistency. > > I'm not sure I entirely agree with this sentiment. Consistency is a > great goal to target, but it shouldn't hinder changes that are > beneficial. Exactly - if we wait for an entire test file to be modernized before using our modern test idioms in it we'll be waiting forever. It is much better to introduce the use of things like ${SQ} that improve readability as we add new tests or modify existing ones. Best Wishes Phillip In our case, if you make the first change, there is now > reason for upcoming changes to the same file to also use '${SQ}' and > eventually we can reach consistency of using '${SQ}' throughout the file. > > - Karthik
On Tue Oct 15, 2024 at 16:10, Phillip Wood <phillip.wood123@gmail.com> wrote: > On 15/10/2024 08:51, karthik nayak wrote: >> "Bence Ferdinandy" <bence@ferdinandy.com> writes: >>> On Fri Oct 11, 2024 at 22:43, karthik nayak <karthik.188@gmail.com> wrote: >> >> I'm not sure I entirely agree with this sentiment. Consistency is a >> great goal to target, but it shouldn't hinder changes that are >> beneficial. > > Exactly - if we wait for an entire test file to be modernized before > using our modern test idioms in it we'll be waiting forever. It is much > better to introduce the use of things like ${SQ} that improve > readability as we add new tests or modify existing ones. Ok, I'll change the patch to use SQ. As I gather, it would be preferred if _all_ the tests would use ${SQ}? If this patch series is finally resolved I can send a patch for this style change, shouldn't be too complicated.
diff --git a/builtin/remote.c b/builtin/remote.c index 353ffd2c43..24f9caf149 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1399,10 +1399,34 @@ static int show(int argc, const char **argv, const char *prefix) return result; } +static void report_auto(const char *remote, const char *head_name, + struct strbuf *buf_prev) { + struct strbuf buf_prefix = STRBUF_INIT; + const char *prev_head = NULL; + + strbuf_addf(&buf_prefix, "refs/remotes/%s/", remote); + skip_prefix(buf_prev->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 (buf_prev->len == 0) + printf("'%s/HEAD' is now created and points to '%s'\n", + remote, head_name); + else + printf("'%s/HEAD' used to point to '%s' " + "(which is unusual), but now points to '%s'\n", + remote, buf_prev->buf, head_name); +} + static int set_head(int argc, const char **argv, const char *prefix) { int i, opt_a = 0, opt_d = 0, result = 0; - struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, + buf_prev = STRBUF_INIT; char *head_name = NULL; struct ref_store *refs = get_main_ref_store(the_repository); @@ -1445,15 +1469,17 @@ static int set_head(int argc, const char **argv, const char *prefix) /* make sure it's valid */ if (!refs_ref_exists(refs, buf2.buf)) result |= error(_("Not a valid ref: %s"), buf2.buf); - else if (refs_update_symref(refs, buf.buf, buf2.buf, "remote set-head", NULL)) + else if (refs_update_symref(refs, buf.buf, buf2.buf, "remote set-head", &buf_prev)) result |= error(_("Could not setup %s"), buf.buf); - else if (opt_a) - printf("%s/HEAD set to %s\n", argv[0], head_name); + else if (opt_a) { + report_auto(argv[0], head_name, &buf_prev); + } free(head_name); } strbuf_release(&buf); strbuf_release(&buf2); + strbuf_release(&buf_prev); return result; } diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 532035933f..77c12b8709 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -429,12 +429,51 @@ test_expect_success 'set-head --auto' ' ) ' +test_expect_success 'set-head --auto detects creation' ' + ( + cd test && + rm .git/refs/remotes/origin/HEAD && + git remote set-head --auto origin >output && + echo "'\''origin/HEAD'\'' is now created and points to '\''main'\''" >expect && + test_cmp expect output + ) +' + +test_expect_success 'set-head --auto detects no change' ' + ( + cd test && + git remote set-head --auto origin >output && + echo "'\''origin/HEAD'\'' is unchanged and points to '\''main'\''" >expect && + test_cmp expect output + ) +' + +test_expect_success 'set-head --auto detects change' ' + ( + cd test && + echo "ref: refs/remotes/origin/ahead" >.git/refs/remotes/origin/HEAD && + git remote set-head --auto origin >output && + echo "'\''origin/HEAD'\'' has changed from '\''ahead'\'' and now points to '\''main'\''" >expect && + test_cmp expect output + ) +' + +test_expect_success 'set-head --auto detects strange ref' ' + ( + cd test && + echo "ref: refs/heads/main" >.git/refs/remotes/origin/HEAD && + git remote set-head --auto origin >output && + echo "'\''origin/HEAD'\'' used to point to '\''refs/heads/main'\'' (which is unusual), but now points to '\''main'\''" >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 "'\''two/HEAD'\'' is now created and points to '\''main'\''" >expect && test_cmp expect output ) ' @@ -453,6 +492,17 @@ 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 "'\''origin/HEAD'\'' has changed from '\''side2'\'' and now points to '\''main'\''" >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 a fourth output, if HEAD is changed from a previous value that was not a remote branch, explicitly call attention to this fact. 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 builtin/remote.c | 34 +++++++++++++++++++++++++++---- t/t5505-remote.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 5 deletions(-)