Re: [PATCH v7 2/4] fetch: infer branches to fetch from a refmap-only remote

[Junio C Hamano <gitster@pobox.com>]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Configuring remote.<name>.refmap without remote.<name>.fetch used to
> make a refspec-less "git fetch <name>" fail with "--refmap option is
> only meaningful with command-line refspec(s)", since a refmap only
> says where to put fetched refs, not what to fetch.
>
> Make that case infer what to fetch: the local branches whose
> @{upstream} is already on that remote. This lets a remote be
> configured to fetch only the branches actually in use, without
> listing them by hand in remote.<name>.fetch, and without needing to
> touch the command line every time.

Nicely explained what we want out of this new feature.

>  remote.<name>.refmap::
>  	The default value of the `--refmap` option for linkgit:git-fetch[1].
>  	Used to map remote refs being fetched to remote-tracking refs to
> -	store. See the `--refmap` entry in linkgit:git-fetch[1].
> +	store. If `remote.<name>.fetch` is not set either, a refspec-less
> +	fetch infers what to fetch from local branches built on this
> +	remote, instead of fetching every branch it has. See the
> +	`--refmap` entry in linkgit:git-fetch[1].

OK.

> diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
> index c2101a7b39..75899d91cc 100644
> --- a/Documentation/fetch-options.adoc
> +++ b/Documentation/fetch-options.adoc
> @@ -245,8 +245,10 @@ endif::git-pull[]
>  	command-line arguments. See section on "Configured Remote-tracking
>  	Branches" for details.
>  +
> -`remote.<name>.refmap` provides the default value for this option, the
> -same way `remote.<name>.fetch` provides the default refspecs to fetch.
> +When a refmap is active (from `--refmap` or `remote.<name>.refmap`) but
> +there is nothing to fetch, neither on the command line nor from
> +`remote.<name>.fetch`, Git infers what to fetch from the local branches
> +whose `@{upstream}` is on that remote.

To say "infers what to fetch" without explicitly saying how the
inference is made is not sufficient in a technical manual.  Even
a reading like "up to three branches that our local branches
have as '@{u}'" is possible, if not very probable.

Perhaps something like this instead:

    ... but nothing to fetch is specified on the command line,
    branches from the remote that are used as '@{upstream}' of
    our local branches are fetched.


[...]

> @@ -1960,15 +1982,30 @@ static int do_fetch(struct transport *transport,
>  		refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes);
>  	} else {
>  		struct branch *branch = branch_get(NULL);
> -
> -		if (transport->remote->fetch.nr) {
> +		int tracks_this_remote = branch && branch_has_merge_config(branch) &&
> +			!strcmp(branch->remote_name, transport->remote->name);
> +		struct refspec *effective_refmap = refmap.nr ? &refmap :
> +			&transport->remote->refmap;
> +		int inferred_branches = !transport->remote->fetch.nr &&
> +			effective_refmap->nr;
> +
> +		if (inferred_branches) {
> +			struct string_list tracked = STRING_LIST_INIT_DUP;
> +			struct string_list_item *item;
> +
> +			branches_tracking_remote(transport->remote, &tracked);
> +			for_each_string_list_item(item, &tracked)
> +				strvec_push(&transport_ls_refs_options.ref_prefixes,
> +					    item->string);
> +			string_list_clear(&tracked, 0);
> +		} else if (transport->remote->fetch.nr) {
>  			refspec_ref_prefixes(&transport->remote->fetch,
>  					     &transport_ls_refs_options.ref_prefixes);
> -			if (follow_remote_head != FOLLOW_REMOTE_NEVER)
> -				do_set_head = 1;
>  		}
> -		if (branch && branch_has_merge_config(branch) &&
> -		    !strcmp(branch->remote_name, transport->remote->name)) {
> +		if ((transport->remote->fetch.nr || inferred_branches) &&
> +		    follow_remote_head != FOLLOW_REMOTE_NEVER)
> +			do_set_head = 1;
> +		if (tracks_this_remote) {
>  			int i;
>  			for (i = 0; i < branch->merge_nr; i++) {
>  				strvec_push(&transport_ls_refs_options.ref_prefixes,


The unified diff is a bit messy to compare the before-and-after
behaviour, so let's see what the preimage said first.

>  		struct branch *branch = branch_get(NULL);
> -
> -		if (transport->remote->fetch.nr) {
>  			refspec_ref_prefixes(&transport->remote->fetch,
>  					     &transport_ls_refs_options.ref_prefixes);
> -			if (follow_remote_head != FOLLOW_REMOTE_NEVER)
> -				do_set_head = 1;
>  		}
> -		if (branch && branch_has_merge_config(branch) &&
> -		    !strcmp(branch->remote_name, transport->remote->name)) {
>  			int i;
>  			for (i = 0; i < branch->merge_nr; i++) {
>  				strvec_push(&transport_ls_refs_options.ref_prefixes,

So, when !rs->nr (i.e., nothing given on the command line to be fetched)
and there is no fetch refspec, we checked the current branch and if
it has merge config to merge from branches at the remote, we
automatically fetched them.  This is the world order before this
"infer with @{u} and refmap" work, and we should behave the same way
when remote.*.refmap is not set.

Let's see what the postimage says.

>  		struct branch *branch = branch_get(NULL);
> +		int tracks_this_remote = branch && branch_has_merge_config(branch) &&
> +			!strcmp(branch->remote_name, transport->remote->name);

This is the "does the current branch pull branches from the remote
we are working with right now?" condition we saw in the original.

> +		struct refspec *effective_refmap = refmap.nr ? &refmap :
> +			&transport->remote->refmap;

It is a bit annoying that refmap is a file scope static variable but
here we say "The value of the --refmap option from the command line,
or the value remote.*.refmap otherwise".

> +		int inferred_branches = !transport->remote->fetch.nr &&
> +			effective_refmap->nr;

This variable tells the code that it must infer branches, but named
as if it were a list of branches that were inferred.  "When remote.*.fetch
does not exist and we have the refmap to use for inferring".

> +		if (inferred_branches) {
> +			struct string_list tracked = STRING_LIST_INIT_DUP;
> +			struct string_list_item *item;
> +
> +			branches_tracking_remote(transport->remote, &tracked);
> +			for_each_string_list_item(item, &tracked)
> +				strvec_push(&transport_ls_refs_options.ref_prefixes,
> +					    item->string);
> +			string_list_clear(&tracked, 0);
> +		} else if (transport->remote->fetch.nr) {
>  			refspec_ref_prefixes(&transport->remote->fetch,
>  					     &transport_ls_refs_options.ref_prefixes);
>  		}

It would have been much easier to follow if the existing code came
first to make it clear that the new code is an add-on.  After all,
when transport->remote->fetch.nr is true, inferred_branches is never
true.

> +		if ((transport->remote->fetch.nr || inferred_branches) &&
> +		    follow_remote_head != FOLLOW_REMOTE_NEVER)
> +			do_set_head = 1;
> +		if (tracks_this_remote) {
>  			int i;
>  			for (i = 0; i < branch->merge_nr; i++) {
>  				strvec_push(&transport_ls_refs_options.ref_prefixes,

How does tracks_this_remote and inferred_branches interact?  Doesn't
the old code that grabs necessary remote-tracking branches for the
current branch add the same branch from the remote?  Doesn't @{u}
for the current branch added twice on the list of branches to fetch?

> @@ -2009,6 +2046,7 @@ static int do_fetch(struct transport *transport,
>  
>  	ref_map = get_ref_map(transport->remote, remote_refs, rs,
>  			      tags, &autotags);
> +
>  	if (!update_head_ok)
>  		check_not_current_branch(ref_map);

Useless patch noise.

> diff --git a/remote.c b/remote.c
> index 99a086ea5a..c26312beea 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1884,6 +1884,35 @@ int branch_merge_matches(struct branch *branch,
>  	return refname_match(branch->merge[i]->src, refname);
>  }
>  
> +struct branches_tracking_remote_cb_data {
> +	struct remote *remote;
> +	struct string_list *tracked;
> +};
> +
> +static int add_if_tracking_remote(const struct reference *ref, void *cb_data)
> +{
> +	struct branches_tracking_remote_cb_data *data = cb_data;
> +	struct branch *branch;
> +
> +	branch = branch_get(ref->name);

I know branch_get() is defined here and allows implicit use of
the_repository, but can't we pass "struct repository *" around in
cb_data so that we can use repo_branch_get() here?

> +	if (!branch_has_merge_config(branch) ||
> +	    strcmp(branch->remote_name, data->remote->name))
> +		return 0;
> +
> +	for (int i = 0; i < branch->merge_nr; i++)
> +		string_list_insert(data->tracked, branch->merge[i]->src);
> +
> +	return 0;
> +}

This is more or less identical to the "if current branch integrates
with branches from the remote, then fetch them" code we saw earlier
in the builtin/fetch.c:do_fetch() above.  I notice that its return
value is meaningless, as it always returns 0.

Stepping back a bit, because your new logic would become superset of
what we already have to support the current branch when refmap is
used, would it make sense to restructure the code change to
do_fetch() more like this:

	if (rs->nr) {
		... use command line refspec ...
	} else if (transport->remote->fetch.nr) {
		... use remote.*.fetch refspec ...
	} else if (effective_refmap->nr) {
		... your new logic ...
	} else {
		struct string_list list = STRING_LIST_INIT;
		collect_upstream_from_remote(&list, remote, NULL);
		for_each_string_list_item(item, &list)
			strvec_push(&transport_ls_refs_options.ref_prefixes,
					item->string);
	}
			
where collect_upstream_from_remote() performs the bulk of what
add_if_tracking_remote() does, which means add_if_tracking_remote()
becomes

	static int add_if_tracking_remote(...)
	{
		struct branches_tracking_remote_cb_data *data = cb_data;

		collect_upstream_from_remote(data->tracked, data->remote, ref->name);
	}

This will mean we will have a very small preliminary patch to
introduce collect_upstream_from_remote() function in remote.c and
update the "help current branch by fetching what are merged into it"
code in do_fetch() to use it, which will have the above ontlined
if/else if/ cascade except for your new refmap code.  On top, this
step will insert a single "else if" block to add your new logic to
do_fetch().

Doesn't it make the series (and more importantly, the resulting
code) much easier to understand?

> +void branches_tracking_remote(struct remote *remote, struct string_list *tracked)
> +{
> +	struct branches_tracking_remote_cb_data data = { remote, tracked };
> +
> +	refs_for_each_branch_ref(get_main_ref_store(the_repository),
> +				  add_if_tracking_remote, &data);
> +}

This also hardcodes the_repository, but shouldn't this function take
"struct repository *" pointer (and shove it in data structure to
pass it down)?

By the way, when merged to 'seen', it seems to have some
interactions with other topics and makes t5505 and t5586 fail.  I
didn't have time to dig down to the cause.  Can you perhaps help
finding the cause when I push the integration result out early this
afternoon?

Thanks.