| Message ID | 43d6f83fedc022c44d6a3be249e7fd8cd2a25007.1639524556.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | branch: inherit tracking configs | expand |
Josh Steadmon <steadmon@google.com> writes: > + * `origin` is the name of the remote owning the upstream branches. NULL means > + * the upstream branches are local to this repo. > + * > + * `remotes` is a list of refs that are upstream of local > + */ > +static int install_branch_config_multiple_remotes(int flag, const char *local, > + const char *origin, struct string_list *remotes) > { > 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")); > + > + if (!origin) > + for_each_string_list_item(item, remotes) > + if (skip_prefix(item->string, "refs/heads/", &shortname) > + && !strcmp(local, shortname)) { > + warning(_("not setting branch '%s' as its own upstream."), > + local); > + return 0; > + } OK, if there is the current branch _among_ the remotes and we are synching with the local repository, we reject to muck the configuration for 'local' branch with any of the refs in 'remotes'. Makes sense. Just FYI (because there is nothing actionable on your part), there is another topic in flight that wants to change the warning message to be phrased like so: _("not setting branch '%s' as its own upstream") i.e. without capitalizing the first word, and without finishing the sentence with a full stop. I'll make the conflict resolution to read as such. > -test_expect_success '--set-upstream-to notices an error to set branch as own upstream' ' > +test_expect_success '--set-upstream-to notices an error to set branch as own upstream' " > git branch --set-upstream-to refs/heads/my13 my13 2>actual && > cat >expect <<-\EOF && > - warning: Not setting branch my13 as its own upstream. > + warning: not setting branch 'my13' as its own upstream. Likewise. > EOF > test_expect_code 1 git config branch.my13.remote && > test_expect_code 1 git config branch.my13.merge && > test_cmp expect actual > -' > +" The above is currently correct but it often is an invitation for future bugs to use double-quotes around the executable part of test_expect_success. We can always write ' --> '\'' to enclose the thing in a pair of single quotes when we start needing to refer to $shell_variables in the executable part, so let's take the above patch as-is, but something to keep in mind. Thanks, queued.
Josh Steadmon <steadmon@google.com> writes: > --- a/branch.c > +++ b/branch.c > @@ -49,25 +49,41 @@ static int should_setup_rebase(const char *origin) > return 0; > } > > -static const char tracking_advice[] = > -N_("\n" > -"After fixing the error cause you may try to fix up\n" > -"the remote tracking information by invoking\n" > -"\"git branch --set-upstream-to=%s%s%s\"."); > - > -int install_branch_config(int flag, const char *local, const char *origin, const char *remote) > +/** > + * Install upstream tracking configuration for a branch; specifically, add > + * `branch.<name>.remote` and `branch.<name>.merge` entries. > + * > + * `flag` contains integer flags for options; currently only > + * BRANCH_CONFIG_VERBOSE is checked. > + * > + * `local` is the name of the branch whose configuration we're installing. > + * > + * `origin` is the name of the remote owning the upstream branches. NULL means > + * the upstream branches are local to this repo. > + * > + * `remotes` is a list of refs that are upstream of local > + */ > +static int install_branch_config_multiple_remotes(int flag, const char *local, > + const char *origin, struct string_list *remotes) Very helpful description. I got slightly confused when I first reviewed this, so having the comments will help future readers a lot. > { > 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. > @@ -75,8 +91,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, then write our branches. 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, NULL) < 0) > goto out_err; > + for_each_string_list_item(item, remotes) > + if (git_config_set_multivar_gently(key.buf, item->string, CONFIG_REGEX_NONE, 0) < 0) > + goto out_err; We get to use for_each_string_item() now, nice. > @@ -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... > } 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. 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
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.
Josh Steadmon <steadmon@google.com> writes: >> > @@ -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. I see. So if I'm reading this correctly, the error message in the remote case would read something like: branch 'main' set up to track from 'origin': main topic1 topic2 Is there any reason why we couldn't append the origin to the ref to make it consistent? I think this could be as simple as: for_each_string_list_item(item, remotes) { name = item->string; skip_prefix(name, "refs/heads/", &name); if (origin) + strbuf_addf(&ref_string, "%s/", origin); strbuf_addf(&ref_string, " %s\n", name); } and the resulting list could look like: branch 'main' set up to track from 'origin': origin/main origin/topic1 origin/topic2 This looks repetitive, but I suggest this because, as I understand it, we are omitting the "{local,remote} ref" phrase based on conventions around ref names, like "origin/main" is probably a remote ref and not an oddly named local ref. However, when we print the list like so, branch 'main' set up to track from 'origin': main topic1 topic2 we now expect the user to understand that 'main', 'topic1' and 'topic2' to implicitly have 'origin/' prepended to them. This behavior seems inconsistent to me; I'd anticipate most users responding "Wait, I was supposed to be tracking 'origin' branches right? Why am I looking at local branches?". Some users would be able to recover because they can figure out what we mean, but others might just give up. Prepending 'origin/' would get rid of this problem altogether, and it would let us drop the '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. I see. So for instance, post-simplification you have: printf_ln(rebasing ? _("branch '%s' set up to track '%s' by rebasing.") : _("branch '%s' set up to track '%s'."), local, refname.buf); if you preserve the same amount of detail as before, you'd have to distinguish between local/remote, which doubles the number of cases to 4, which is why the conditional v5 is so complicated. That said, I think that it's already much simpler than v5 because you've split the singular and plural cases. I wonder if you have considered building the final string purely from format strings, like: char *message_format = _("branch %s set up to track %s%s%s%s"); char *ref_type_clause = origin ? " remote ref " : " local ref "; char *rebasing_clause = rebasing ? " by rebasing." : "."; char *branch_names = "<branch names>"; printf_ln(message_format, local, ref_type_clause, branch_names, rebasing_clause); This sounds potentially unfriendly to i18n, but it would make the conditional simpler. What do you think?
On 2021.12.20 10:29, Glen Choo wrote: > Josh Steadmon <steadmon@google.com> writes: > > >> > @@ -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. > > I see. So if I'm reading this correctly, the error message in the remote > case would read something like: > > branch 'main' set up to track from 'origin': > main > topic1 > topic2 > > Is there any reason why we couldn't append the origin to the ref to make > it consistent? I think this could be as simple as: > > > for_each_string_list_item(item, remotes) { > name = item->string; > skip_prefix(name, "refs/heads/", &name); > if (origin) > + strbuf_addf(&ref_string, "%s/", origin); > strbuf_addf(&ref_string, " %s\n", name); > } > > and the resulting list could look like: > > branch 'main' set up to track from 'origin': > origin/main > origin/topic1 > origin/topic2 > > This looks repetitive, but I suggest this because, as I understand it, > we are omitting the "{local,remote} ref" phrase based on conventions > around ref names, like "origin/main" is probably a remote ref and not an > oddly named local ref. However, when we print the list like so, > > branch 'main' set up to track from 'origin': > main > topic1 > topic2 > > we now expect the user to understand that 'main', 'topic1' and 'topic2' > to implicitly have 'origin/' prepended to them. This behavior seems > inconsistent to me; I'd anticipate most users responding "Wait, I was > supposed to be tracking 'origin' branches right? Why am I looking at > local branches?". Some users would be able to recover because they can > figure out what we mean, but others might just give up. > > Prepending 'origin/' would get rid of this problem altogether, and it > would let us drop the 'from'. Yeah, I think that's better. Fixed in V7, thanks. > >> > } 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. > > I see. So for instance, post-simplification you have: > > printf_ln(rebasing ? > _("branch '%s' set up to track '%s' by rebasing.") : > _("branch '%s' set up to track '%s'."), > local, refname.buf); > > if you preserve the same amount of detail as before, you'd have to > distinguish between local/remote, which doubles the number of cases to > 4, which is why the conditional v5 is so complicated. > > That said, I think that it's already much simpler than v5 because you've > split the singular and plural cases. I wonder if you have considered > building the final string purely from format strings, like: > > char *message_format = _("branch %s set up to track %s%s%s%s"); > char *ref_type_clause = origin ? " remote ref " : " local ref "; > char *rebasing_clause = rebasing ? " by rebasing." : "."; > char *branch_names = "<branch names>"; > printf_ln(message_format, local, ref_type_clause, branch_names, rebasing_clause); > > This sounds potentially unfriendly to i18n, but it would make the > conditional simpler. What do you think? Yeah, the translation-unfriendliness is why I avoided this approach.
diff --git a/branch.c b/branch.c index 7a88a4861e..fa165ebdab 100644 --- a/branch.c +++ b/branch.c @@ -49,25 +49,41 @@ static int should_setup_rebase(const char *origin) return 0; } -static const char tracking_advice[] = -N_("\n" -"After fixing the error cause you may try to fix up\n" -"the remote tracking information by invoking\n" -"\"git branch --set-upstream-to=%s%s%s\"."); - -int install_branch_config(int flag, const char *local, const char *origin, const char *remote) +/** + * Install upstream tracking configuration for a branch; specifically, add + * `branch.<name>.remote` and `branch.<name>.merge` entries. + * + * `flag` contains integer flags for options; currently only + * BRANCH_CONFIG_VERBOSE is checked. + * + * `local` is the name of the branch whose configuration we're installing. + * + * `origin` is the name of the remote owning the upstream branches. NULL means + * the upstream branches are local to this repo. + * + * `remotes` is a list of refs that are upstream of local + */ +static int install_branch_config_multiple_remotes(int flag, const char *local, + const char *origin, struct string_list *remotes) { 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")); + + if (!origin) + for_each_string_list_item(item, remotes) + if (skip_prefix(item->string, "refs/heads/", &shortname) + && !strcmp(local, shortname)) { + warning(_("not setting branch '%s' as its own upstream."), + local); + return 0; + } strbuf_addf(&key, "branch.%s.remote", local); if (git_config_set_gently(key.buf, origin ? origin : ".") < 0) @@ -75,8 +91,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, then write our branches. 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, NULL) < 0) goto out_err; + for_each_string_list_item(item, remotes) + if (git_config_set_multivar_gently(key.buf, item->string, CONFIG_REGEX_NONE, 0) < 0) + goto out_err; if (rebasing) { strbuf_reset(&key); @@ -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); } 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); } + + strbuf_release(&ref_string); } return 0; @@ -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); return -1; } +int install_branch_config(int flag, const char *local, const char *origin, + const char *remote) +{ + int ret; + struct string_list remotes = STRING_LIST_INIT_DUP; + + string_list_append(&remotes, remote); + ret = install_branch_config_multiple_remotes(flag, local, origin, &remotes); + string_list_clear(&remotes, 0); + return ret; +} + /* * This is called when new_ref is branched off of orig_ref, and tries * to infer the settings for branch.<new_ref>.{remote,merge} from the diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index cc4b10236e..4b0ef35913 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -950,15 +950,15 @@ test_expect_success 'disabled option --set-upstream fails' ' test_must_fail git branch --set-upstream origin/main ' -test_expect_success '--set-upstream-to notices an error to set branch as own upstream' ' +test_expect_success '--set-upstream-to notices an error to set branch as own upstream' " git branch --set-upstream-to refs/heads/my13 my13 2>actual && cat >expect <<-\EOF && - warning: Not setting branch my13 as its own upstream. + warning: not setting branch 'my13' as its own upstream. EOF test_expect_code 1 git config branch.my13.remote && test_expect_code 1 git config branch.my13.merge && test_cmp expect actual -' +" # Keep this test last, as it changes the current branch cat >expect <<EOF
Add a new static variant of install_branch_config() that accepts multiple remote branch names for tracking. This will be used in an upcoming commit that enables inheriting the tracking configuration from a parent branch. Currently, all callers of install_branch_config() pass only a single remote. Make install_branch_config() a small wrapper around install_branch_config_multiple_remotes() so that existing callers do not need to be changed. Signed-off-by: Josh Steadmon <steadmon@google.com> --- branch.c | 135 ++++++++++++++++++++++++++++++++-------------- t/t3200-branch.sh | 6 +-- 2 files changed, 99 insertions(+), 42 deletions(-)