| Message ID | ba7d557725e70f2ae8f10ae5992c8168eb97f2fc.1638859949.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | branch: inherit tracking configs | expand |
On Mon, Dec 06 2021, Josh Steadmon wrote: > 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 | 120 ++++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 87 insertions(+), 33 deletions(-) > > diff --git a/branch.c b/branch.c > index 7a88a4861e..1aabef4de0 100644 > --- a/branch.c > +++ b/branch.c > @@ -55,19 +55,24 @@ N_("\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) > +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; > - 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; > - } > + int i, rebasing = should_setup_rebase(origin); > + > + if (remotes->nr < 1) > + BUG("must provide at least one remote for branch config"); Since it's unsigned IMO this would be clearer: if (!remotes->nr) > + > + if (!origin) > + for (i = 0; i < remotes->nr; i++) > + if (skip_prefix(remotes->items[i].string, "refs/heads/", &shortname) For this and others, since you don't use the [i] for anything except getting the current item using for_each_string_list_item() would be better. Partially a nit, partially that I've got another WIP soon-to-be-submitted topic to fix overflow bugs in that API, and not having "int i" etc. hardcoded in various places helps. I.e. for_each_string_list_item() is future-proof. > + && !strcmp(local, shortname)) { > + warning(_("Not setting branch %s as its own upstream."), Better to quote '%s', also s/Not/not/ (lower-case) for all error/warning/die etc. > + local); > + return 0; > + } > > strbuf_addf(&key, "branch.%s.remote", local); > if (git_config_set_gently(key.buf, origin ? origin : ".") < 0) > @@ -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; This.... > + 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); > + } > + /* The last two characters are an extraneous ", ", so trim those. */ > + strbuf_setlen(&ref_string, ref_string.len - 2); languages RTL in around way wrong the be to going is thing of sort This. :) We deal with this in most other places by just formatting a list of branches. E.g. in the ambiguous object output: https://lore.kernel.org/git/patch-v5-4.6-36b6b440c37-20211125T215529Z-avarab@gmail.com/ > + > + if (all_shortnames && origin) { > + if (rebasing && plural) > + msg_fmt = "Branch '%s' set up to track remote branches %s from '%s' by rebasing."; > + 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'."; ...and this is hardcoding plural rules used in English that don't apply in a lot of other languages... > + > + printf_ln(_(msg_fmt), local, ref_string, origin); > } 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); > + if (all_shortnames && !origin && rebasing && plural) > + msg_fmt = "Branch '%s' set up to track local branches %s by rebasing."; > + if (all_shortnames && !origin && rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track local branch %s by rebasing."; > + if (all_shortnames && !origin && !rebasing && plural) > + msg_fmt = "Branch '%s' set up to track local branches %s."; > + if (all_shortnames && !origin && !rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track local branch %s."; > + if (!all_shortnames && origin && rebasing && plural) > + msg_fmt = "Branch '%s' set up to track remote refs %s by rebasing."; > + if (!all_shortnames && origin && rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track remote ref %s by rebasing."; > + if (!all_shortnames && origin && !rebasing && plural) > + msg_fmt = "Branch '%s' set up to track remote refs %s."; > + if (!all_shortnames && origin && !rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track remote ref %s."; > + if (!all_shortnames && !origin && rebasing && plural) > + msg_fmt = "Branch '%s' set up to track local refs %s by rebasing."; > + if (!all_shortnames && !origin && rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track local ref %s by rebasing."; > + if (!all_shortnames && !origin && !rebasing && plural) > + msg_fmt = "Branch '%s' set up to track local refs %s."; > + if (!all_shortnames && !origin && !rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track local ref %s."; ...in English you've got one dog, then dogs, so == 1 and >1, but in various other languages it's: git grep Plural-Forms -- po Anyway, this is easily solved, and even with less verbosity, see: git grep -E -W '\bQ_\(' For examples of how to use the magic of libintl to do this for you. > + > + printf_ln(_(msg_fmt), local, ref_string); ...also s/Branch/branch/ for all of them/. > } > + > + strbuf_release(&ref_string); > } > > return 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; > } > > +int install_branch_config(int flag, const char *local, const char *origin, const char *remote) { nit: overly long line.. > + struct string_list remotes = STRING_LIST_INIT_DUP; nit: an extra \n after variable decls... > + string_list_append(&remotes, remote); > + return install_branch_config_multiple_remotes(flag, local, origin, &remotes); > + string_list_clear(&remotes, 0); > +} > + > /* > * 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
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? > { > 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. > - 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? > @@ -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. > + 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.
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 :)
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 :)
I noticed some test failures due to the printf_ln(msg_fmt) region. Since you are reworking this, the problem might just go away, but I thought I should mention it just in case. > + if (all_shortnames && origin) { > + if (rebasing && plural) > + msg_fmt = "Branch '%s' set up to track remote branches %s from '%s' by rebasing."; > + 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); Here > } 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); > + if (all_shortnames && !origin && rebasing && plural) > + msg_fmt = "Branch '%s' set up to track local branches %s by rebasing."; > + if (all_shortnames && !origin && rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track local branch %s by rebasing."; > + if (all_shortnames && !origin && !rebasing && plural) > + msg_fmt = "Branch '%s' set up to track local branches %s."; > + if (all_shortnames && !origin && !rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track local branch %s."; > + if (!all_shortnames && origin && rebasing && plural) > + msg_fmt = "Branch '%s' set up to track remote refs %s by rebasing."; > + if (!all_shortnames && origin && rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track remote ref %s by rebasing."; > + if (!all_shortnames && origin && !rebasing && plural) > + msg_fmt = "Branch '%s' set up to track remote refs %s."; > + if (!all_shortnames && origin && !rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track remote ref %s."; > + if (!all_shortnames && !origin && rebasing && plural) > + msg_fmt = "Branch '%s' set up to track local refs %s by rebasing."; > + if (!all_shortnames && !origin && rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track local ref %s by rebasing."; > + if (!all_shortnames && !origin && !rebasing && plural) > + msg_fmt = "Branch '%s' set up to track local refs %s."; > + if (!all_shortnames && !origin && !rebasing && !plural) > + msg_fmt = "Branch '%s' set up to track local ref %s."; > + > + printf_ln(_(msg_fmt), local, ref_string); and here > } > + > + strbuf_release(&ref_string); > } We print ref_string, which is a strbuf. This causes t/t3200-branch.sh to segfault on my mac + clang, but inconsistently! With -O2, it doesn't always segfault, but the wrong memory is read: Branch 'my3' set up to track remote branch local from 'Branch '%s' set up to track remote branch %s from '%s'.'. With -O0, it always segfaults. You can see this in the osx-clang run in [1], but it looks like the gcc ones refuse to build. s/ref_string/ref_string.buf should fix the problem. [1] https://github.com/chooglen/git/runs/4464134763?check_suite_focus=true
Glen Choo <chooglen@google.com> writes: > We print ref_string, which is a strbuf. This causes t/t3200-branch.sh to > segfault on my mac + clang, but inconsistently! With -O2, it doesn't > always segfault, but the wrong memory is read: > > Branch 'my3' set up to track remote branch local from 'Branch '%s' set up to track remote branch %s from '%s'.'. I forgot to mention this earlier but in this example, the test *passes* even though the stderr message is obviously wrong. I don't see any coverage of the help message in t3200, which is a bit worrying to me. After this series is done, is it worth adding test coverage of the help message?
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 :)
On 2021.12.08 16:08, Glen Choo wrote: > Glen Choo <chooglen@google.com> writes: > > > We print ref_string, which is a strbuf. This causes t/t3200-branch.sh to > > segfault on my mac + clang, but inconsistently! With -O2, it doesn't > > always segfault, but the wrong memory is read: > > > > Branch 'my3' set up to track remote branch local from 'Branch '%s' set up to track remote branch %s from '%s'.'. > > I forgot to mention this earlier but in this example, the test *passes* > even though the stderr message is obviously wrong. I don't see any > coverage of the help message in t3200, which is a bit worrying to me. > > After this series is done, is it worth adding test coverage of the help > message? Yeah, I caught this earlier while reworking this section based on Ævar's review, but thank you for pointing it out. I'm unsure about checking formatting of message strings in tests; it would certainly have caught this bug but it seems that more often they're just "change detectors" rather than good tests. But I could be swayed if you or others feel it's important.
On 2021.12.07 09:57, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Dec 06 2021, Josh Steadmon wrote: > > > 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 | 120 ++++++++++++++++++++++++++++++++++++++++--------------- > > 1 file changed, 87 insertions(+), 33 deletions(-) > > > > diff --git a/branch.c b/branch.c > > index 7a88a4861e..1aabef4de0 100644 > > --- a/branch.c > > +++ b/branch.c > > @@ -55,19 +55,24 @@ N_("\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) > > +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; > > - 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; > > - } > > + int i, rebasing = should_setup_rebase(origin); > > + > > + if (remotes->nr < 1) > > + BUG("must provide at least one remote for branch config"); > > Since it's unsigned IMO this would be clearer: if (!remotes->nr) Fixed in v6. > > + > > + if (!origin) > > + for (i = 0; i < remotes->nr; i++) > > + if (skip_prefix(remotes->items[i].string, "refs/heads/", &shortname) > > For this and others, since you don't use the [i] for anything except > getting the current item using for_each_string_list_item() would be > better. > > Partially a nit, partially that I've got another WIP > soon-to-be-submitted topic to fix overflow bugs in that API, and not > having "int i" etc. hardcoded in various places > helps. I.e. for_each_string_list_item() is future-proof. Thanks. I somehow missed for_each_string_list_item() when I checked the header file. Fixed in v6. > > + && !strcmp(local, shortname)) { > > + warning(_("Not setting branch %s as its own upstream."), > > Better to quote '%s', also s/Not/not/ (lower-case) for all error/warning/die etc. Done (for now) in v6, but this might become moot as I address Junio's review comments. > > + local); > > + return 0; > > + } > > > > strbuf_addf(&key, "branch.%s.remote", local); > > if (git_config_set_gently(key.buf, origin ? origin : ".") < 0) > > @@ -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; > > This.... > > > + 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); > > + } > > + /* The last two characters are an extraneous ", ", so trim those. */ > > + strbuf_setlen(&ref_string, ref_string.len - 2); > > languages RTL in around way wrong the be to going is thing of sort This. > > :) > > We deal with this in most other places by just formatting a list of > branches. E.g. in the ambiguous object output: > > https://lore.kernel.org/git/patch-v5-4.6-36b6b440c37-20211125T215529Z-avarab@gmail.com/ Done, thanks for the suggestion. > > + > > + if (all_shortnames && origin) { > > + if (rebasing && plural) > > + msg_fmt = "Branch '%s' set up to track remote branches %s from '%s' by rebasing."; > > + 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'."; > > ...and this is hardcoding plural rules used in English that don't apply > in a lot of other languages... > > > + > > + printf_ln(_(msg_fmt), local, ref_string, origin); > > } 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); > > + if (all_shortnames && !origin && rebasing && plural) > > + msg_fmt = "Branch '%s' set up to track local branches %s by rebasing."; > > + if (all_shortnames && !origin && rebasing && !plural) > > + msg_fmt = "Branch '%s' set up to track local branch %s by rebasing."; > > + if (all_shortnames && !origin && !rebasing && plural) > > + msg_fmt = "Branch '%s' set up to track local branches %s."; > > + if (all_shortnames && !origin && !rebasing && !plural) > > + msg_fmt = "Branch '%s' set up to track local branch %s."; > > + if (!all_shortnames && origin && rebasing && plural) > > + msg_fmt = "Branch '%s' set up to track remote refs %s by rebasing."; > > + if (!all_shortnames && origin && rebasing && !plural) > > + msg_fmt = "Branch '%s' set up to track remote ref %s by rebasing."; > > + if (!all_shortnames && origin && !rebasing && plural) > > + msg_fmt = "Branch '%s' set up to track remote refs %s."; > > + if (!all_shortnames && origin && !rebasing && !plural) > > + msg_fmt = "Branch '%s' set up to track remote ref %s."; > > + if (!all_shortnames && !origin && rebasing && plural) > > + msg_fmt = "Branch '%s' set up to track local refs %s by rebasing."; > > + if (!all_shortnames && !origin && rebasing && !plural) > > + msg_fmt = "Branch '%s' set up to track local ref %s by rebasing."; > > + if (!all_shortnames && !origin && !rebasing && plural) > > + msg_fmt = "Branch '%s' set up to track local refs %s."; > > + if (!all_shortnames && !origin && !rebasing && !plural) > > + msg_fmt = "Branch '%s' set up to track local ref %s."; > > ...in English you've got one dog, then dogs, so == 1 and >1, but in > various other languages it's: > > git grep Plural-Forms -- po > > Anyway, this is easily solved, and even with less verbosity, see: > > git grep -E -W '\bQ_\(' > > For examples of how to use the magic of libintl to do this for you. Thank you for the pointer. I looked specifically for dealing with plural forms in our docs, but the referenced "Preparing Strings" gettext docs were not helpful for this. (Although I see now I should have read further in po/README.md to find the relevant advice). I may send a separate change to po/README.md to make it easier to find in the future. > > + > > + printf_ln(_(msg_fmt), local, ref_string); > > ...also s/Branch/branch/ for all of them/. Done. > > } > > + > > + strbuf_release(&ref_string); > > } > > > > return 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; > > } > > > > +int install_branch_config(int flag, const char *local, const char *origin, const char *remote) { > > nit: overly long line.. Fixed. > > + struct string_list remotes = STRING_LIST_INIT_DUP; > > nit: an extra \n after variable decls... Fixed. > > + string_list_append(&remotes, remote); > > + return install_branch_config_multiple_remotes(flag, local, origin, &remotes); > > + string_list_clear(&remotes, 0); > > +} > > + > > /* > > * 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 > Thanks for the review!
Josh Steadmon <steadmon@google.com> writes: >> > We print ref_string, which is a strbuf. This causes t/t3200-branch.sh to >> > segfault on my mac + clang, but inconsistently! With -O2, it doesn't >> > always segfault, but the wrong memory is read: >> > >> > Branch 'my3' set up to track remote branch local from 'Branch '%s' set up to track remote branch %s from '%s'.'. >> >> I forgot to mention this earlier but in this example, the test *passes* >> even though the stderr message is obviously wrong. I don't see any >> coverage of the help message in t3200, which is a bit worrying to me. >> >> After this series is done, is it worth adding test coverage of the help >> message? > > Yeah, I caught this earlier while reworking this section based on Ævar's > review, but thank you for pointing it out. I'm unsure about checking > formatting of message strings in tests; it would certainly have caught > this bug but it seems that more often they're just "change detectors" > rather than good tests. I agree, although such a test might still be beneficial on the whole if the change detector is easy to update. > But I could be swayed if you or others feel it's important. Because this is rather "change detector"y, I don't think it's important either, but I'm also open to being convinced.
Josh Steadmon <steadmon@google.com> writes: >> > @@ -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? That sounds like a good idea if it's not too much work. Otherwise, a NEEDSWORK is still acceptable to me (but that said, I'm not an authority on this matter).
On Thu, Dec 09 2021, Josh Steadmon wrote: > On 2021.12.07 09:57, Ævar Arnfjörð Bjarmason wrote: >> >> On Mon, Dec 06 2021, Josh Steadmon wrote: >> ...in English you've got one dog, then dogs, so == 1 and >1, but in >> various other languages it's: >> >> git grep Plural-Forms -- po >> >> Anyway, this is easily solved, and even with less verbosity, see: >> >> git grep -E -W '\bQ_\(' >> >> For examples of how to use the magic of libintl to do this for you. > > Thank you for the pointer. I looked specifically for dealing with plural > forms in our docs, but the referenced "Preparing Strings" gettext docs > were not helpful for this. (Although I see now I should have read > further in po/README.md to find the relevant advice). I may send a > separate change to po/README.md to make it easier to find in the future. Thanks, that would be really helpful. >> > + string_list_append(&remotes, remote); >> > + return install_branch_config_multiple_remotes(flag, local, origin, &remotes); >> > + string_list_clear(&remotes, 0); >> > +} >> > + >> > /* >> > * 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 >> > > > Thanks for the review! Happy to help, cheers!
On Thu, Dec 09 2021, Glen Choo wrote: > Josh Steadmon <steadmon@google.com> writes: > >>> > @@ -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? > > That sounds like a good idea if it's not too much work. Otherwise, a > NEEDSWORK is still acceptable to me (but that said, I'm not an authority > on this matter). We haven't used Q_() with advise() yet, but there's no reason not to: advise(Q_("fix your branch by doing xyz", "fix your branches by doing xyz", branches_nr));
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Thu, Dec 09 2021, Glen Choo wrote: > >> Josh Steadmon <steadmon@google.com> writes: >> >>>> > @@ -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? >> >> That sounds like a good idea if it's not too much work. Otherwise, a >> NEEDSWORK is still acceptable to me (but that said, I'm not an authority >> on this matter). > > We haven't used Q_() with advise() yet, but there's no reason not to: > > advise(Q_("fix your branch by doing xyz", > "fix your branches by doing xyz", > branches_nr)); Neat, that should do it in most cases. I think this one is a little trickier because the plural advice messages requires constructing a list, e.g. Singular: "\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\"." Plural: "\n" "After fixing the error cause you may try to fix up\n" "the remote tracking information by invoking\n" "\"git config --add my_new_remote remote_name\" "\"git config --add my_new_upstream1 upstream_name1\" "\"git config --add my_new_upstream2 upstream_name2\" But perhaps this is not too hard since you've already included examples of how to format lists of strings [1] [1] https://lore.kernel.org/git/211207.86mtlcpyu4.gmgdl@evledraar.gmail.com
On Fri, Dec 10 2021, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Thu, Dec 09 2021, Glen Choo wrote: >> >>> Josh Steadmon <steadmon@google.com> writes: >>> >>>>> > @@ -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? >>> >>> That sounds like a good idea if it's not too much work. Otherwise, a >>> NEEDSWORK is still acceptable to me (but that said, I'm not an authority >>> on this matter). >> >> We haven't used Q_() with advise() yet, but there's no reason not to: >> >> advise(Q_("fix your branch by doing xyz", >> "fix your branches by doing xyz", >> branches_nr)); > > Neat, that should do it in most cases. I think this one is a little > trickier because the plural advice messages requires constructing a > list, e.g. > > Singular: > "\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\"." > > Plural: > "\n" > "After fixing the error cause you may try to fix up\n" > "the remote tracking information by invoking\n" > "\"git config --add my_new_remote remote_name\" > "\"git config --add my_new_upstream1 upstream_name1\" > "\"git config --add my_new_upstream2 upstream_name2\" > > But perhaps this is not too hard since you've already included examples > of how to format lists of strings [1] > > [1] https://lore.kernel.org/git/211207.86mtlcpyu4.gmgdl@evledraar.gmail.com You don't need to use the plural facility for that sort of message. Plurals in translated messages are specifically for the case where you need to compose a sentence like: I had %d glasses of water with breakfast But it's not needed for a message that can be rehrased as a heading followed by a list of 1 or more items, e.g.: Things I've consumed in liquid form during breakfast this week: - Water - Tea - Coffe If that list stopped at "Water" it would be OK. Every language has some way of referring to items on a list in general terms, without knowing if that list is composed of only one item, two etc.
On 2021.12.07 11:28, Junio C Hamano wrote: > 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? Added a comment explaining the purpose and arguments. > > { > > 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. > > > - 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? The inheritance case when creating a new branch is the only time we get multiple branches, and I think it is a sign of a wider misconfiguration if we have our own branch name listed as upstream in this case. When inheriting, `local` should always be a new branch, and `remotes` should contain the `branch.<name>.merge` entries of the branch we're inheriting from. For this check to trigger would mean that the parent branch has configured a local upstream branch that doesn't actually exist. So it seems that something has gone wrong; perhaps we can assume what the user wanted such as in your case above, but it seems to me that it's safer to warn when this happens. > > @@ -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); > } Ack, although changes to address other review feedback has made this point moot. > > + 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. No, that's an oversight on my part. This will now exit with an error if we try to rebase onto multiple branches. > > + 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. Thanks for the feedback!
diff --git a/branch.c b/branch.c index 7a88a4861e..1aabef4de0 100644 --- a/branch.c +++ b/branch.c @@ -55,19 +55,24 @@ N_("\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) +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; - 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; - } + 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; + } strbuf_addf(&key, "branch.%s.remote", local); if (git_config_set_gently(key.buf, origin ? origin : ".") < 0) @@ -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); + } + /* The last two characters are an extraneous ", ", so trim those. */ + strbuf_setlen(&ref_string, ref_string.len - 2); + + if (all_shortnames && origin) { + if (rebasing && plural) + msg_fmt = "Branch '%s' set up to track remote branches %s from '%s' by rebasing."; + 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); } 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); + if (all_shortnames && !origin && rebasing && plural) + msg_fmt = "Branch '%s' set up to track local branches %s by rebasing."; + if (all_shortnames && !origin && rebasing && !plural) + msg_fmt = "Branch '%s' set up to track local branch %s by rebasing."; + if (all_shortnames && !origin && !rebasing && plural) + msg_fmt = "Branch '%s' set up to track local branches %s."; + if (all_shortnames && !origin && !rebasing && !plural) + msg_fmt = "Branch '%s' set up to track local branch %s."; + if (!all_shortnames && origin && rebasing && plural) + msg_fmt = "Branch '%s' set up to track remote refs %s by rebasing."; + if (!all_shortnames && origin && rebasing && !plural) + msg_fmt = "Branch '%s' set up to track remote ref %s by rebasing."; + if (!all_shortnames && origin && !rebasing && plural) + msg_fmt = "Branch '%s' set up to track remote refs %s."; + if (!all_shortnames && origin && !rebasing && !plural) + msg_fmt = "Branch '%s' set up to track remote ref %s."; + if (!all_shortnames && !origin && rebasing && plural) + msg_fmt = "Branch '%s' set up to track local refs %s by rebasing."; + if (!all_shortnames && !origin && rebasing && !plural) + msg_fmt = "Branch '%s' set up to track local ref %s by rebasing."; + if (!all_shortnames && !origin && !rebasing && plural) + msg_fmt = "Branch '%s' set up to track local refs %s."; + if (!all_shortnames && !origin && !rebasing && !plural) + msg_fmt = "Branch '%s' set up to track local ref %s."; + + printf_ln(_(msg_fmt), local, ref_string); } + + strbuf_release(&ref_string); } return 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; } +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); +} + /* * 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
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 | 120 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 87 insertions(+), 33 deletions(-)