| Message ID | cover.1638859949.git.steadmon@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | branch: inherit tracking configs | expand |
Josh Steadmon <steadmon@google.com> writes: > I've addressed feedback from V4. Since 2/3 reviewers seemed to (at least > slightly) prefer handling multiple upstream branches in the existing > tracking setup, I've gone that direction rather than repurposing the > branch copy code. None of the other issues were controversial. > > In this version, I'd appreciate feedback mainly on patch 1: > * Is the combination of `git_config_set_gently()` + > `git_config_set_multivar_gently() the best way to write multiple > config entries for the same key? IIRC git_config_set_*() is Dscho's brainchild. If he is available to comment, it may be a valuable input. > * Does the reorganization of the BRANCH_CONFIG_VERBOSE output make > things more readable, or less? Should I try to simplify the output > here so that we don't end up with so many translatable variants of the > same message? Let me find time to take a look. > Also, a question specifically for Junio: this will conflict with > gc/branch-recurse-submodules; should I rebase on that, or wait till it > hits next, or just ignore it for now? Can you two work out the preferred plan, taking relative importance, priority, and difficulty between the topics into account, and report to us how you want to proceed and why you chose the route once you are done? Unless the plan you two come up with is outrageously bad, such a decision by stakeholders would be far more acceptable by the community than going by my gut feeling. In short, I'd prefer decentralization ;-) Having said that, I think this one is a simpler topic that is closer to become stable enough than the other one, so it could be that the rebases want to go the other direction. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Can you two work out the preferred plan, taking relative importance, > priority, and difficulty between the topics into account, and report > to us how you want to proceed and why you chose the route once you > are done? > > Unless the plan you two come up with is outrageously bad, such a > decision by stakeholders would be far more acceptable by the > community than going by my gut feeling. In short, I'd prefer > decentralization ;-) > > Having said that, I think this one is a simpler topic that is closer > to become stable enough than the other one, so it could be that the > rebases want to go the other direction. Josh and I have discussed this, and yes, we agree with your assessment. Rebasing my changes on top of this is also easier from a dependency perspective because this series has a very obvious interface that I can use. I'll send a re-roll soon. Thanks!
Hi, On Tue, 7 Dec 2021, Junio C Hamano wrote: > Josh Steadmon <steadmon@google.com> writes: > > > I've addressed feedback from V4. Since 2/3 reviewers seemed to (at least > > slightly) prefer handling multiple upstream branches in the existing > > tracking setup, I've gone that direction rather than repurposing the > > branch copy code. None of the other issues were controversial. > > > > In this version, I'd appreciate feedback mainly on patch 1: > > * Is the combination of `git_config_set_gently()` + > > `git_config_set_multivar_gently() the best way to write multiple > > config entries for the same key? > > IIRC git_config_set_*() is Dscho's brainchild. If he is available > to comment, it may be a valuable input. The `git_config_set_multivar_gently()` function was really only intended to add one key/value pair. Currently, there is no function to add multiple key/value pairs, and while it is slightly wasteful to lock the config multiple times to write a bunch of key/value pairs, it is not the worst in the world for a small use case like this one. So yes, for the moment I would go with the suggested design. One thing you might want to do is to avoid the extra `git_config_set_gently()` before the `for` loop, simply by passing `i == 0 ? 0 : CONFIG_FLAGS_MULTI_REPLACE` as `flags` parameter to the multivar version of the function. But that would optimize for code size rather than for readability, and I would actually prefer the more verbose version. Ciao, Dscho
On 2021.12.10 23:48, Johannes Schindelin wrote: > Hi, > > On Tue, 7 Dec 2021, Junio C Hamano wrote: > > > Josh Steadmon <steadmon@google.com> writes: > > > > > I've addressed feedback from V4. Since 2/3 reviewers seemed to (at least > > > slightly) prefer handling multiple upstream branches in the existing > > > tracking setup, I've gone that direction rather than repurposing the > > > branch copy code. None of the other issues were controversial. > > > > > > In this version, I'd appreciate feedback mainly on patch 1: > > > * Is the combination of `git_config_set_gently()` + > > > `git_config_set_multivar_gently() the best way to write multiple > > > config entries for the same key? > > > > IIRC git_config_set_*() is Dscho's brainchild. If he is available > > to comment, it may be a valuable input. > > The `git_config_set_multivar_gently()` function was really only intended > to add one key/value pair. > > Currently, there is no function to add multiple key/value pairs, and while > it is slightly wasteful to lock the config multiple times to write a bunch > of key/value pairs, it is not the worst in the world for a small use case > like this one. > > So yes, for the moment I would go with the suggested design. > > One thing you might want to do is to avoid the extra > `git_config_set_gently()` before the `for` loop, simply by passing `i == 0 > ? 0 : CONFIG_FLAGS_MULTI_REPLACE` as `flags` parameter to the multivar > version of the function. > > But that would optimize for code size rather than for readability, and I > would actually prefer the more verbose version. Sounds good, thanks for the advice! > Ciao, > Dscho
I've addressed feedback from V4. Since 2/3 reviewers seemed to (at least slightly) prefer handling multiple upstream branches in the existing tracking setup, I've gone that direction rather than repurposing the branch copy code. None of the other issues were controversial. In this version, I'd appreciate feedback mainly on patch 1: * Is the combination of `git_config_set_gently()` + `git_config_set_multivar_gently() the best way to write multiple config entries for the same key? * Does the reorganization of the BRANCH_CONFIG_VERBOSE output make things more readable, or less? Should I try to simplify the output here so that we don't end up with so many translatable variants of the same message? Also, a question specifically for Junio: this will conflict with gc/branch-recurse-submodules; should I rebase on that, or wait till it hits next, or just ignore it for now? Changes since V4: * Add new patch (1/2) to refactor branch.c:install_branch_config() to accept multiple upstream refs * When multiple upstream branches are set in the parent branch, inherit them all, instead of just the first * Break out error string arguments for easier translation * Don't ignore case for values of branch.autosetupmerge * Move reference to git-pull out of usage string for --track into git-branch.txt * Use test_config instead of `git config` in t2027 * Style fixes: add single-quotes around warning string arguments, remove unnecessary braces Changes since V3: * Use branch_get() instead of git_config_get_string() to look up branch configuration. * Remove unnecessary string formatting in new error message in parse-options-cb.c. Josh Steadmon (2): branch: accept multiple upstream branches for tracking branch: add flags and config to inherit tracking Documentation/config/branch.txt | 3 +- Documentation/git-branch.txt | 24 +++-- Documentation/git-checkout.txt | 2 +- Documentation/git-switch.txt | 2 +- branch.c | 169 ++++++++++++++++++++++++-------- branch.h | 3 +- builtin/branch.c | 6 +- builtin/checkout.c | 6 +- config.c | 5 +- parse-options-cb.c | 15 +++ parse-options.h | 2 + t/t2017-checkout-orphan.sh | 7 ++ t/t2027-checkout-track.sh | 23 +++++ t/t2060-switch.sh | 28 ++++++ t/t3200-branch.sh | 33 +++++++ t/t7201-co.sh | 17 ++++ 16 files changed, 289 insertions(+), 56 deletions(-) Range-diff against v4: -: ---------- > 1: ba7d557725 branch: accept multiple upstream branches for tracking 1: 7ad7507f18 ! 2: c7e4af9a36 branch: add flags and config to inherit tracking @@ Documentation/git-branch.txt: This option is only applicable in non-verbose mode +start-point is either a local or remote-tracking branch. Set it to +`inherit` if you want to copy the tracking configuration from the +branch point. +++ ++See linkgit:git-pull[1] and linkgit:git-config[1] for additional discussion on ++how the `branch.<name>.remote` and `branch.<name>.merge` options are used. --no-track:: Do not set up "upstream" configuration, even if the @@ Documentation/git-switch.txt: should result in deletion of the path). details. ## branch.c ## +@@ + + struct tracking { + struct refspec_item spec; +- char *src; ++ struct string_list *srcs; + const char *remote; + int matches; + }; +@@ branch.c: static int find_tracked_branch(struct remote *remote, void *priv) + + if (!remote_find_tracking(remote, &tracking->spec)) { + if (++tracking->matches == 1) { +- tracking->src = tracking->spec.src; ++ string_list_append(tracking->srcs, tracking->spec.src); + tracking->remote = remote->name; + } else { + free(tracking->spec.src); +- FREE_AND_NULL(tracking->src); ++ string_list_clear(tracking->srcs, 0); + } + tracking->spec.src = NULL; + } @@ branch.c: int install_branch_config(int flag, const char *local, const char *origin, const - return -1; + string_list_clear(&remotes, 0); } +static int inherit_tracking(struct tracking *tracking, const char *orig_ref) +{ + const char *bare_ref; + struct branch *branch; ++ int i; + + bare_ref = orig_ref; + skip_prefix(orig_ref, "refs/heads/", &bare_ref); + + branch = branch_get(bare_ref); + if (!branch->remote_name) { -+ warning(_("asked to inherit tracking from %s, but no remote is set"), ++ warning(_("asked to inherit tracking from '%s', but no remote is set"), + bare_ref); + return -1; + } + + if (branch->merge_nr < 1 || !branch->merge_name || !branch->merge_name[0]) { -+ warning(_("asked to inherit tracking from %s, but no merge configuration is set"), ++ warning(_("asked to inherit tracking from '%s', but no merge configuration is set"), + bare_ref); + return -1; + } + + tracking->remote = xstrdup(branch->remote_name); -+ tracking->src = xstrdup(branch->merge_name[0]); ++ for (i = 0; i < branch->merge_nr; i++) ++ string_list_append(tracking->srcs, branch->merge_name[i]); + tracking->matches = 1; + return 0; +} @@ branch.c: int install_branch_config(int flag, const char *local, const char *ori * 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 @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, + enum branch_track track, int quiet) + { + struct tracking tracking; ++ struct string_list tracking_srcs = STRING_LIST_INIT_DUP; + int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE; memset(&tracking, 0, sizeof(tracking)); tracking.spec.dst = (char *)orig_ref; - if (for_each_remote(find_tracked_branch, &tracking)) -+ if (track != BRANCH_TRACK_INHERIT) { ++ tracking.srcs = &tracking_srcs; ++ if (track != BRANCH_TRACK_INHERIT) + for_each_remote(find_tracked_branch, &tracking); -+ } else if (inherit_tracking(&tracking, orig_ref)) ++ else if (inherit_tracking(&tracking, orig_ref)) return; if (!tracking.matches) +@@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref, + die(_("Not tracking: ambiguous information for ref %s"), + orig_ref); + +- if (install_branch_config(config_flags, new_ref, tracking.remote, +- tracking.src ? tracking.src : orig_ref) < 0) ++ if (tracking.srcs->nr < 1) ++ string_list_append(tracking.srcs, orig_ref); ++ if (install_branch_config_multiple_remotes(config_flags, new_ref, tracking.remote, ++ tracking.srcs) < 0) + exit(-1); + +- free(tracking.src); ++ string_list_clear(tracking.srcs, 0); + } + + int read_branch_desc(struct strbuf *buf, const char *branch_name) ## branch.h ## @@ branch.h: enum branch_track { @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix - OPT_SET_INT('t', "track", &track, N_("set up tracking mode (see git-pull(1))"), - BRANCH_TRACK_EXPLICIT), + OPT_CALLBACK_F('t', "track", &track, "direct|inherit", -+ N_("set up tracking mode (see git-pull(1))"), ++ N_("set branch tracking configuration"), + PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, + parse_opt_tracking_mode), OPT_SET_INT_F(0, "set-upstream", &track, N_("do not use"), @@ builtin/checkout.c: static struct option *add_common_switch_branch_options( OPT_STRING(0, "orphan", &opts->new_orphan_branch, N_("new-branch"), N_("new unparented branch")), ## config.c ## -@@ config.c: static int git_default_branch_config(const char *var, const char *value) - if (value && !strcasecmp(value, "always")) { +@@ config.c: static int git_default_i18n_config(const char *var, const char *value) + static int git_default_branch_config(const char *var, const char *value) + { + if (!strcmp(var, "branch.autosetupmerge")) { +- if (value && !strcasecmp(value, "always")) { ++ if (value && !strcmp(value, "always")) { git_branch_track = BRANCH_TRACK_ALWAYS; return 0; -+ } else if (value && !strcasecmp(value, "inherit")) { ++ } else if (value && !strcmp(value, "inherit")) { + git_branch_track = BRANCH_TRACK_INHERIT; + return 0; } @@ parse-options-cb.c: int parse_opt_passthru_argv(const struct option *opt, const + else if (!strcmp(arg, "inherit")) + *(enum branch_track *)opt->value = BRANCH_TRACK_INHERIT; + else -+ return error(_("option `--track' expects \"direct\" or \"inherit\"")); ++ return error(_("option `%s' expects \"%s\" or \"%s\""), ++ "--track", "direct", "inherit"); + + return 0; +} @@ t/t2027-checkout-track.sh: test_expect_success 'checkout --track -b rejects an e +test_expect_success 'checkout --track -b overrides autoSetupMerge=inherit' ' + # Set up tracking config on main -+ git config branch.main.remote origin && -+ git config branch.main.merge refs/heads/main && ++ test_config branch.main.remote origin && ++ test_config branch.main.merge refs/heads/main && + test_config branch.autoSetupMerge inherit && + # With --track=inherit, we copy the tracking config from main + git checkout --track=inherit -b b1 main && base-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee