Re: [PATCH v5 1/2] branch: accept multiple upstream branches for tracking

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

Josh Steadmon <steadmon@google.com> writes:

> +static int install_branch_config_multiple_remotes(int flag, const char *local, const char *origin,
> +		struct string_list *remotes)

The line got overly long so perhaps cut the line after "*local,",
as "origin" and "remotes" conceptually are closer together.

What is in the string list?  Names of refs at the remote "origin",
instead of a single ref there?

>  {
>  	const char *shortname = NULL;
>  	struct strbuf key = STRBUF_INIT;
> -	int rebasing = should_setup_rebase(origin);
> -
> -	if (skip_prefix(remote, "refs/heads/", &shortname)
> -	    && !strcmp(local, shortname)
> -	    && !origin) {
> -		warning(_("Not setting branch %s as its own upstream."),
> -			local);

When 'origin' is NULL in the original caller, it means a local
tracking, and making sure we do not say "my 'master' branch builds
on top of itself" makes sense.

> -		return 0;
> -	}
> +	int i, rebasing = should_setup_rebase(origin);
> +
> +	if (remotes->nr < 1)
> +		BUG("must provide at least one remote for branch config");
> +
> +	if (!origin)
> +		for (i = 0; i < remotes->nr; i++)
> +			if (skip_prefix(remotes->items[i].string, "refs/heads/", &shortname)
> +			    && !strcmp(local, shortname)) {
> +				warning(_("Not setting branch %s as its own upstream."),
> +					local);
> +				return 0;

I am a bit surprised with this warning and early return before
inspecting the remainder of the list.  When 'origin' is NULL,
i.e. we are talking about the local building on top of another local
branch, if the function is called for the local branch 'main' with
'main' in the remotes list alone, we do want to issue the warning
and exit without doing anything (i.e. degenerating to the original
behaviour of taking a single string variable, when a string list
with a single element is given).  But if the remotes list has 'main'
and 'master', would we want to just "skip" the same one, but still
handle the other ones as if the "same" branch were not in the list?

> @@ -75,8 +80,17 @@ int install_branch_config(int flag, const char *local, const char *origin, const
>  
>  	strbuf_reset(&key);
>  	strbuf_addf(&key, "branch.%s.merge", local);
> -	if (git_config_set_gently(key.buf, remote) < 0)
> +	/*
> +	 * We want to overwrite any existing config with all the branches in
> +	 * "remotes". Override any existing config with the first branch, but if
> +	 * more than one is provided, use CONFIG_REGEX_NONE to preserve what
> +	 * we've written so far.
> +	 */
> +	if (git_config_set_gently(key.buf, remotes->items[0].string) < 0)
>  		goto out_err;
> +	for (i = 1; i < remotes->nr; i++)
> +		if (git_config_set_multivar_gently(key.buf, remotes->items[i].string, CONFIG_REGEX_NONE, 0) < 0)
> +			goto out_err;
>  
>  	if (rebasing) {
>  		strbuf_reset(&key);
> @@ -87,29 +101,62 @@ int install_branch_config(int flag, const char *local, const char *origin, const
>  	strbuf_release(&key);
>  
>  	if (flag & BRANCH_CONFIG_VERBOSE) {
> -		if (shortname) {
> -			if (origin)
> -				printf_ln(rebasing ?
> -					  _("Branch '%s' set up to track remote branch '%s' from '%s' by rebasing.") :
> -					  _("Branch '%s' set up to track remote branch '%s' from '%s'."),
> -					  local, shortname, origin);
> -			else
> -				printf_ln(rebasing ?
> -					  _("Branch '%s' set up to track local branch '%s' by rebasing.") :
> -					  _("Branch '%s' set up to track local branch '%s'."),
> -					  local, shortname);
> +		int plural = remotes->nr > 1;
> +		int all_shortnames = 1;
> +		const char *msg_fmt;
> +		struct strbuf ref_string = STRBUF_INIT;
> +
> +		for (i = 0; i < remotes->nr; i++)
> +			if (skip_prefix(remotes->items[i].string, "refs/heads/", &shortname)) {
> +				strbuf_addf(&ref_string, "'%s', ", shortname);
> +			} else {
> +				all_shortnames = 0;
> +				strbuf_addf(&ref_string, "'%s', ", remotes->items[i].string);

So, all_shortnames == true means everything was a local branch in
the 'origin' remote, and when it has a non-branch (like a tag),
all_shortnames becomes false?

> +			}
> +		/* The last two characters are an extraneous ", ", so trim those. */
> +		strbuf_setlen(&ref_string, ref_string.len - 2);

As you are starting from an empty ref_string, a more idiomatic way
to build concatenated string would be to prefix when you add a new
item, e.g.

	loop {
		if (ref_string already has items)
			ref_string.append(", ");
		ref_string.append(this_item);
	}

> +		if (all_shortnames && origin) {
> +			if (rebasing && plural)
> +				msg_fmt = "Branch '%s' set up to track remote branches %s from '%s' by rebasing.";

What does it mean to keep my 'topic' branch up-to-date by rebasing
on top of more than one remote sources?  By merging, I can sort-of
understand (i.e. creating an octopus), but would it make sense to
track more than one remote sources in general?  Is it common?

When the benefit is not clear, it might make more sense not to do
this when there are already multiple tracking sources defined for
the original; it might be a mistake that we may not want to spread
with the new option.

Of course, it is very possible that I am missing a perfectly valid
use case where having more than one makes good sense.  If so, please
do not take the above comments as an objection, but adding some
comments before the function to explain when having remote list with
more than one items makes sense and how such a setting can be used
to avoid future readers asking the same (stupid) question as I just
did.


> +			else if (rebasing && !plural)
> +				msg_fmt = "Branch '%s' set up to track remote branch %s from '%s' by rebasing.";
> +			else if (!rebasing && plural)
> +				msg_fmt = "Branch '%s' set up to track remote branches %s from '%s'.";
> +			else if (!rebasing && !plural)
> +				msg_fmt = "Branch '%s' set up to track remote branch %s from '%s'.";
> +
> +			printf_ln(_(msg_fmt), local, ref_string, origin);

I am not sure how well the "plural" thing works with i18n.  It may
suffice for the original in English to have only two choices between
one or more-than-one, but not all languages are English.  Counting
the actual number (I guess remotes->nr is it) and using Q_() to
choose between the possible variants.  I think Ævar knows about this
much better than I do.

But if we are not doing this "set multiple" and instead go the
"detect existing multiple and refrain from spreading the damage"
route, all of that is moot.

Thanks.