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

[Josh Steadmon <steadmon@google.com>]

On 2021.12.07 16:17, Glen Choo wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > @@ -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;
> 
> I think that instead of overriding all config with the first value and
> then appending every value after that, it'll be more obvious to readers
> if we first unset all of the config, then write every value (then the
> comment wouldn't have to justify why we make two calls and iteration
> starts at 1).
> 
> I believe that unsetting all values for a key is supported by
> git_config_set_multivar_gently() with value == NULL, i.e.
> 
>   /*
>    * unset with value = NULL, not sure how this interacts with
>    * CONFIG_REGEX_NONE
>    */
>   if (git_config_set_multivar_gently(key.buf, NULL,
>     CONFIG_REGEX_NONE, 0))
>     goto out_err;
> 
>   for_each_string_list_item(item, remotes) {
>     git_config_set_multivar_gently(key.buf, item, CONFIG_REGEX_NONE, 0);
>   }

Fixed in V6, thanks for the suggestion.


> > @@ -121,11 +168,18 @@ int install_branch_config(int flag, const char *local, const char *origin, const
> >  	advise(_(tracking_advice),
> >  	       origin ? origin : "",
> >  	       origin ? "/" : "",
> > -	       shortname ? shortname : remote);
> > +	       remotes->items[0].string);
> >  
> >  	return -1;
> >  }
> 
> When there is more than one item in remotes->items, this advice is
> _technically_ incorrect because --set-upstream-to only takes a single
> upstream branch. I think that supporting multiple upstreams in
> --set-upstream-to is a fairly niche use case and is out of scope of this
> series, so let's not pursue that option.
> 
> Another option would be to replace the mention of --set-upstream-to with
> "git config add", but that's unfriendly to the >90% of the user
> population that doesn't want multiple merge entries.
> 
> If we leave the advice as-is, even though it is misleading, a user who
> is sophisticated enough to set up multiple merge entries should also
> know that --set-upstream-to won't solve their problems, and would
> probably be able to fix their problems by mucking around with
> .git/config or git config.
> 
> So I think it is ok to not change the advice and to only mention the
> first merge item. However, it might be worth marking this as NEEDSWORK
> so that subsequent readers of this file understand that this advice is
> overly-simplistic and might be worth fixing.

Sounds like we should just have separate advice strings for single vs.
multiple merge configs?


> >  
> > +int install_branch_config(int flag, const char *local, const char *origin, const char *remote) {
> > +	struct string_list remotes = STRING_LIST_INIT_DUP;
> > +	string_list_append(&remotes, remote);
> > +	return install_branch_config_multiple_remotes(flag, local, origin, &remotes);
> > +	string_list_clear(&remotes, 0);
> > +}
> 
> string_list_clear() is being called after `return`.

That's an embarrassing bug, thank you for catching it.


> Ævar and Junio have commented on i18n, but I'm unfamiliar with that, so
> I won't comment on that :)