Re: [PATCH v6 1/3] branch: accept multiple upstream branches for tracking

[Glen Choo <chooglen@google.com>]

Josh Steadmon <steadmon@google.com> writes:

> --- a/branch.c
> +++ b/branch.c
> @@ -49,25 +49,41 @@ static int should_setup_rebase(const char *origin)
>  	return 0;
>  }
>  
> -static const char tracking_advice[] =
> -N_("\n"
> -"After fixing the error cause you may try to fix up\n"
> -"the remote tracking information by invoking\n"
> -"\"git branch --set-upstream-to=%s%s%s\".");
> -
> -int install_branch_config(int flag, const char *local, const char *origin, const char *remote)
> +/**
> + * Install upstream tracking configuration for a branch; specifically, add
> + * `branch.<name>.remote` and `branch.<name>.merge` entries.
> + *
> + * `flag` contains integer flags for options; currently only
> + * BRANCH_CONFIG_VERBOSE is checked.
> + *
> + * `local` is the name of the branch whose configuration we're installing.
> + *
> + * `origin` is the name of the remote owning the upstream branches. NULL means
> + * the upstream branches are local to this repo.
> + *
> + * `remotes` is a list of refs that are upstream of local
> + */
> +static int install_branch_config_multiple_remotes(int flag, const char *local,
> +		const char *origin, struct string_list *remotes)

Very helpful description. I got slightly confused when I first reviewed
this, so having the comments will help future readers a lot.

>  {
>  	const char *shortname = NULL;
>  	struct strbuf key = STRBUF_INIT;
> +	struct string_list_item *item;
>  	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);
> -		return 0;
> -	}
> +	if (!remotes->nr)
> +		BUG("must provide at least one remote for branch config");
> +	if (rebasing && remotes->nr > 1)
> +		die(_("cannot inherit upstream tracking configuration when rebasing is requested"));

Nit: if we're being pedantic, we cannot inherit upstream tracking
configuration when rebasing is requested with multiple upstream
branches.

But this message is already very niche and loaded with specifics, so
adding "...with multiple upstream branches" might just be more
confusing, so this is a nit.

> @@ -75,8 +91,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, then write our branches. 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, NULL) < 0)
>  		goto out_err;
> +	for_each_string_list_item(item, remotes)
> +		if (git_config_set_multivar_gently(key.buf, item->string, CONFIG_REGEX_NONE, 0) < 0)
> +			goto out_err;

We get to use for_each_string_item() now, nice.

> @@ -87,29 +112,42 @@ int install_branch_config(int flag, const char *local, const char *origin, const
>  	strbuf_release(&key);
>  
>  	if (flag & BRANCH_CONFIG_VERBOSE) {
> -		if (shortname) {
> +		const char *name;
> +		struct strbuf ref_string = STRBUF_INIT;
> +
> +		for_each_string_list_item(item, remotes) {
> +			name = item->string;
> +			skip_prefix(name, "refs/heads/", &name);
> +			strbuf_addf(&ref_string, "  %s\n", name);
> +		}
> +
> +		if (remotes->nr == 1) {
> +			struct strbuf refname = STRBUF_INIT;
> +
>  			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);
> +				strbuf_addf(&refname, "%s/", origin);
> +			strbuf_addstr(&refname, remotes->items[0].string);
> +
> +			/*
> +			 * Rebasing is only allowed in the case of a single
> +			 * upstream branch.
> +			 */
> +			printf_ln(rebasing ?
> +				_("branch '%s' set up to track '%s' by rebasing.") :
> +				_("branch '%s' set up to track '%s'."),
> +				local, refname.buf);
> +
> +			strbuf_release(&refname);
> +		} else if (origin) {
> +			printf_ln(_("branch '%s' set up to track from '%s':"),
> +				local, origin);
> +			printf("%s", ref_string.buf);

It's not clear to me why the hint contains the word 'from' when it is a
remote ref...

>  		} else {
> -			if (origin)
> -				printf_ln(rebasing ?
> -					  _("Branch '%s' set up to track remote ref '%s' by rebasing.") :
> -					  _("Branch '%s' set up to track remote ref '%s'."),
> -					  local, remote);
> -			else
> -				printf_ln(rebasing ?
> -					  _("Branch '%s' set up to track local ref '%s' by rebasing.") :
> -					  _("Branch '%s' set up to track local ref '%s'."),
> -					  local, remote);
> +			printf_ln(_("branch '%s' set up to track:"), local);
> +			printf("%s", ref_string.buf);

but does not have the word 'from' when it is a local ref. As far as I
can tell, this is the only difference between remote and local refs, and
adding the word 'from' does not seem like a good enough reason to add an
'if' condition. Maybe I missed something here?

This motivates my answer to the question you asked in [1]:

  I removed as many distinctions as possible, as most can still be
  inferred from context. [...] Likewise, we don't need to specify whether
  refs are remote or local: "some-remote/some-branch" vs.
  "a-local-branch" should be understandable without us spelling it out.

I agree that there is adequate context, so I would be ok with the
simplification if there was corresponding code simplification e.g.
dropping "if (origin)". But in its current form, I don't think there is
good enough reason to simplify the message.

Of course, IIUC, this is as simple as dropping 'from' in the "if
(origin)" case.

> @@ -118,14 +156,33 @@ int install_branch_config(int flag, const char *local, const char *origin, const
>  	strbuf_release(&key);
>  	error(_("Unable to write upstream branch configuration"));
>  
> -	advise(_(tracking_advice),
> -	       origin ? origin : "",
> -	       origin ? "/" : "",
> -	       shortname ? shortname : remote);
> +	advise(_("\nAfter fixing the error cause you may try to fix up\n"
> +		"the remote tracking information by invoking:"));
> +	if (remotes->nr == 1)
> +		advise("  git branch --set-upstream-to=%s%s%s",
> +			origin ? origin : "",
> +			origin ? "/" : "",
> +			remotes->items[0].string);
> +	else
> +		for_each_string_list_item(item, remotes)
> +			advise("  git config --add branch.\"%s\".merge %s",
> +				local, item->string);

The advice is now correct, nice! Does the user also need to run "git
config --add branch.%s.remote %s" ?

[1] https://lore.kernel.org/git/Ybk6QsMdeBl6IweW@google.com