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

[Josh Steadmon <steadmon@google.com>]

On 2021.12.16 11:57, Glen Choo wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> >  {
> >  	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.

I think it's worthwhile to be precise. Thanks for pointing this out.
Fixed in V7.


> > @@ -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...

Because in the multiple-branch case, we don't prepend the origin to each
ref, so we need to let users know which remote the refs are coming from.


> >  		} 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.

I think the proper point of comparison is not the original code, but the
code from V5 where we try to preserve the same level of detail in output
as the original code. If we are committed to both having multiple
remotes and keeping similar styles of output as the original
implementation, then something like the massive conditional in V5 is
unavoidable.

> 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

Yes, thank you for the catch. Fixed in V7.