{"thread":{"id":"62115","subject":"[PATCH v3] set-head: no update without change and better output for --auto","startedAt":"2024-09-15T22:17:12Z","lastAt":"2024-09-19T20:58:57Z","messageCount":6,"participants":["Bence Ferdinandy","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"502815","messageId":"20240915221055.904107-1-bence@ferdinandy.com","threadId":"62115","inReplyTo":null,"subject":"[PATCH v3] set-head: no update without change and better output for --auto","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-09-15T22:09:54Z","receivedAt":"2024-09-15T22:17:12Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"Currently, even if there is no actual change to remote/HEAD calling\nremote set-head will overwrite the appropriate file and if set to --auto\nwill also print a message saying \"remote/HEAD set to branch\", which\nimplies something was changed.\n\nChange the behaviour of remote set-head so that the reference is only\nupdated if it actually needs to change.\n\nChange the output of --auto, so the output actually reflects what was\ndone: a) set a previously unset HEAD, b) change HEAD because remote\nchanged or c) no updates. Make the output easily parsable, by using\na slightly clunky wording that allows all three outputs to have the same\nstructure and number of words.\n\nSigned-off-by: Bence Ferdinandy <bence@ferdinandy.com>\n---\n\nNotes:\n    v1-v2:\n    \n        was RFC in https://lore.kernel.org/git/20240910203835.2288291-1-bence@ferdinandy.com/\n    \n    v3:\n    \n        This patch was originally sent along when I thought set-head was\n        going to be invoked by fetch, but the discussion on the RFC\n        concluded that it should be not. This opened the possibility to make\n        it more explicit.\n    \n        Note: although I feel both things the patch does are really just\n        cosmetic, an argument could be made for breaking it into two, one\n        for the no-op part and one for the --auto print update.\n\n builtin/remote.c  | 14 ++++++++++----\n t/t5505-remote.sh |  2 +-\n 2 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 0acc547d69..4b116dfc19 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1400,8 +1400,8 @@ static int show(int argc, const char **argv, const char *prefix)\n \n static int set_head(int argc, const char **argv, const char *prefix)\n {\n-\tint i, opt_a = 0, opt_d = 0, result = 0;\n-\tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;\n+\tint i, opt_a = 0, opt_d = 0, is_ref_changed = 0, result = 0;\n+\tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT;\n \tchar *head_name = NULL;\n \n \tstruct option options[] = {\n@@ -1440,13 +1440,19 @@ static int set_head(int argc, const char **argv, const char *prefix)\n \n \tif (head_name) {\n \t\tstrbuf_addf(&buf2, \"refs/remotes/%s/%s\", argv[0], head_name);\n+\t\trefs_read_symbolic_ref(get_main_ref_store(the_repository),buf.buf,&buf3);\n+\t\tis_ref_changed = strcmp(buf2.buf,buf3.buf);\n \t\t/* make sure it's valid */\n \t\tif (!refs_ref_exists(get_main_ref_store(the_repository), buf2.buf))\n \t\t\tresult |= error(_(\"Not a valid ref: %s\"), buf2.buf);\n-\t\telse if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, \"remote set-head\"))\n+\t\telse if (is_ref_changed && refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, \"remote set-head\"))\n \t\t\tresult |= error(_(\"Could not setup %s\"), buf.buf);\n+\t\telse if (opt_a && !strcmp(buf3.buf,\"\"))\n+\t\t\tprintf(\"%s/HEAD was unset: set to %s\\n\", argv[0], head_name);\n+\t\telse if (opt_a && is_ref_changed)\n+\t\t\tprintf(\"%s/HEAD was changed: set to %s\\n\", argv[0], head_name);\n \t\telse if (opt_a)\n-\t\t\tprintf(\"%s/HEAD set to %s\\n\", argv[0], head_name);\n+\t\t\tprintf(\"%s/HEAD was unchanged: set to %s\\n\", argv[0], head_name);\n \t\tfree(head_name);\n \t}\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 532035933f..6b61f043d3 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -434,7 +434,7 @@ test_expect_success 'set-head --auto has no problem w/multiple HEADs' '\n \t\tcd test &&\n \t\tgit fetch two \"refs/heads/*:refs/remotes/two/*\" &&\n \t\tgit remote set-head --auto two >output 2>&1 &&\n-\t\techo \"two/HEAD set to main\" >expect &&\n+\t\techo \"two/HEAD was unset: set to main\" >expect &&\n \t\ttest_cmp expect output\n \t)\n '\n-- \n2.46.1.507.g531a0ea24e.dirty\n\n"},{"id":"502900","messageId":"xmqq5xqvo37s.fsf@gitster.g","threadId":"62115","inReplyTo":"20240915221055.904107-1-bence@ferdinandy.com","subject":"Re: [PATCH v3] set-head: no update without change and better output for --auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-16T18:36:39Z","receivedAt":"2024-09-16T18:36:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The message I am responding to is sent as the first message to start\na brand-new thread, but for future reference, it makes it easier for\neverybody involved if you sent a new iteration of a patch (or a\npatch series) as a reply/follow-up to the last round.\n\nIf it is too cumbersome to do for any reason, as an alternative, you\ncan give a reference to previous rounds as URLs to the list archive,\ni.e.\n\n (v1) https://lore.kernel.org/git/20240910203129.2251090-1-bence@ferdinandy.com/\n (v2) https://lore.kernel.org/git/20240910203835.2288291-1-bence@ferdinandy.com/\n\n> Currently, even if there is no actual change to remote/HEAD calling\n> remote set-head will overwrite the appropriate file and if set to --auto\n> will also print a message saying \"remote/HEAD set to branch\", which\n> implies something was changed.\n>\n> Change the behaviour of remote set-head so that the reference is only\n> updated if it actually needs to change.\n\nI sense a patch with a racy TOCTOU change coming ;-)\n\n> Change the output of --auto, so the output actually reflects what was\n> done: a) set a previously unset HEAD, b) change HEAD because remote\n> changed or c) no updates. Make the output easily parsable, by using\n> a slightly clunky wording that allows all three outputs to have the same\n> structure and number of words.\n\nThis would be useful.\n\n>         This patch was originally sent along when I thought set-head was\n>         going to be invoked by fetch, but the discussion on the RFC\n>         concluded that it should be not. This opened the possibility to make\n>         it more explicit.\n\nI am not quite sure what the discussion concluded \"should be not\",\nthough.  In a message from you\n\n  https://lore.kernel.org/git/D43G2CGX2N7L.ZRETD4HLIH0E@ferdinandy.com/\n\nwhat we agreed was that \"git fetch\" does not need a new option, but\nwe also agreed that it would be a good idea for \"git fetch\" to\ncreate, refs/remotes/$remote/HEAD when it does not exist without\nbeing told.\n\nI take it that you still want to see such a change to \"git fetch\"\neventually happen, but decided to tackle the other half of the\noriginal two-patch series first with this patch?\n\n>  static int set_head(int argc, const char **argv, const char *prefix)\n>  {\n> -\tint i, opt_a = 0, opt_d = 0, result = 0;\n> -\tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;\n> +\tint i, opt_a = 0, opt_d = 0, is_ref_changed = 0, result = 0;\n\nShall we simply call it ref_changed instead, so that I do not have\nto wonder which is better between has_ref_changed and is_ref_changed? ;-)\n\n> +\tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT;\n>  \tchar *head_name = NULL;\n>  \n>  \tstruct option options[] = {\n> @@ -1440,13 +1440,19 @@ static int set_head(int argc, const char **argv, const char *prefix)\n>  \n>  \tif (head_name) {\n>  \t\tstrbuf_addf(&buf2, \"refs/remotes/%s/%s\", argv[0], head_name);\n\n> +\t\trefs_read_symbolic_ref(get_main_ref_store(the_repository),buf.buf,&buf3);\n> +\t\tis_ref_changed = strcmp(buf2.buf,buf3.buf);\n>  \t\t/* make sure it's valid */\n>  \t\tif (!refs_ref_exists(get_main_ref_store(the_repository), buf2.buf))\n>  \t\t\tresult |= error(_(\"Not a valid ref: %s\"), buf2.buf);\n> -\t\telse if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, \"remote set-head\"))\n> +\t\telse if (is_ref_changed && refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, \"remote set-head\"))\n>  \t\t\tresult |= error(_(\"Could not setup %s\"), buf.buf);\n\nTwo and a half small issues:\n\n - Do not omit \" \" after \",\" and avoid overlong lines by folding\n   lines at an appropriate place (usually after an operator at\n   higher point in parse tree, i.e. after \"is_ref_changed &&\").\n\n - This is inherently racy, isn't it?  We read the _current_ value.\n   After we do so, but before we write _our_ value, another process\n   may update it, so we'd end up overwriting the value they wrote.\n\n - To compute is_ref_changed, we look at buf3.buf but what happens\n   if such a ref does not exist, exists but is not a symbolic ref,\n   or is a hierarchy under which other refs hang (e.g., a funny ref\n   \"refs/remotes/origin/HEAD/main\" exists)?  Does the strcmp()\n   compare a valid buf2.buf and an uninitialized junk in buf3.buf\n   and give a random result?  Shouldn't the code checking the return\n   value from the refs_read_symbolic_ref() call, and if we did so,\n   would we learn enough to tell among the cases where (1) it is a\n   symref, (2) it is a regular ref, (3) no such ref exists, and (4)\n   the refs layer hit an error to prevent us from giving one of the\n   previous 3 answers?\n\nIf we really wanted to resolve the raciness, I think we need to\nenhance the set of values that create_symref() can optionally return\nto its callers so that the caller can tell what the old value was.\n\nI am not sure offhand how involved such an update would be, though.\n\n> +\t\telse if (opt_a && !strcmp(buf3.buf,\"\"))\n> +\t\t\tprintf(\"%s/HEAD was unset: set to %s\\n\", argv[0], head_name);\n\nI suspect that this does not account for a case where the\n\"read_symbolic_ref()\" we did earlier failed for a reason other than\n\"the ref did not exist\".\n\n> +\t\telse if (opt_a && is_ref_changed)\n> +\t\t\tprintf(\"%s/HEAD was changed: set to %s\\n\", argv[0], head_name);\n>  \t\telse if (opt_a)\n> -\t\t\tprintf(\"%s/HEAD set to %s\\n\", argv[0], head_name);\n> +\t\t\tprintf(\"%s/HEAD was unchanged: set to %s\\n\", argv[0], head_name);\n>  \t\tfree(head_name);\n>  \t}\n\nQuoting the values we obtained from the environment or the user may\nbe nicer, e.g.\n\n    'refs/remotes/origin/HEAD' is now set to 'main'.\n    'refs/remotes/origin/HEAD' is unchanged and points at 'main'.\n\nThis is a tangent outside the immediate topic of this patch, but I\nwonder if we need \"--quiet\" mode.\n\nThanks.\n"},{"id":"502906","messageId":"D47YSXC0GSIW.MN2MW7IR9GMW@ferdinandy.com","threadId":"62115","inReplyTo":"xmqq5xqvo37s.fsf@gitster.g","subject":"Re: [PATCH v3] set-head: no update without change and better output for --auto","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-09-16T19:44:47Z","receivedAt":"2024-09-16T19:45:25Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nOn Mon Sep 16, 2024 at 20:36, Junio C Hamano <gitster@pobox.com> wrote:\n> The message I am responding to is sent as the first message to start\n> a brand-new thread, but for future reference, it makes it easier for\n> everybody involved if you sent a new iteration of a patch (or a\n> patch series) as a reply/follow-up to the last round.\n\nSorry about that, I've only ever used git-send-email on sourcehut before, where\nthey ask you _not_ to use --in-reply-to because of their UI and got used it...\nI'll do that next time!\n\n>\n> If it is too cumbersome to do for any reason, as an alternative, you\n> can give a reference to previous rounds as URLs to the list archive,\n> i.e.\n>\n>  (v1) https://lore.kernel.org/git/20240910203129.2251090-1-bence@ferdinandy.com/\n>  (v2) https://lore.kernel.org/git/20240910203835.2288291-1-bence@ferdinandy.com/\n\nTbf, I did put this in the commit notes :)\n\n>\n> > Currently, even if there is no actual change to remote/HEAD calling\n> > remote set-head will overwrite the appropriate file and if set to --auto\n> > will also print a message saying \"remote/HEAD set to branch\", which\n> > implies something was changed.\n> >\n> > Change the behaviour of remote set-head so that the reference is only\n> > updated if it actually needs to change.\n>\n> I sense a patch with a racy TOCTOU change coming ;-)\n\nIt seems that indeed :(\n\n[snip]\n\n> I am not quite sure what the discussion concluded \"should be not\",\n> though.  In a message from you\n>\n>   https://lore.kernel.org/git/D43G2CGX2N7L.ZRETD4HLIH0E@ferdinandy.com/\n>\n> what we agreed was that \"git fetch\" does not need a new option, but\n> we also agreed that it would be a good idea for \"git fetch\" to\n> create, refs/remotes/$remote/HEAD when it does not exist without\n> being told.\n>\n> I take it that you still want to see such a change to \"git fetch\"\n> eventually happen, but decided to tackle the other half of the\n> original two-patch series first with this patch?\n\nYes, but it did also turn up, that calling `git remote set-head --auto`\ndirectly from `git fetch` as I did in the original RFC requires the user to\nauthenticate twice, which is not the best UX. So now I'm hammering away at\nadding a \"set_head\" directly to fetch (I've sort-of figured it out already, but\nit is definitely more involved than this one, EDIT: since reading below about\nracyness, maybe this one is actually more complicated :D)). It's just that now\nI can handle the two patches independently.\n\n\n>\n> >  static int set_head(int argc, const char **argv, const char *prefix)\n> >  {\n> > -\tint i, opt_a = 0, opt_d = 0, result = 0;\n> > -\tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;\n> > +\tint i, opt_a = 0, opt_d = 0, is_ref_changed = 0, result = 0;\n>\n> Shall we simply call it ref_changed instead, so that I do not have\n> to wonder which is better between has_ref_changed and is_ref_changed? ;-)\n\nMakes sense :)\n\n>\n> > +\tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT;\n> >  \tchar *head_name = NULL;\n> >  \n> >  \tstruct option options[] = {\n> > @@ -1440,13 +1440,19 @@ static int set_head(int argc, const char **argv, const char *prefix)\n> >  \n> >  \tif (head_name) {\n> >  \t\tstrbuf_addf(&buf2, \"refs/remotes/%s/%s\", argv[0], head_name);\n>\n> > +\t\trefs_read_symbolic_ref(get_main_ref_store(the_repository),buf.buf,&buf3);\n> > +\t\tis_ref_changed = strcmp(buf2.buf,buf3.buf);\n> >  \t\t/* make sure it's valid */\n> >  \t\tif (!refs_ref_exists(get_main_ref_store(the_repository), buf2.buf))\n> >  \t\t\tresult |= error(_(\"Not a valid ref: %s\"), buf2.buf);\n> > -\t\telse if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, \"remote set-head\"))\n> > +\t\telse if (is_ref_changed && refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, \"remote set-head\"))\n> >  \t\t\tresult |= error(_(\"Could not setup %s\"), buf.buf);\n>\n> Two and a half small issues:\n>\n>  - Do not omit \" \" after \",\" and avoid overlong lines by folding\n>    lines at an appropriate place (usually after an operator at\n>    higher point in parse tree, i.e. after \"is_ref_changed &&\").\n>\n>  - This is inherently racy, isn't it?  We read the _current_ value.\n>    After we do so, but before we write _our_ value, another process\n>    may update it, so we'd end up overwriting the value they wrote.\n\nHonestly, I've just sort of assumed git in general protects against parallel\nwrites, but I guess I'm wrong :D We'd need to block writes to the reference\nwhile we're doing this, but I'm not quite sure how to achieve that.\n\n>\n>  - To compute is_ref_changed, we look at buf3.buf but what happens\n>    if such a ref does not exist, exists but is not a symbolic ref,\n>    or is a hierarchy under which other refs hang (e.g., a funny ref\n>    \"refs/remotes/origin/HEAD/main\" exists)?  Does the strcmp()\n>    compare a valid buf2.buf and an uninitialized junk in buf3.buf\n>    and give a random result?  Shouldn't the code checking the return\n>    value from the refs_read_symbolic_ref() call, and if we did so,\n>    would we learn enough to tell among the cases where (1) it is a\n>    symref, (2) it is a regular ref, (3) no such ref exists, and (4)\n>    the refs layer hit an error to prevent us from giving one of the\n>    previous 3 answers?\n>\n> If we really wanted to resolve the raciness, I think we need to\n> enhance the set of values that create_symref() can optionally return\n> to its callers so that the caller can tell what the old value was.\n>\n> I am not sure offhand how involved such an update would be, though.\n\nHmm, so there's a lot to unpack for me here. Is create_symref already atomic so\nif it could give back the previous value we could decide based on that what to\nprint (and forget about not writing the ref when there's no need)? Anyhow, I'll\nexplore this part of the code and see if I can understand what's going on.\n\n[snip]\n\n> Quoting the values we obtained from the environment or the user may\n> be nicer, e.g.\n>\n>     'refs/remotes/origin/HEAD' is now set to 'main'.\n>     'refs/remotes/origin/HEAD' is unchanged and points at 'main'.\n\nAck. I also gather from this that we should be a bit more verbose rather than\ntrying to make it easy to parse.\n\nI also forgot to free up buf3, which I'll do for the next round.\n\n>\n> This is a tangent outside the immediate topic of this patch, but I\n> wonder if we need \"--quiet\" mode.\n\nEven with the planned changed to fetch, if one want to be completely\nup-to-date, they'd be running fetch and remote set-head and since the former\nhas a quiet I think it would make sense. At least: adding it would be more\nstraight forward than solving raciness :D\n\nBest,\nBence\n\n\n\n\n\n-- \nbence.ferdinandy.com\n\n"},{"id":"502966","messageId":"D48UGAZA205N.37QFSURUDN3ZS@ferdinandy.com","threadId":"62115","inReplyTo":"xmqq5xqvo37s.fsf@gitster.g","subject":"Re: [PATCH v3] set-head: no update without change and better output for --auto","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-09-17T20:32:54Z","receivedAt":"2024-09-17T20:33:30Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nOn Mon Sep 16, 2024 at 20:36, Junio C Hamano <gitster@pobox.com> wrote:\n\n[snip]\n\n>  - This is inherently racy, isn't it?  We read the _current_ value.\n>    After we do so, but before we write _our_ value, another process\n>    may update it, so we'd end up overwriting the value they wrote.\n\nSo I've been thinking my first patch need not be so ambitious. The current\nbehaviour is to indiscriminately overwrite remote/HEAD. So what if we dial this\nback a bit, leave the indiscriminate overwrite in place (the added benefit of\nthat is pretty small anyway) and only improve the printed output, which was the\nmain goal anyway? \n\nThis is still slightly racy, since between the read and the write the status of\nremote/HEAD could still change potentially, so the information received by the\nuser may be _slightly_ off, as we may be printing that the command just created\nremote/HEAD when it was actually created a split millisecond earlier by another\ncommand, but the printed end result will be always correct. And since the\noutput is aimed at humans really, I'd say even if some other process did create\nthat remote/HEAD between read and write, it's still actually true that before\nrunning set-head it did not exist and now it does and is set to X.\n\nIn short: any raciness left should not have practical implications if we always\nactually write remote/HEAD.\n\nWhat do you think?\n\nBest,\nBence\n\n-- \nbence.ferdinandy.com\n\n"},{"id":"502968","messageId":"xmqq34lyknqy.fsf@gitster.g","threadId":"62115","inReplyTo":"D48UGAZA205N.37QFSURUDN3ZS@ferdinandy.com","subject":"Re: [PATCH v3] set-head: no update without change and better output for --auto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-17T20:51:17Z","receivedAt":"2024-09-17T20:51:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Bence Ferdinandy\" <bence@ferdinandy.com> writes:\n\n> On Mon Sep 16, 2024 at 20:36, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> [snip]\n>\n>>  - This is inherently racy, isn't it?  We read the _current_ value.\n>>    After we do so, but before we write _our_ value, another process\n>>    may update it, so we'd end up overwriting the value they wrote.\n>\n> So I've been thinking my first patch need not be so ambitious. The current\n> behaviour is to indiscriminately overwrite remote/HEAD. So what if we dial this\n> back a bit, leave the indiscriminate overwrite in place (the added benefit of\n> that is pretty small anyway) and only improve the printed output, which was the\n> main goal anyway? \n\nYes.  You could ignore races and it does not degrade correctness of\nthe end result (i.e., you will still get the same \"oops, that is not\nwhat I wrote, but somebody else overwritten what I wrote\" and vice\nversa).\n\nYour new messages that try to differenciate \"we noticed you are\nwriting the same so we refrained from doing anything\" and \"we wrote\nthis new value\" can racily incorrect the same way.  In other words,\nthe message may claim that we detected a no-op change from value A\nto new value A hence we didn't do anything, but after the detection\nsomebody may have wrote another value B from sideways, so the last\nmessage that hit the end user's eyes may imply the resulting value\nought to be A but it is not because somebody else silently changed\nit to B.  If we did not skip writing using racy logic, we would have\nwritten value A and reported that we updated it to value A, but the\nother party who silently wrote B may do so after all that happens,\nand the end result the user would see is the same (i.e. we tell the\nuser the last value we expect it to be is A, but the final value is\nB).\n\nSo, as long as we can explain the behaviour to the end-user well, I\ndo not care too deeply.  My impression was that avoiding it by just\ntaking advantage of the atomicity afforded by create_symref() looked\nlike a low hanging fruit, but that can be done by somebody who are\ncurious to see how involved such a change actually is and can be\nsafely left as a #leftoverbit ;-).\n\nThanks.\n\n\n\n"},{"id":"503097","messageId":"D4AK4USDVP5T.10INJOFE2I8LE@ferdinandy.com","threadId":"62115","inReplyTo":"xmqq34lyknqy.fsf@gitster.g","subject":"Re: [PATCH v3] set-head: no update without change and better output for --auto","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-09-19T20:53:05Z","receivedAt":"2024-09-19T20:58:57Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nOn Tue Sep 17, 2024 at 22:51, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> So, as long as we can explain the behaviour to the end-user well, I\n> do not care too deeply.  My impression was that avoiding it by just\n> taking advantage of the atomicity afforded by create_symref() looked\n> like a low hanging fruit, but that can be done by somebody who are\n> curious to see how involved such a change actually is and can be\n> safely left as a #leftoverbit ;-).\n>\n> Thanks.\n\n\nNow that I've poked around a bit (cf.\nhttps://lore.kernel.org/git/20240919121335.298856-2-bence@ferdinandy.com/T/#u)\nthat fruit does seem to hang lower :)\n\nMy idea is the following: Add a new member to the ref_update struct that\nrecords the value of the ref _before_ the update transaction (in the\nfiles-backend), that can then be accessed post transaction_commit in\nrefs_update_symref. If we want to pass this info up to the caller of\nrefs_update_symref then I guess the only option is to pass an extra buf where\nwe can write this, or if we're fine with update_symref itself doing the\nprinting we probably need a bool to turn this on/off.\n\nMy PoC seems to work and pass tests, although judging from previous\ninteractions I'm probably overlooking a couple of edge cases. \n\nDoes this seem reasonable?\n\nBest,\nBence\n"}]}