Re: [PATCH v5 1/2] branch: accept multiple upstream branches for tracking
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 7, 2021, 19:28 UTC
- Message-ID
- <xmqqk0gg6wqn.fsf@gitster.g>
- In-Reply-To
- <ba7d557725e70f2ae8f10ae5992c8168eb97f2fc.1638859949.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> +static int install_branch_config_multiple_remotes(int flag, const char *local, const char *origin, > + struct string_list *remotes)
The line got overly long so perhaps cut the line after "*local,", as "origin" and "remotes" conceptually are closer together.
What is in the string list? Names of refs at the remote "origin", instead of a single ref there?
Show 10 quoted lines
> {
> const char *shortname = NULL;
> struct strbuf key = STRBUF_INIT;
> - 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);When 'origin' is NULL in the original caller, it means a local tracking, and making sure we do not say "my 'master' branch builds on top of itself" makes sense.
Show 14 quoted lines
> - return 0;
> - }
> + int i, rebasing = should_setup_rebase(origin);
> +
> + if (remotes->nr < 1)
> + BUG("must provide at least one remote for branch config");
> +
> + if (!origin)
> + for (i = 0; i < remotes->nr; i++)
> + if (skip_prefix(remotes->items[i].string, "refs/heads/", &shortname)
> + && !strcmp(local, shortname)) {
> + warning(_("Not setting branch %s as its own upstream."),
> + local);
> + return 0;I am a bit surprised with this warning and early return before inspecting the remainder of the list. When 'origin' is NULL, i.e. we are talking about the local building on top of another local branch, if the function is called for the local branch 'main' with 'main' in the remotes list alone, we do want to issue the warning and exit without doing anything (i.e. degenerating to the original behaviour of taking a single string variable, when a string list with a single element is given). But if the remotes list has 'main' and 'master', would we want to just "skip" the same one, but still handle the other ones as if the "same" branch were not in the list?
Show 45 quoted lines
> @@ -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;
>
> if (rebasing) {
> strbuf_reset(&key);
> @@ -87,29 +101,62 @@ int install_branch_config(int flag, const char *local, const char *origin, const
> strbuf_release(&key);
>
> if (flag & BRANCH_CONFIG_VERBOSE) {
> - if (shortname) {
> - 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);
> + int plural = remotes->nr > 1;
> + int all_shortnames = 1;
> + const char *msg_fmt;
> + struct strbuf ref_string = STRBUF_INIT;
> +
> + for (i = 0; i < remotes->nr; i++)
> + if (skip_prefix(remotes->items[i].string, "refs/heads/", &shortname)) {
> + strbuf_addf(&ref_string, "'%s', ", shortname);
> + } else {
> + all_shortnames = 0;
> + strbuf_addf(&ref_string, "'%s', ", remotes->items[i].string);So, all_shortnames == true means everything was a local branch in the 'origin' remote, and when it has a non-branch (like a tag), all_shortnames becomes false?
> + } > + /* The last two characters are an extraneous ", ", so trim those. */ > + strbuf_setlen(&ref_string, ref_string.len - 2);
As you are starting from an empty ref_string, a more idiomatic way to build concatenated string would be to prefix when you add a new item, e.g.
loop {
if (ref_string already has items)
ref_string.append(", ");
ref_string.append(this_item);
}> + if (all_shortnames && origin) {
> + if (rebasing && plural)
> + msg_fmt = "Branch '%s' set up to track remote branches %s from '%s' by rebasing.";What does it mean to keep my 'topic' branch up-to-date by rebasing on top of more than one remote sources? By merging, I can sort-of understand (i.e. creating an octopus), but would it make sense to track more than one remote sources in general? Is it common?
When the benefit is not clear, it might make more sense not to do this when there are already multiple tracking sources defined for the original; it might be a mistake that we may not want to spread with the new option.
Of course, it is very possible that I am missing a perfectly valid use case where having more than one makes good sense. If so, please do not take the above comments as an objection, but adding some comments before the function to explain when having remote list with more than one items makes sense and how such a setting can be used to avoid future readers asking the same (stupid) question as I just did.
Show 8 quoted lines
> + else if (rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track remote branch %s from '%s' by rebasing."; > + else if (!rebasing && plural) > + msg_fmt = "Branch '%s' set up to track remote branches %s from '%s'."; > + else if (!rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track remote branch %s from '%s'."; > + > + printf_ln(_(msg_fmt), local, ref_string, origin);
I am not sure how well the "plural" thing works with i18n. It may suffice for the original in English to have only two choices between one or more-than-one, but not all languages are English. Counting the actual number (I guess remotes->nr is it) and using Q_() to choose between the possible variants. I think Ævar knows about this much better than I do.
But if we are not doing this "set multiple" and instead go the "detect existing multiple and refrain from spreading the damage" route, all of that is moot.
Thanks.