git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/2] Add fetch.updateHead option

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Apr 5, 2023, 09:16 UTC
Message-ID
<230405.86fs9evfte.gmgdl@evledraar.gmail.com>
In-Reply-To
<20230405012742.2452208-2-felipe.contreras@gmail.com>
On Tue, Apr 04 2023, Felipe Contreras wrote:
Show 73 quoted lines
> Users might change the behavior when running "git fetch" so that the
> remote's HEAD symbolic ref is updated at certain point.
>
> For example after running "git remote add" the remote HEAD is not
> set like it is with "git clone".
>
> Setting "fetch.updatehead = missing" would probably be a sensible
> default that everyone would want, but for now the default behavior is to
> never update HEAD, so there shouldn't be any functional changes.
>
> For the next major version of Git, we might want to change this default.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/config/fetch.txt  |  4 +++
>  Documentation/config/remote.txt |  3 ++
>  builtin/fetch.c                 | 64 ++++++++++++++++++++++++++++++++-
>  remote.c                        | 21 +++++++++++
>  remote.h                        | 11 ++++++
>  t/t5510-fetch.sh                | 31 ++++++++++++++++
>  6 files changed, 133 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
> index 568f0f75b3..dc147ffb35 100644
> --- a/Documentation/config/fetch.txt
> +++ b/Documentation/config/fetch.txt
> @@ -120,3 +120,7 @@ fetch.bundleCreationToken::
>  The creation token values are chosen by the provider serving the specific
>  bundle URI. If you modify the URI at `fetch.bundleURI`, then be sure to
>  remove the value for the `fetch.bundleCreationToken` value before fetching.
> +
> +fetch.updateHead::
> +	Defines when to update the remote HEAD symbolic ref. Values are 'never',
> +	'missing' (update only when HEAD is missing), and 'always'.
> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt
> index 0678b4bcfe..9d739d2ed4 100644
> --- a/Documentation/config/remote.txt
> +++ b/Documentation/config/remote.txt
> @@ -86,3 +86,6 @@ remote.<name>.partialclonefilter::
>  	Changing or clearing this value will only affect fetches for new commits.
>  	To fetch associated objects for commits already present in the local object
>  	database, use the `--refetch` option of linkgit:git-fetch[1].
> +
> +remote.<name>.updateHead::
> +	Defines when to update the remote HEAD symbolic ref. See `fetch.updateHead`.
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 7221e57f35..7e93a1aa46 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -59,6 +59,8 @@ static int fetch_prune_tags_config = -1; /* unspecified */
>  static int prune_tags = -1; /* unspecified */
>  #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */
>  
> +static int fetch_update_head = FETCH_UPDATE_HEAD_DEFAULT;
> +
>  static int all, append, dry_run, force, keep, multiple, update_head_ok;
>  static int write_fetch_head = 1;
>  static int verbosity, deepen_relative, set_upstream, refetch;
> @@ -129,6 +131,9 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>  		return 0;
>  	}
>  
> +	if (!strcmp(k, "fetch.updatehead"))
> +		return parse_update_head(&fetch_update_head, k, v);
> +
>  	return git_default_config(k, v, cb);
>  }
>  
> @@ -1579,6 +1584,47 @@ static int backfill_tags(struct transport *transport,
>  	return retcode;
>  }
>  
> +static void update_head(int config, const struct ref *head, const struct remote *remote)
Here you pass a "const struct remote".
Show 10 quoted lines
> +{
> +	char *ref, *target;
> +	const char *r;
> +	int flags;
> +
> +	if (!head || !head->symref || !remote)
> +		return;
> +
> +	ref = apply_refspecs((struct refspec *)&remote->fetch, "refs/heads/HEAD");
> +	target = apply_refspecs((struct refspec *)&remote->fetch, head->symref);

But here we end up with this cast, as it's not const after all, we're modifying it.

I think this sort of thing makes the code harder to read & reason about, and adds cast verbosity.

If you want to clearly communicate that the "remote->name" and "remote->mirror" you're using are "const" I think a better way to do this is to pass those as explicit parameters to this new static helper function, and then just pass a "struct refspec *fetch_rs" directly.

Show 17 quoted lines
> +
> +	if (!ref || !target) {
> +		warning(_("could not update remote head"));
> +		return;
> +	}
> +
> +	r = resolve_ref_unsafe(ref, 0, NULL, &flags);
> +
> +	if (r) {
> +		if (config == FETCH_UPDATE_HEAD_MISSING) {
> +			if (flags & REF_ISSYMREF)
> +				/* already present */
> +				return;
> +		} else if (config == FETCH_UPDATE_HEAD_ALWAYS) {
> +			if (!strcmp(r, target))
> +				/* already up-to-date */
> +				return;

I think you should name the "enum" you're adding below, the one that contains the new "FETCH_UPDATE_HEAD_DEFAULT".

Then this could be a "switch", and the compiler could check the arguments, i.e. you could pass an enum type instead of an "int".

> +		} else
{} missing, if you keep this, but...
> +			/* should never happen */
> +			return;

...so, here we're not checking some enum values, but presumably other things check this, I haven't checked.

But for a "should never happen", should we make this a "BUG()", or is it user-controlled?

Show 50 quoted lines
> +	}
> +
> +	if (!create_symref(ref, target, "remote update head")) {
> +		if (verbosity >= 0)
> +			printf(_("Updated remote '%s' HEAD\n"), remote->name);
> +	} else {
> +		warning(_("could not update remote head"));
> +	}
> +}
> +
>  static int do_fetch(struct transport *transport,
>  		    struct refspec *rs)
>  {
> @@ -1592,6 +1638,7 @@ static int do_fetch(struct transport *transport,
>  	int must_list_refs = 1;
>  	struct fetch_head fetch_head = { 0 };
>  	struct strbuf err = STRBUF_INIT;
> +	int need_update_head = 0, update_head_config = 0;
>  
>  	if (tags == TAGS_DEFAULT) {
>  		if (transport->remote->fetch_tags == 2)
> @@ -1626,9 +1673,21 @@ static int do_fetch(struct transport *transport,
>  	} else {
>  		struct branch *branch = branch_get(NULL);
>  
> -		if (transport->remote->fetch.nr)
> +		if (transport->remote->fetch.nr) {
> +
> +			if (transport->remote->update_head)
> +				update_head_config = transport->remote->update_head;
> +			else
> +				update_head_config = fetch_update_head;
> +
> +			need_update_head = update_head_config && update_head_config != FETCH_UPDATE_HEAD_NEVER;
> +
> +			if (need_update_head)
> +				strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
>  			refspec_ref_prefixes(&transport->remote->fetch,
>  					     &transport_ls_refs_options.ref_prefixes);
> +		}
> +
>  		if (branch_has_merge_config(branch) &&
>  		    !strcmp(branch->remote_name, transport->remote->name)) {
>  			int i;
> @@ -1737,6 +1796,9 @@ static int do_fetch(struct transport *transport,
>  
>  	commit_fetch_head(&fetch_head);
>  
> +	if (need_update_head)
> +		update_head(update_head_config, find_ref_by_name(remote_refs, "HEAD"), transport->remote);
Some overly long lines here...
Show 24 quoted lines
> +
>  	if (set_upstream) {
>  		struct branch *branch = branch_get("HEAD");
>  		struct ref *rm;
> diff --git a/remote.c b/remote.c
> index 641b083d90..5f3a9aa53e 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -344,6 +344,25 @@ static void read_branches_file(struct remote_state *remote_state,
>  	remote->fetch_tags = 1; /* always auto-follow */
>  }
>  
> +int parse_update_head(int *r, const char *var, const char *value)
> +{
> +	if (!r)
> +		return -1;
> +	else if (!value)
> +		return config_error_nonbool(var);
> +	else if (!strcmp(value, "never"))
> +		*r = FETCH_UPDATE_HEAD_NEVER;
> +	else if (!strcmp(value, "missing"))
> +		*r = FETCH_UPDATE_HEAD_MISSING;
> +	else if (!strcmp(value, "always"))
> +		*r = FETCH_UPDATE_HEAD_ALWAYS;

Ditto, this could really benefit from an enum type, instead of the bare "int".

> +	else {
> +		error(_("malformed value for %s: %s"), var, value);
> +		return error(_("must be one of never, missing, or always"));

Shouldn't we use git_die_config() or similar here, to get the line number etc., or do we get that somehow (I can't recall).

Show 26 quoted lines
> +	}
> +	return 0;
> +}
> +
>  static int handle_config(const char *key, const char *value, void *cb)
>  {
>  	const char *name;
> @@ -473,6 +492,8 @@ static int handle_config(const char *key, const char *value, void *cb)
>  					 key, value);
>  	} else if (!strcmp(subkey, "vcs")) {
>  		return git_config_string(&remote->foreign_vcs, key, value);
> +	} else if (!strcmp(subkey, "updatehead")) {
> +		return parse_update_head(&remote->update_head, key, value);
>  	}
>  	return 0;
>  }
> diff --git a/remote.h b/remote.h
> index 73638cefeb..9dce42d65d 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -22,6 +22,13 @@ enum {
>  	REMOTE_BRANCHES
>  };
>  
> +enum {
> +	FETCH_UPDATE_HEAD_DEFAULT = 0,

We tend to only init these to 0 when the default being 0 matters, i.e. we use it as a boolean, but is that the case here?

> +	FETCH_UPDATE_HEAD_NEVER,
> +	FETCH_UPDATE_HEAD_MISSING,
> +	FETCH_UPDATE_HEAD_ALWAYS,
> +};
I.e. let's name this.
Show 18 quoted lines
> +
>  struct rewrite {
>  	const char *base;
>  	size_t baselen;
> @@ -97,6 +104,8 @@ struct remote {
>  	int prune;
>  	int prune_tags;
>  
> +	int update_head;
> +
>  	/**
>  	 * The configured helper programs to run on the remote side, for
>  	 * Git-native protocols.
> @@ -449,4 +458,6 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
>  char *relative_url(const char *remote_url, const char *url,
>  		   const char *up_path);
>  
> +int parse_update_head(int *r, const char *var, const char *value);

For new functions and/or enums, having some brief API docs (using the "/** ... */" syntax) would be better.

Show 15 quoted lines
> +
>  #endif
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index dc44da9c79..dbeb2928ae 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -814,6 +814,37 @@ test_expect_success 'fetch from multiple configured URLs in single remote' '
>  	git fetch multipleurls
>  '
>  
> +test_cmp_symbolic_ref () {
> +	git symbolic-ref "$1" >actual &&
> +	echo "$2" >expected &&
> +	test_cmp expected actual
> +}

Sort of an aside, but this seems to be the Nth use of this pattern in the test suite, e.g. t1401-symbolic-ref.sh repeatedly hardcodes the same.

I wonder if a prep commit to stick this in test-lib-functions.sh would be in order, or maybe a "--symbolic" argument to "test_cmp_rev"?

Previous: Felipe ContrerasNext: Patrick Steinhardt
Message 3 of 9 in “Add fetch.updateHead option”
  1. 0/2 Add fetch.updateHead optionFelipe Contreras, Apr 5, 2023
  2. 1/2 Add fetch.updateHead optionFelipe Contreras, Apr 5, 2023
  3. Ævar Arnfjörð BjarmasonApr 5, 2023
  4. Patrick SteinhardtApr 5, 2023
  5. Felipe ContrerasApr 5, 2023
  6. Ævar Arnfjörð BjarmasonApr 6, 2023
  7. Felipe ContrerasApr 7, 2023
  8. Ævar Arnfjörð BjarmasonApr 5, 2023
  9. 2/2 fetch: add support for HEAD update on mirrorsFelipe Contreras, Apr 5, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.