[RFC PATCH 0/2] set remote/HEAD with fetch

[RFC PATCH 0/2] set remote/HEAD with fetch

[Bence Ferdinandy]

Hi,

these two patches attempt to solve the issue raised in
https://lore.kernel.org/git/D3HBD7C1FR14.74FL1Q1S9UCB@ferdinandy.com/

As a quick summary: `clone` sets `refs/remotes/[remote]/HEAD` while going init
-> remote add -> fetch does not, one has to manually run `remote set-head -a
[remote]`.

The first patch adds a `--set-head` flag to `fetch` and `remote update` which
runs `remote set-head -a` for us. Unfortunately, with the current behaviour of
set-head this will always print a message, even though a no-op fetch doesn't
print anything, and I think this is also confusing for people who do not care
about remote/HEAD, so the second patch removes the print if `set-head -a` is
a no-op (and actually makes it into a no-op, instead of just idempotent).

Another way could of course be duplicating some of the code from remote
set-head in fetch.c instead of calling directly, but it didn't look like an
anti-pattern in the code-base and it felt the best way to insure identical
behaviour between a `git fetch --all --set-head` and a 
`git fetch --all && git remote | xargs -i git remote set-head -a {}`.

What is missing for sure is:
- documentation
- tests (if needed)
- settings

For settings, my idea would be a fetch/remote.set_head that could take three values:
    * never
    * missing: run it only if the ref is missing, this setting would basically
      allow replicating the result of a clone
    * always (with the other patch, this would still be a no-op if it didn't change)

This would probably also require a --no-set-head flag, to disable an
always/missing setting. A --missing-set-head or something of the like also may
or may not make sense. Alternatively, only two behaviours might be enough
(missing and always) since clone already sort of does this.

I'm not sure if the general approach is fine or not, nor am I sure the code
itself is any good, but it "works on my computer" :) I'm also hoping that
I managed to read all the relevant parts for sending a patch.

Feedback would be highly appreciated!

Thanks,
Bence


Bence Ferdinandy (2):
  fetch: set-head with --set-head option
  set-head: do not update if there is no change

 builtin/fetch.c  | 29 ++++++++++++++++++++++++-----
 builtin/remote.c | 15 +++++++++++----
 2 files changed, 35 insertions(+), 9 deletions(-)

-- 
2.46.0

[RFC PATCH 1/2] fetch: set-head with --set-head option

[Bence Ferdinandy]

When cloning a repository refs/remotes/origin/HEAD is set automatically.
In contrast, when using init, remote add and fetch to set a remote, one
needs to call remote set-head --auto to achieve the same result.

Add a --set-head option to git fetch to automatically set heads on
remotes.

Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
---
 builtin/fetch.c  | 29 ++++++++++++++++++++++++-----
 builtin/remote.c |  5 +++++
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b2b5aee5bf..6392314c6a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1961,8 +1961,19 @@ static int fetch_finished(int result, struct strbuf *out,
 	return 0;
 }
 
-static int fetch_multiple(struct string_list *list, int max_children,
-			  const struct fetch_config *config)
+static int run_set_head(const char *name)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	strvec_push(&cmd.args, "remote");
+	strvec_push(&cmd.args, "set-head");
+	strvec_push(&cmd.args, "--auto");
+	strvec_push(&cmd.args, name);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
+}
+
+static int fetch_multiple(struct string_list *list, int max_children, int set_head,
+			const struct fetch_config *config)
 {
 	int i, result = 0;
 	struct strvec argv = STRVEC_INIT;
@@ -2014,6 +2025,8 @@ static int fetch_multiple(struct string_list *list, int max_children,
 				error(_("could not fetch %s"), name);
 				result = 1;
 			}
+			if (set_head && run_set_head(name))
+				result = 1;
 		}
 
 	strvec_clear(&argv);
@@ -2062,7 +2075,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)
 }
 
 static int fetch_one(struct remote *remote, int argc, const char **argv,
-		     int prune_tags_ok, int use_stdin_refspecs,
+		     int prune_tags_ok, int set_head, int use_stdin_refspecs,
 		     const struct fetch_config *config)
 {
 	struct refspec rs = REFSPEC_INIT_FETCH;
@@ -2135,9 +2148,12 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
 	refspec_clear(&rs);
 	transport_disconnect(gtransport);
 	gtransport = NULL;
+	if (set_head && run_set_head(remote -> name))
+		exit_code = 1;
 	return exit_code;
 }
 
+
 int cmd_fetch(int argc, const char **argv, const char *prefix)
 {
 	struct fetch_config config = {
@@ -2154,6 +2170,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	struct string_list list = STRING_LIST_INIT_DUP;
 	struct remote *remote = NULL;
 	int all = -1, multiple = 0;
+	int set_head = 0;
 	int result = 0;
 	int prune_tags_ok = 1;
 	int enable_auto_gc = 1;
@@ -2171,6 +2188,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		OPT__VERBOSITY(&verbosity),
 		OPT_BOOL(0, "all", &all,
 			 N_("fetch from all remotes")),
+		OPT_BOOL(0, "set-head", &set_head,
+			 N_("auto set remote HEAD")),
 		OPT_BOOL(0, "set-upstream", &set_upstream,
 			 N_("set upstream for git pull/fetch")),
 		OPT_BOOL('a', "append", &append,
@@ -2436,7 +2455,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			trace2_region_leave("fetch", "setup-partial", the_repository);
 		}
 		trace2_region_enter("fetch", "fetch-one", the_repository);
-		result = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs,
+		result = fetch_one(remote, argc, argv, prune_tags_ok, set_head, stdin_refspecs,
 				   &config);
 		trace2_region_leave("fetch", "fetch-one", the_repository);
 	} else {
@@ -2459,7 +2478,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 
 		/* TODO should this also die if we have a previous partial-clone? */
 		trace2_region_enter("fetch", "fetch-multiple", the_repository);
-		result = fetch_multiple(&list, max_children, &config);
+		result = fetch_multiple(&list, max_children, set_head, &config);
 		trace2_region_leave("fetch", "fetch-multiple", the_repository);
 	}
 
diff --git a/builtin/remote.c b/builtin/remote.c
index 0acc547d69..35c54dd103 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1536,9 +1536,12 @@ static int get_remote_default(const char *key, const char *value UNUSED,
 static int update(int argc, const char **argv, const char *prefix)
 {
 	int i, prune = -1;
+	int set_head = 0;
 	struct option options[] = {
 		OPT_BOOL('p', "prune", &prune,
 			 N_("prune remotes after fetching")),
+		OPT_BOOL(0, "set-head", &set_head,
+			 N_("auto set remote HEAD")),
 		OPT_END()
 	};
 	struct child_process cmd = CHILD_PROCESS_INIT;
@@ -1552,6 +1555,8 @@ static int update(int argc, const char **argv, const char *prefix)
 
 	if (prune != -1)
 		strvec_push(&cmd.args, prune ? "--prune" : "--no-prune");
+	if (set_head)
+		strvec_push(&cmd.args, "--set-head");
 	if (verbose)
 		strvec_push(&cmd.args, "-v");
 	strvec_push(&cmd.args, "--multiple");
-- 
2.46.0

Re: [RFC PATCH 1/2] fetch: set-head with --set-head option

[Jeff King]

On Tue, Sep 10, 2024 at 10:24:58PM +0200, Bence Ferdinandy wrote:

> When cloning a repository refs/remotes/origin/HEAD is set automatically.
> In contrast, when using init, remote add and fetch to set a remote, one
> needs to call remote set-head --auto to achieve the same result.

Yes, I think this is a good goal, but...

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b2b5aee5bf..6392314c6a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1961,8 +1961,19 @@ static int fetch_finished(int result, struct strbuf *out,
>  	return 0;
>  }
>  
> -static int fetch_multiple(struct string_list *list, int max_children,
> -			  const struct fetch_config *config)
> +static int run_set_head(const char *name)
> +{
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> +	strvec_push(&cmd.args, "remote");
> +	strvec_push(&cmd.args, "set-head");
> +	strvec_push(&cmd.args, "--auto");
> +	strvec_push(&cmd.args, name);
> +	cmd.git_cmd = 1;
> +	return run_command(&cmd);
> +}

...this is just calling "git remote" to do the real work. Which means
that git-remote is going to make its own separate connection to the
server (so slow, but may also require the user to reauthenticate, etc).

I think the intent of your patch 2 is that we'd only invoke this when we
saw a change, which mitigates the impact, but it still seems somewhat
hacky to me. We already have all of the information we need to do the
update inside fetch itself.

-Peff

Re: [RFC PATCH 1/2] fetch: set-head with --set-head option

[Bence Ferdinandy]

On Wed Sep 11, 2024 at 08:54, Jeff King <peff@peff.net> wrote:
> On Tue, Sep 10, 2024 at 10:24:58PM +0200, Bence Ferdinandy wrote:
>
> > When cloning a repository refs/remotes/origin/HEAD is set automatically.
> > In contrast, when using init, remote add and fetch to set a remote, one
> > needs to call remote set-head --auto to achieve the same result.
>
> Yes, I think this is a good goal, but...
>
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index b2b5aee5bf..6392314c6a 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -1961,8 +1961,19 @@ static int fetch_finished(int result, struct strbuf *out,
> >  	return 0;
> >  }
> >  
> > -static int fetch_multiple(struct string_list *list, int max_children,
> > -			  const struct fetch_config *config)
> > +static int run_set_head(const char *name)
> > +{
> > +	struct child_process cmd = CHILD_PROCESS_INIT;
> > +	strvec_push(&cmd.args, "remote");
> > +	strvec_push(&cmd.args, "set-head");
> > +	strvec_push(&cmd.args, "--auto");
> > +	strvec_push(&cmd.args, name);
> > +	cmd.git_cmd = 1;
> > +	return run_command(&cmd);
> > +}
>
> ...this is just calling "git remote" to do the real work. Which means
> that git-remote is going to make its own separate connection to the
> server (so slow, but may also require the user to reauthenticate, etc).

And indeed it does authenticate the user twice. I'll change this in a v3 (see
the discussion on v2, I royally messed up CC address on this one :) ).

>
> I think the intent of your patch 2 is that we'd only invoke this when we
> saw a change, which mitigates the impact, but it still seems somewhat
> hacky to me. We already have all of the information we need to do the
> update inside fetch itself.

It was more about not printing slightly misleading information, it did still
always try to get the new information with --auto. With the changes mentioned
in the other thread I'll also rework this a bit.

Best,
Bence

[RFC PATCH 2/2] set-head: do not update if there is no change

[Bence Ferdinandy]

Currently, even if there is no actual change to remote/HEAD calling
remote set-head will overwrite the appropriate file and if set to --auto
will also print a message saying "remote/HEAD set to branch", which
implies something was changed. In contrast, on a nil operation e.g. pull
will clearly state that nothing was done, while fetch will not output
anything.

Change the behaviour of remote set-head so that the reference is only
updated if it actually needs to change. Since set-head --auto is
essentially a fetch-like operation, align it's behaviour with fetch and
only print output if something was actually done.

Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
---
 builtin/remote.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 35c54dd103..e220e51b84 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1400,8 +1400,8 @@ static int show(int argc, const char **argv, const char *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 buf = STRBUF_INIT, buf2 = STRBUF_INIT;
+	int i, opt_a = 0, opt_d = 0, is_ref_changed = 0, result = 0;
+	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT;
 	char *head_name = NULL;
 
 	struct option options[] = {
@@ -1440,12 +1440,14 @@ static int set_head(int argc, const char **argv, const char *prefix)
 
 	if (head_name) {
 		strbuf_addf(&buf2, "refs/remotes/%s/%s", argv[0], head_name);
+		refs_read_symbolic_ref(get_main_ref_store(the_repository),buf.buf,&buf3);
+		is_ref_changed = strcmp(buf2.buf,buf3.buf);
 		/* make sure it's valid */
 		if (!refs_ref_exists(get_main_ref_store(the_repository), buf2.buf))
 			result |= error(_("Not a valid ref: %s"), buf2.buf);
-		else if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, "remote set-head"))
+		else if (is_ref_changed && refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, "remote set-head"))
 			result |= error(_("Could not setup %s"), buf.buf);
-		else if (opt_a)
+		else if (opt_a && is_ref_changed)
 			printf("%s/HEAD set to %s\n", argv[0], head_name);
 		free(head_name);
 	}
-- 
2.46.0