On Tue, Apr 04 2023, Felipe Contreras wrote:
> 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@xxxxxxxxx>
> ---
> 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".
> +{
> + 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.
> +
> + 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?
> + }
> +
> + 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...
> +
> 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).
> + }
> + 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.
> +
> 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.
> +
> #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"?
