{"thread":{"id":"62095","subject":"[RFC PATCH 1/2] fetch: set-head with --set-head option","startedAt":"2024-09-10T20:37:11Z","lastAt":"2024-09-11T12:17:18Z","messageCount":5,"participants":["Bence Ferdinandy","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"502588","messageId":"20240910203129.2251090-2-bence@ferdinandy.com","threadId":"62095","inReplyTo":"20240910203129.2251090-1-bence@ferdinandy.com","subject":"[RFC PATCH 1/2] fetch: set-head with --set-head option","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-09-10T20:24:58Z","receivedAt":"2024-09-10T20:37:11Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"When cloning a repository refs/remotes/origin/HEAD is set automatically.\nIn contrast, when using init, remote add and fetch to set a remote, one\nneeds to call remote set-head --auto to achieve the same result.\n\nAdd a --set-head option to git fetch to automatically set heads on\nremotes.\n\nSigned-off-by: Bence Ferdinandy <bence@ferdinandy.com>\n---\n builtin/fetch.c  | 29 ++++++++++++++++++++++++-----\n builtin/remote.c |  5 +++++\n 2 files changed, 29 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b2b5aee5bf..6392314c6a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1961,8 +1961,19 @@ static int fetch_finished(int result, struct strbuf *out,\n \treturn 0;\n }\n \n-static int fetch_multiple(struct string_list *list, int max_children,\n-\t\t\t  const struct fetch_config *config)\n+static int run_set_head(const char *name)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstrvec_push(&cmd.args, \"remote\");\n+\tstrvec_push(&cmd.args, \"set-head\");\n+\tstrvec_push(&cmd.args, \"--auto\");\n+\tstrvec_push(&cmd.args, name);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n+}\n+\n+static int fetch_multiple(struct string_list *list, int max_children, int set_head,\n+\t\t\tconst struct fetch_config *config)\n {\n \tint i, result = 0;\n \tstruct strvec argv = STRVEC_INIT;\n@@ -2014,6 +2025,8 @@ static int fetch_multiple(struct string_list *list, int max_children,\n \t\t\t\terror(_(\"could not fetch %s\"), name);\n \t\t\t\tresult = 1;\n \t\t\t}\n+\t\t\tif (set_head && run_set_head(name))\n+\t\t\t\tresult = 1;\n \t\t}\n \n \tstrvec_clear(&argv);\n@@ -2062,7 +2075,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n }\n \n static int fetch_one(struct remote *remote, int argc, const char **argv,\n-\t\t     int prune_tags_ok, int use_stdin_refspecs,\n+\t\t     int prune_tags_ok, int set_head, int use_stdin_refspecs,\n \t\t     const struct fetch_config *config)\n {\n \tstruct refspec rs = REFSPEC_INIT_FETCH;\n@@ -2135,9 +2148,12 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n \trefspec_clear(&rs);\n \ttransport_disconnect(gtransport);\n \tgtransport = NULL;\n+\tif (set_head && run_set_head(remote -> name))\n+\t\texit_code = 1;\n \treturn exit_code;\n }\n \n+\n int cmd_fetch(int argc, const char **argv, const char *prefix)\n {\n \tstruct fetch_config config = {\n@@ -2154,6 +2170,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n \tstruct remote *remote = NULL;\n \tint all = -1, multiple = 0;\n+\tint set_head = 0;\n \tint result = 0;\n \tint prune_tags_ok = 1;\n \tint enable_auto_gc = 1;\n@@ -2171,6 +2188,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tOPT__VERBOSITY(&verbosity),\n \t\tOPT_BOOL(0, \"all\", &all,\n \t\t\t N_(\"fetch from all remotes\")),\n+\t\tOPT_BOOL(0, \"set-head\", &set_head,\n+\t\t\t N_(\"auto set remote HEAD\")),\n \t\tOPT_BOOL(0, \"set-upstream\", &set_upstream,\n \t\t\t N_(\"set upstream for git pull/fetch\")),\n \t\tOPT_BOOL('a', \"append\", &append,\n@@ -2436,7 +2455,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\ttrace2_region_leave(\"fetch\", \"setup-partial\", the_repository);\n \t\t}\n \t\ttrace2_region_enter(\"fetch\", \"fetch-one\", the_repository);\n-\t\tresult = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs,\n+\t\tresult = fetch_one(remote, argc, argv, prune_tags_ok, set_head, stdin_refspecs,\n \t\t\t\t   &config);\n \t\ttrace2_region_leave(\"fetch\", \"fetch-one\", the_repository);\n \t} else {\n@@ -2459,7 +2478,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \t\t/* TODO should this also die if we have a previous partial-clone? */\n \t\ttrace2_region_enter(\"fetch\", \"fetch-multiple\", the_repository);\n-\t\tresult = fetch_multiple(&list, max_children, &config);\n+\t\tresult = fetch_multiple(&list, max_children, set_head, &config);\n \t\ttrace2_region_leave(\"fetch\", \"fetch-multiple\", the_repository);\n \t}\n \ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 0acc547d69..35c54dd103 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1536,9 +1536,12 @@ static int get_remote_default(const char *key, const char *value UNUSED,\n static int update(int argc, const char **argv, const char *prefix)\n {\n \tint i, prune = -1;\n+\tint set_head = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL('p', \"prune\", &prune,\n \t\t\t N_(\"prune remotes after fetching\")),\n+\t\tOPT_BOOL(0, \"set-head\", &set_head,\n+\t\t\t N_(\"auto set remote HEAD\")),\n \t\tOPT_END()\n \t};\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n@@ -1552,6 +1555,8 @@ static int update(int argc, const char **argv, const char *prefix)\n \n \tif (prune != -1)\n \t\tstrvec_push(&cmd.args, prune ? \"--prune\" : \"--no-prune\");\n+\tif (set_head)\n+\t\tstrvec_push(&cmd.args, \"--set-head\");\n \tif (verbose)\n \t\tstrvec_push(&cmd.args, \"-v\");\n \tstrvec_push(&cmd.args, \"--multiple\");\n-- \n2.46.0\n\n"},{"id":"502589","messageId":"20240910203129.2251090-1-bence@ferdinandy.com","threadId":"62095","inReplyTo":null,"subject":"[RFC PATCH 0/2] set remote/HEAD with fetch","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-09-10T20:24:57Z","receivedAt":"2024-09-10T20:37:14Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"Hi,\n\nthese two patches attempt to solve the issue raised in\nhttps://lore.kernel.org/git/D3HBD7C1FR14.74FL1Q1S9UCB@ferdinandy.com/\n\nAs a quick summary: `clone` sets `refs/remotes/[remote]/HEAD` while going init\n-> remote add -> fetch does not, one has to manually run `remote set-head -a\n[remote]`.\n\nThe first patch adds a `--set-head` flag to `fetch` and `remote update` which\nruns `remote set-head -a` for us. Unfortunately, with the current behaviour of\nset-head this will always print a message, even though a no-op fetch doesn't\nprint anything, and I think this is also confusing for people who do not care\nabout remote/HEAD, so the second patch removes the print if `set-head -a` is\na no-op (and actually makes it into a no-op, instead of just idempotent).\n\nAnother way could of course be duplicating some of the code from remote\nset-head in fetch.c instead of calling directly, but it didn't look like an\nanti-pattern in the code-base and it felt the best way to insure identical\nbehaviour between a `git fetch --all --set-head` and a \n`git fetch --all && git remote | xargs -i git remote set-head -a {}`.\n\nWhat is missing for sure is:\n- documentation\n- tests (if needed)\n- settings\n\nFor settings, my idea would be a fetch/remote.set_head that could take three values:\n    * never\n    * missing: run it only if the ref is missing, this setting would basically\n      allow replicating the result of a clone\n    * always (with the other patch, this would still be a no-op if it didn't change)\n\nThis would probably also require a --no-set-head flag, to disable an\nalways/missing setting. A --missing-set-head or something of the like also may\nor may not make sense. Alternatively, only two behaviours might be enough\n(missing and always) since clone already sort of does this.\n\nI'm not sure if the general approach is fine or not, nor am I sure the code\nitself is any good, but it \"works on my computer\" :) I'm also hoping that\nI managed to read all the relevant parts for sending a patch.\n\nFeedback would be highly appreciated!\n\nThanks,\nBence\n\n\nBence Ferdinandy (2):\n  fetch: set-head with --set-head option\n  set-head: do not update if there is no change\n\n builtin/fetch.c  | 29 ++++++++++++++++++++++++-----\n builtin/remote.c | 15 +++++++++++----\n 2 files changed, 35 insertions(+), 9 deletions(-)\n\n-- \n2.46.0\n\n"},{"id":"502590","messageId":"20240910203129.2251090-3-bence@ferdinandy.com","threadId":"62095","inReplyTo":"20240910203129.2251090-1-bence@ferdinandy.com","subject":"[RFC PATCH 2/2] set-head: do not update if there is no change","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-09-10T20:24:59Z","receivedAt":"2024-09-10T20:37:16Z","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. In contrast, on a nil operation e.g. pull\nwill clearly state that nothing was done, while fetch will not output\nanything.\n\nChange the behaviour of remote set-head so that the reference is only\nupdated if it actually needs to change. Since set-head --auto is\nessentially a fetch-like operation, align it's behaviour with fetch and\nonly print output if something was actually done.\n\nSigned-off-by: Bence Ferdinandy <bence@ferdinandy.com>\n---\n builtin/remote.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 35c54dd103..e220e51b84 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,12 +1440,14 @@ 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)\n+\t\telse if (opt_a && is_ref_changed)\n \t\t\tprintf(\"%s/HEAD set to %s\\n\", argv[0], head_name);\n \t\tfree(head_name);\n \t}\n-- \n2.46.0\n\n"},{"id":"502609","messageId":"20240911065406.GC1538586@coredump.intra.peff.net","threadId":"62095","inReplyTo":"20240910203129.2251090-2-bence@ferdinandy.com","subject":"Re: [RFC PATCH 1/2] fetch: set-head with --set-head option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-11T06:54:06Z","receivedAt":"2024-09-11T06:54:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 10, 2024 at 10:24:58PM +0200, Bence Ferdinandy wrote:\n\n> When cloning a repository refs/remotes/origin/HEAD is set automatically.\n> In contrast, when using init, remote add and fetch to set a remote, one\n> needs to call remote set-head --auto to achieve the same result.\n\nYes, I think this is a good goal, but...\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b2b5aee5bf..6392314c6a 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1961,8 +1961,19 @@ static int fetch_finished(int result, struct strbuf *out,\n>  \treturn 0;\n>  }\n>  \n> -static int fetch_multiple(struct string_list *list, int max_children,\n> -\t\t\t  const struct fetch_config *config)\n> +static int run_set_head(const char *name)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\tstrvec_push(&cmd.args, \"remote\");\n> +\tstrvec_push(&cmd.args, \"set-head\");\n> +\tstrvec_push(&cmd.args, \"--auto\");\n> +\tstrvec_push(&cmd.args, name);\n> +\tcmd.git_cmd = 1;\n> +\treturn run_command(&cmd);\n> +}\n\n...this is just calling \"git remote\" to do the real work. Which means\nthat git-remote is going to make its own separate connection to the\nserver (so slow, but may also require the user to reauthenticate, etc).\n\nI think the intent of your patch 2 is that we'd only invoke this when we\nsaw a change, which mitigates the impact, but it still seems somewhat\nhacky to me. We already have all of the information we need to do the\nupdate inside fetch itself.\n\n-Peff\n"},{"id":"502615","messageId":"D43G52W1ZPWU.2HPFFSQEM3WGV@ferdinandy.com","threadId":"62095","inReplyTo":"20240911065406.GC1538586@coredump.intra.peff.net","subject":"Re: [RFC PATCH 1/2] fetch: set-head with --set-head option","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-09-11T12:16:39Z","receivedAt":"2024-09-11T12:17:18Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nOn Wed Sep 11, 2024 at 08:54, Jeff King <peff@peff.net> wrote:\n> On Tue, Sep 10, 2024 at 10:24:58PM +0200, Bence Ferdinandy wrote:\n>\n> > When cloning a repository refs/remotes/origin/HEAD is set automatically.\n> > In contrast, when using init, remote add and fetch to set a remote, one\n> > needs to call remote set-head --auto to achieve the same result.\n>\n> Yes, I think this is a good goal, but...\n>\n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index b2b5aee5bf..6392314c6a 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -1961,8 +1961,19 @@ static int fetch_finished(int result, struct strbuf *out,\n> >  \treturn 0;\n> >  }\n> >  \n> > -static int fetch_multiple(struct string_list *list, int max_children,\n> > -\t\t\t  const struct fetch_config *config)\n> > +static int run_set_head(const char *name)\n> > +{\n> > +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> > +\tstrvec_push(&cmd.args, \"remote\");\n> > +\tstrvec_push(&cmd.args, \"set-head\");\n> > +\tstrvec_push(&cmd.args, \"--auto\");\n> > +\tstrvec_push(&cmd.args, name);\n> > +\tcmd.git_cmd = 1;\n> > +\treturn run_command(&cmd);\n> > +}\n>\n> ...this is just calling \"git remote\" to do the real work. Which means\n> that git-remote is going to make its own separate connection to the\n> server (so slow, but may also require the user to reauthenticate, etc).\n\nAnd indeed it does authenticate the user twice. I'll change this in a v3 (see\nthe discussion on v2, I royally messed up CC address on this one :) ).\n\n>\n> I think the intent of your patch 2 is that we'd only invoke this when we\n> saw a change, which mitigates the impact, but it still seems somewhat\n> hacky to me. We already have all of the information we need to do the\n> update inside fetch itself.\n\nIt was more about not printing slightly misleading information, it did still\nalways try to get the new information with --auto. With the changes mentioned\nin the other thread I'll also rework this a bit.\n\nBest,\nBence\n"}]}