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

[Glen Choo <chooglen@google.com>]

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);
  }

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

>  
> +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`.

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