{"thread":{"id":"59290","subject":"[PATCH] update-ref: add forward command to safely fast-forward refs","startedAt":"2023-02-23T01:22:39Z","lastAt":"2023-02-23T18:02:00Z","messageCount":3,"participants":["Ronan Pigott","Junio C Hamano","ronan@rjp.ie"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"472469","messageId":"20230223011530.47477-1-ronan@rjp.ie","threadId":"59290","inReplyTo":null,"subject":"[PATCH] update-ref: add forward command to safely fast-forward refs","fromName":"Ronan Pigott","fromEmail":"ronan@rjp.ie","sentAt":"2023-02-23T01:15:30Z","receivedAt":"2023-02-23T01:22:39Z","isPatch":true,"sender":{"key":"ronan@rjp.ie","avatar":"https://avatars.githubusercontent.com/u/12577529?v=4"},"body":"forward is an update-ref command that behaves similarly to update, but\ntakes an additional argument, <ancestor>, and verifies that the new\nvalue is a descendent of ancestor before updating. This is useful for\nfast-forwarding prefetched refs.\n---\nI've found git fetch --prefetch to be useful in my workflow for\nasynchronously updating git repos, but I'd really like a nice way to\nupdate my remote tracking branches using the latest prefetched refs\nwithout querying the remote in case the remote is temporarily\nunavailable or I'm on a plane or something. I've settled on using\n\n$ git for-each-ref refs/prefetch/ --format='update refs/%(refname:lstrip=2) %(refname)'\n\nto prepare a transaction for git update-ref, but it has a defect: if I\nhave fetched the remote more recently than the last prefetch my remote\ntracking branches will be sent into the past. \"forward\" is a convenience\nfeature for update-ref that will save the trouble of looping through\neach ref and finding the merge base so it can be fast-forwarded.\n Documentation/git-update-ref.txt |  5 ++++\n builtin/update-ref.c             | 49 ++++++++++++++++++++++++++++++++\n t/t1400-update-ref.sh            | 29 +++++++++++++++++++\n 3 files changed, 83 insertions(+)\n\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex 48b6683071e6..138a478b437f 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -66,6 +66,7 @@ performs all modifications together.  Specify commands of the form:\n \tdelete SP <ref> [SP <oldvalue>] LF\n \tverify SP <ref> [SP <oldvalue>] LF\n \toption SP <opt> LF\n+\tforward SP <ref> SP <newvalue> SP <ancestor> [SP <oldvalue>] LF\n \tstart LF\n \tprepare LF\n \tcommit LF\n@@ -105,6 +106,10 @@ update::\n \tafter the update and/or a zero <oldvalue> to make sure the\n \tref does not exist before the update.\n \n+forward::\n+\tSet <ref> to <newvalue> after verifying <oldvalue> only if\n+\t<newvalue> is a descendent of <ancestor>.\n+\n create::\n \tCreate <ref> with <newvalue> after verifying it does not\n \texist.  The given <newvalue> may not be zero.\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex a84e7b47a206..cdffa52b2d93 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"config.h\"\n+#include \"commit-reach.h\"\n #include \"refs.h\"\n #include \"builtin.h\"\n #include \"parse-options.h\"\n@@ -86,6 +87,11 @@ static char *parse_refname(const char **next)\n  */\n #define PARSE_SHA1_ALLOW_EMPTY 0x02\n \n+/*\n+ * The value being parsed is <ancestor>, for forward updates.\n+ */\n+#define PARSE_SHA1_ANCESTOR 0x04\n+\n /*\n  * Parse an argument separator followed by the next argument, if any.\n  * If there is an argument, convert it to a SHA-1, write it to sha1,\n@@ -157,12 +163,16 @@ static int parse_next_oid(const char **next, const char *end,\n  invalid:\n \tdie(flags & PARSE_SHA1_OLD ?\n \t    \"%s %s: invalid <oldvalue>: %s\" :\n+\t    flags & PARSE_SHA1_ANCESTOR ?\n+\t    \"%s %s: invalid <ancestor>: %s\" :\n \t    \"%s %s: invalid <newvalue>: %s\",\n \t    command, refname, arg.buf);\n \n  eof:\n \tdie(flags & PARSE_SHA1_OLD ?\n \t    \"%s %s: unexpected end of input when reading <oldvalue>\" :\n+\t    flags & PARSE_SHA1_ANCESTOR ?\n+\t    \"%s %s: unexpected end of input when reading <ancestor>\" :\n \t    \"%s %s: unexpected end of input when reading <newvalue>\",\n \t    command, refname);\n }\n@@ -211,6 +221,44 @@ static void parse_cmd_update(struct ref_transaction *transaction,\n \tstrbuf_release(&err);\n }\n \n+static void parse_cmd_forward(struct ref_transaction *transaction,\n+\t\t\t     const char *next, const char *end)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tchar *refname;\n+\tstruct object_id new_oid, anc_oid, old_oid;\n+\tint have_old;\n+\n+\trefname = parse_refname(&next);\n+\tif (!refname)\n+\t\tdie(\"forward: missing <ref>\");\n+\n+\tif (parse_next_oid(&next, end, &new_oid, \"forward\", refname, 0))\n+\t\tdie(\"forward %s: missing <newvalue>\", refname);\n+\n+\tif (parse_next_oid(&next, end, &anc_oid, \"forward\", refname, PARSE_SHA1_ANCESTOR))\n+\t\tdie(\"forward %s: missing <ancestor>\", refname);\n+\n+\thave_old = !parse_next_oid(&next, end, &old_oid, \"forward\", refname,\n+\t\t\t\t   PARSE_SHA1_OLD);\n+\n+\tif (*next != line_termination)\n+\t\tdie(\"forward %s: extra input: %s\", refname, next);\n+\n+\tif (!ref_newer(&new_oid, &anc_oid))\n+\t\tdie(\"forward %s: not a fast-forward\", refname);\n+\n+\tif (ref_transaction_update(transaction, refname,\n+\t\t\t\t   &new_oid, have_old ? &old_oid : NULL,\n+\t\t\t\t   update_flags | create_reflog_flag,\n+\t\t\t\t   msg, &err))\n+\t\tdie(\"%s\", err.buf);\n+\n+\tupdate_flags = default_flags;\n+\tfree(refname);\n+\tstrbuf_release(&err);\n+}\n+\n static void parse_cmd_create(struct ref_transaction *transaction,\n \t\t\t     const char *next, const char *end)\n {\n@@ -378,6 +426,7 @@ static const struct parse_cmd {\n \tenum update_refs_state state;\n } command[] = {\n \t{ \"update\",  parse_cmd_update,  3, UPDATE_REFS_OPEN },\n+\t{ \"forward\", parse_cmd_forward, 4, UPDATE_REFS_OPEN },\n \t{ \"create\",  parse_cmd_create,  2, UPDATE_REFS_OPEN },\n \t{ \"delete\",  parse_cmd_delete,  2, UPDATE_REFS_OPEN },\n \t{ \"verify\",  parse_cmd_verify,  2, UPDATE_REFS_OPEN },\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex cf58cf025cd2..6ac90aca1917 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -785,6 +785,35 @@ test_expect_success 'stdin update ref works with right old value' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'stdin forward ref fails with wrong ancestor value' '\n+\techo \"forward $c $m~1 $m\" >stdin &&\n+\ttest_must_fail git update-ref --stdin <stdin 2>err &&\n+\tgrep \"forward $c: not a fast-forward\" err &&\n+\ttest_must_fail git rev-parse --verify -q $c\n+'\n+\n+test_expect_success 'stdin forward ref fails with bad ancestor value' '\n+\techo \"forward $c $m does-not-exist\" >stdin &&\n+\ttest_must_fail git update-ref --stdin <stdin 2>err &&\n+\tgrep \"fatal: forward $c: invalid <ancestor>: does-not-exist\" err &&\n+\ttest_must_fail git rev-parse --verify -q $c\n+'\n+\n+test_expect_success 'stdin forward ref fails with bad old value' '\n+\techo \"forward $c $m $m~1 does-not-exist\" >stdin &&\n+\ttest_must_fail git update-ref --stdin <stdin 2>err &&\n+\tgrep \"fatal: forward $c: invalid <oldvalue>: does-not-exist\" err &&\n+\ttest_must_fail git rev-parse --verify -q $c\n+'\n+\n+test_expect_success 'stdin forward ref works with right ancestor value' '\n+\techo \"forward $b $m~1 $m~2\" >stdin &&\n+\tgit update-ref --stdin <stdin &&\n+\tgit rev-parse $m~1 >expect &&\n+\tgit rev-parse $b >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'stdin delete ref fails with wrong old value' '\n \techo \"delete $a $m~1\" >stdin &&\n \ttest_must_fail git update-ref --stdin <stdin 2>err &&\n-- \n2.39.2\n\n"},{"id":"472557","messageId":"xmqqpma09udd.fsf@gitster.g","threadId":"59290","inReplyTo":"20230223011530.47477-1-ronan@rjp.ie","subject":"Re: [PATCH] update-ref: add forward command to safely fast-forward refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-23T17:14:38Z","receivedAt":"2023-02-23T17:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronan Pigott <ronan@rjp.ie> writes:\n\n> forward is an update-ref command that behaves similarly to update, but\n> takes an additional argument, <ancestor>, and verifies that the new\n> value is a descendent of ancestor before updating. This is useful for\n> fast-forwarding prefetched refs.\n\nWhy is this necessary?\n\n * Do you expect that you may not know the ancestry relationship\n   between the <newvalue> and <ancestor> values when you need to\n   compute them, in order to formulate the 'forward' command?\n\n * Is there a case where the relationship between <newvalue> and\n   <ancestor> that was fast-forward when you formulated the\n   'forward' command changes by the time the 'forward' command gets\n   executed?\n\nI do not see the reason why this new command is needed, unless one\nor both of the above is what you are trying to address.\n\nFor example, existing \"delete SP <ref> SP <oldvalue>\" is to protect\nthe ref you want to delete, that used to have the oldvalue back when\nyou created the 'delete' command, from getting deleted when somebody\nelse changed it from the sidelines.  We can do\n\n\toldvalue=$(git rev-parse refs/to/be/deleted^{object})\n\techo delete refs/to/be/deleted $oldvalue | ...\n\nand let the command notice if somebody else changed\nrefs/to/be/deleted in between.  It is similar to the second one\nbetween the two I cited above, to make sure that your command does\nnot overwrite what somebody else did.\n\n\n\nIt may make some sense if the new <ancestor> thing is to replace the\n<oldvalue> thing, though.  That is, if there were a three-commit chain\n\n\tA---B---C\n\nwhere the ref you are trying to update currently points at A and you\nwant to update it to C.  You would observe that the current value is\nA, and formulate the command line:\n\n\tupdate ref/to/be/updated C A\n\nwith the current system, and updating the ref is allowed only when\nnobody touched the ref in the meantime.  It is _conceivable_ to say\nthat we are OK as long as the ref points at a decendant of A\n(instead of pointing exactly at A), and is an ancestor of C (the\nvalue we are updating to), with\n\n\tfast-forward ref/to/be/fast-forwarded C A\n\nThen somebody else _could_ update the ref to B from the sideline,\nbut we notice that it is a descendant of A and an ancestor of C, and\nwe are still allowed to update it to C.\n\nEven in that case, I am not sure how useful it would be, but at\nleast that use case I can see why it may make sense.\n\nIf you can write <newvalue> and <ancestor> on the command line, you\ncertainly should be able to compute \"git merge-base\".  And because\nthe commits are immutable, it won't change in the middle.\n\nSo, I am not very impressed.  Unless I am missing something, this\ndoes not seem to be adding anything we cannot already do.\n\nI wonder if\n\n    git fetch . \"+refs/prefetch/remotes/origin/*:refs/remotes/origin/*\"\n\n(with or without the '+' prefix, depending) what you are really\ngoing after, though.\n"},{"id":"472559","messageId":"52a5d6ae0b5d668b1c5e9c05a51701b1@rjp.ie","threadId":"59290","inReplyTo":"xmqqpma09udd.fsf@gitster.g","subject":"Re: [PATCH] update-ref: add forward command to safely fast-forward refs","fromName":"","fromEmail":"ronan@rjp.ie","sentAt":"2023-02-23T18:01:54Z","receivedAt":"2023-02-23T18:02:00Z","isPatch":true,"sender":{"key":"ronan@rjp.ie","avatar":"https://avatars.githubusercontent.com/u/12577529?v=4"},"body":"> Unless I am missing something, this\n> does not seem to be adding anything we cannot already do.\n\nCorrect. In my case it replaces:\n\nMERGE_BASE=$(git merge-base refs/remotes/origin/master refs/prefetch/remotes/origin/master)\nupdate refs/remotes/origin/master refs/prefetch/remotes/origin/master $MERGE_BASE\n\nI just thought the \"forward\" form was a little more convenient since it could output it all in one for-each-ref invocation. I mean, I could always open .git/refs/* in vim but that doesn't mean I want to...\n\nOriginally I left it as a three argument command but figured it might as well be possible to update to the descendent of any given ref, so I separated <ancestor> from <refname>.\n \n> I wonder if\n> \n> git fetch . \"+refs/prefetch/remotes/origin/*:refs/remotes/origin/*\"\n> \n> (with or without the '+' prefix, depending) what you are really\n> going after, though.\n\nOh, that's perfect! I didn't consider that it was possible to \"fetch\" from the local repo. I think the refspec 'refs/prefetch/remotes/origin/*:refs/remotes/origin/*' does what I want — fast-forwarding my remote tracking branches to their latest prefetches.\n\nIn that case, if you don't feel the forward feature is useful feel free to drop it.\n\nThanks,\nRonan\n"}]}