| Message ID | a5265e1c7f4dc1a69e85511d2cc3f659cc46788f.1640039978.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a3f40ec4b0dc56a54e6a6866d0a44a04d4a6edf0 |
| Headers | show |
| Series | branch: inherit tracking configs | expand |
Josh Steadmon <steadmon@google.com> writes: > +/** > + * 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 (!remotes->nr) > + BUG("must provide at least one remote for branch config"); > + if (rebasing && remotes->nr > 1) > + die(_("cannot inherit upstream tracking configuration of " > + "multiple refs when rebasing is requested")); > + > + /* > + * If the new branch is trying to track itself, something has gone > + * wrong. Warn the user and don't proceed any further. > + */ > + 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; > + } Added comments make it easier to follow what is going on and more importantly why. I very much like it ;-) > @@ -75,8 +96,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. > + */ Sorry, but I do not quite get this comment in two ways. - It talks about "if more than one" but the code seems to use regex-none even when there is only one. - I agree that use of set-multivar with regex-none is the right way to append new values to multi valued configuration, but wouldn't the sequence be (1) first clear everything and then (2) set new ones one by one? Ahh, OK, the "config-set" we do first is the "clear everything" step. We want to overwrite any existing config. First clear any existing config, and then write our branches one-by-one. Use of CONFIG_REGEX_NONE ensures that the multiple values are appended to the same variable. perhaps? > + 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; > @@ -87,29 +117,40 @@ int install_branch_config(int flag, const char *local, const char *origin, const > strbuf_release(&key); > > if (flag & BRANCH_CONFIG_VERBOSE) { > + struct strbuf tmp_ref_name = STRBUF_INIT; > + struct string_list friendly_ref_names = STRING_LIST_INIT_DUP; > + > + for_each_string_list_item(item, remotes) { > + shortname = item->string; > + skip_prefix(shortname, "refs/heads/", &shortname); > + if (origin) { > + strbuf_addf(&tmp_ref_name, "%s/%s", > + origin, shortname); > + string_list_append_nodup( > + &friendly_ref_names, > + strbuf_detach(&tmp_ref_name, NULL)); > + } else { > + string_list_append( > + &friendly_ref_names, shortname); > + } > + } > + > + if (remotes->nr == 1) { > + /* > + * Rebasing is only allowed in the case of a single > + * upstream branch. > + */ There does not seem to be any code to forbid "rebasing" when remotes->nr != 1, though. Did I miss a call to die() earlier? > + printf_ln(rebasing ? > + _("branch '%s' set up to track '%s' by rebasing.") : > + _("branch '%s' set up to track '%s'."), > + local, friendly_ref_names.items[0].string); > } else { > + printf_ln(_("branch '%s' set up to track:"), local); > + for_each_string_list_item(item, &friendly_ref_names) > + printf_ln(" %s", item->string); In other words, I would have expected something in this else clause. > } Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> +/** >> + * 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 (!remotes->nr) >> + BUG("must provide at least one remote for branch config"); >> + if (rebasing && remotes->nr > 1) >> + die(_("cannot inherit upstream tracking configuration of " >> + "multiple refs when rebasing is requested")); >> + >> + /* >> + * If the new branch is trying to track itself, something has gone >> + * wrong. Warn the user and don't proceed any further. >> + */ >> + 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; >> + } > > Added comments make it easier to follow what is going on and more > importantly why. I very much like it ;-) Agreed! We made a lot of 'why' decisions and I think the comments do a great job of capturing that. >> @@ -87,29 +117,40 @@ int install_branch_config(int flag, const char *local, const char *origin, const >> strbuf_release(&key); >> >> if (flag & BRANCH_CONFIG_VERBOSE) { >> + struct strbuf tmp_ref_name = STRBUF_INIT; >> + struct string_list friendly_ref_names = STRING_LIST_INIT_DUP; >> + >> + for_each_string_list_item(item, remotes) { >> + shortname = item->string; >> + skip_prefix(shortname, "refs/heads/", &shortname); >> + if (origin) { >> + strbuf_addf(&tmp_ref_name, "%s/%s", >> + origin, shortname); >> + string_list_append_nodup( >> + &friendly_ref_names, >> + strbuf_detach(&tmp_ref_name, NULL)); >> + } else { >> + string_list_append( >> + &friendly_ref_names, shortname); >> + } >> + } >> + >> + if (remotes->nr == 1) { >> + /* >> + * Rebasing is only allowed in the case of a single >> + * upstream branch. >> + */ > > There does not seem to be any code to forbid "rebasing" when > remotes->nr != 1, though. Did I miss a call to die() earlier? The die() call happens in install_branch_config_multiple_remotes(), where it belongs. I think someone who reads this comment will eventually track down the die() call, but it does look a little out of place. Purely as a matter of personal taste, I would have expected this comment to be in the 'else' clause, and it might read something like: install_branch_config_multiple_remotes() does not allow rebasing with multiple upstream branches. but that's just a suggestion :) This patch looks fine to me. > >> + printf_ln(rebasing ? >> + _("branch '%s' set up to track '%s' by rebasing.") : >> + _("branch '%s' set up to track '%s'."), >> + local, friendly_ref_names.items[0].string); >> } else { >> + printf_ln(_("branch '%s' set up to track:"), local); >> + for_each_string_list_item(item, &friendly_ref_names) >> + printf_ln(" %s", item->string); > > In other words, I would have expected something in this else clause. > >> } > > Thanks.
diff --git a/branch.c b/branch.c index 7a88a4861e..a41a300f9c 100644 --- a/branch.c +++ b/branch.c @@ -49,25 +49,46 @@ 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 of " + "multiple refs when rebasing is requested")); + + /* + * If the new branch is trying to track itself, something has gone + * wrong. Warn the user and don't proceed any further. + */ + 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 +96,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 +117,40 @@ 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); + struct strbuf tmp_ref_name = STRBUF_INIT; + struct string_list friendly_ref_names = STRING_LIST_INIT_DUP; + + for_each_string_list_item(item, remotes) { + shortname = item->string; + skip_prefix(shortname, "refs/heads/", &shortname); + if (origin) { + strbuf_addf(&tmp_ref_name, "%s/%s", + origin, shortname); + string_list_append_nodup( + &friendly_ref_names, + strbuf_detach(&tmp_ref_name, NULL)); + } else { + string_list_append( + &friendly_ref_names, shortname); + } + } + + if (remotes->nr == 1) { + /* + * 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, friendly_ref_names.items[0].string); } 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); + for_each_string_list_item(item, &friendly_ref_names) + printf_ln(" %s", item->string); } + + string_list_clear(&friendly_ref_names, 0); } return 0; @@ -118,14 +159,36 @@ 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 { + advise(" git config --add branch.\"%s\".remote %s", + local, origin ? origin : "."); + 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 | 143 +++++++++++++++++++++++++++++++++------------- t/t3200-branch.sh | 6 +- 2 files changed, 106 insertions(+), 43 deletions(-)