On 2021.12.07 16:17, Glen Choo wrote:
> Josh Steadmon <steadmon@xxxxxxxxxx> 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 :)
