RFC patchbranch: add "inherit" option for branch.autoSetupMerge

> It can be helpful when creating a new branch to use the existing
> tracking configuration from the branch point. However, there is
> currently not a method to automatically do so.
>
> Teach branch.autoSetupMerge a new "inherit" option. When this is set,
> creating a new branch will cause the tracking configuration to default
> to the configuration of the branch point, if set.

> Josh Steadmon <steadmon@google.com> writes:
> 
> > It can be helpful when creating a new branch to use the existing
> > tracking configuration from the branch point. However, there is
> > currently not a method to automatically do so.
> >
> > Teach branch.autoSetupMerge a new "inherit" option. When this is set,
> > creating a new branch will cause the tracking configuration to default
> > to the configuration of the branch point, if set.
> 
> So, when a new branch N is forked from an existing branch A that
> builds on branch B (which could be a local branch under refs/heads/
> or a remote-tracking branch under refs/remotes/), a plain-vanilla
> auto-setup-merge makes N build on A but with 'inherit', N is marked
> to build on B instead?
> 
> I do not think it is wise to hide this useful feature behind a
> configuration variable.  
> 
> Rather, this should be made available first to users who do not even
> set the configuration and then as a convenience measure, made usable
> via the configuration mechanism as well.
> 
> The current "git branch --track N A" makes N build on A, so perhaps
> "git branch --track=inherit N A" should make N build on whatever A
> builds on.  We may need to give a synonym --track=direct to the
> traditional "build on the original branch that was used to specfy
> the fork point" while doing so.
> 
> And then on top of that, we can add configuration variable handling.
> 
> Depending on the value of branch.autoSetupMerge, "git branch -b" and
> "git checkout -b" would pretend as if "--track" or "--track=inherit"
> were given, or something along that line.  The end result may be the
> same for those who only use the configuration variables, but it
> would give us some flexibility to countermand the configuration from
> the command line.  Those who set branch.autoSetupMerge to 'inherit'
> cannot say "git checkout -b N --track=direct A" to express "With
> this single invocation alone I am making N build on A, even though I
> know I usually make N build on whatever A builds on" if you give
> only the configuration variable.
> 
> Thanks.

> Josh Steadmon <steadmon@google.com> writes:
> 
> > Thanks for the feedback. I've added "--track=direct" and
> > "--track=inherit" flags in V3, which I'll send out shortly. I am a bit
> > skeptical of the value of having "--track=direct" when just "--track"
> > still works, but I'll leave it up to the list to decide.
> 
> Ah, I assumed that with the configuration variable set to inherit,
> you'd make a command line --track to use the upstream of the fork
> point branch as the upstream of the new branch, but if your
> intention is that (1) without --track or --no-track on the command
> line, if configuration is set to inherit, the new branch will track
> the upstream of the original, and (2) with --track on the command
> line, the new branch will track the original without any "inherit"
> magic, then I agree that there is no need for a way to explicitly
> choose between --track={direct,inherit} from the command line.

> The
> choice would be, for those with remote.autosetupmerge=inherit, to
> use --no-track (to not track anything), --track (to track directly
> the original branch), or say nothing (to inherit tracking), and for
> those without that, --no-track (to not track) and --track (to track
> directly) are the same but there is no way (other than "git -c
> var=val") to do "inherited tracking".
> 
> One advantage of allowing to be explicit is that you do not have to
> remember (or know --- if you are visiting somebody else's repository
> to help them out) how remote.autosetupmerge is configured (or
> unconfigured).  From the command line, without having to worry about
> configured values to interfere, you can control which tracking mode
> is used.

> [...]
>  -t::
> ---track::
> +--track [inherit|direct]::
>  	When creating a new branch, set up `branch.<name>.remote` and
> -	`branch.<name>.merge` configuration entries to mark the
> -	start-point branch as "upstream" from the new branch. This
> +	`branch.<name>.merge` configuration entries to set "upstream" tracking
> +	configuration for the new branch. This

> On Sat, Oct 16 2021, Josh Steadmon wrote:
>
>> It can be helpful when creating a new branch to use the existing
>> tracking configuration from the branch point. However, there is
>> currently not a method to automatically do so.
>
> There's no method to get *only* that config, but this use-case is why
> the "-c" option (copy branch) was added.

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Sat, Oct 16 2021, Josh Steadmon wrote:
>>
>>> It can be helpful when creating a new branch to use the existing
>>> tracking configuration from the branch point. However, there is
>>> currently not a method to automatically do so.
>>
>> There's no method to get *only* that config, but this use-case is why
>> the "-c" option (copy branch) was added.
>
> Hmph, I doubt the claim about the original motivation behind "-c",
> but it sure sounds like an interesting point of view.

> The commit at the tip, as well as configurations are copied, which is
> most of the way there, but I suspect that the --track=inherit is
> mostly to be used in the context of "git checkout -b" and the mention
> of "branch" is merely for simplicity of the description of this topic,
> no?  And you cannot say "git checkout --clone-branch original" (yet).

> 
> On Sat, Oct 16 2021, Josh Steadmon wrote:
> 
> > It can be helpful when creating a new branch to use the existing
> > tracking configuration from the branch point. However, there is
> > currently not a method to automatically do so.
> 
> There's no method to get *only* that config, but this use-case is why
> the "-c" option (copy branch) was added.
> 
> I haven't looked at this in any detail, but the seeming lack of mention
> of it in the commit message & docs makes me wonder if you missed that
> that option could do what you wanted (but granted, it does a lot more,
> which maybe you don't want).

> > [...]
> >  -t::
> > ---track::
> > +--track [inherit|direct]::
> >  	When creating a new branch, set up `branch.<name>.remote` and
> > -	`branch.<name>.merge` configuration entries to mark the
> > -	start-point branch as "upstream" from the new branch. This
> > +	`branch.<name>.merge` configuration entries to set "upstream" tracking
> > +	configuration for the new branch. This
> 
> Setting up ".remote" is what --tracke does, but doesn't it make sense
> for such an option to copy over any other config related to that area,
> e.g. also .pushRemote, as a user may have edited it since the creation
> of the copied-from branch?

> Maybe, maybe not. But this & the above comparison with copy makes me
> wonder if we'd be better off with some mode similar to the matching
> regexes "git config", i.e. you could do a "copy" but only on a list of
> matching variables.
> 
> Then the --track mode could just be implemented in terms of that, no?

> +static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
> +{
> +	struct strbuf key = STRBUF_INIT;
> +	char *remote;
> +	const char *bare_ref;
> +
> +	bare_ref = orig_ref;
> +	skip_prefix(orig_ref, "refs/heads/", &bare_ref);
> +
> +	strbuf_addf(&key, "branch.%s.remote", bare_ref);
> +	if (git_config_get_string(key.buf, &remote)) {
> +		warning("branch.autoSetupMerge=inherit, but could not find %s",
> +			key.buf);
> +		strbuf_release(&key);
> +		return 1;
> +	}
> +	tracking->remote = remote;
> +
> +	strbuf_reset(&key);
> +	strbuf_addf(&key, "branch.%s.merge", bare_ref);
> +	if (git_config_get_string(key.buf, &tracking->src)) {
> +		warning("branch.autoSetupMerge=inherit, but could not find %s",
> +			key.buf);
> +		strbuf_release(&key);
> +		return 1;
> +	}
> +
> +	tracking->matches = 1;
> +	strbuf_release(&key);
> +	return 0;
> +}

> I believe that we can get the branch remote via struct branch. Instead
> of reading the config, we could do something along the lines of:
>
>    int *explicit;
>    struct branch *branch = branch_get();
>    char *remote = remote_for_branch(branch, explicit);
>    /* Optionally check explicit if we don't want to fall back to
>     * "origin" */
>
> I'm not sure which is the idiomatic way to get the branch remote, feel
> free to correct me.

> Josh Steadmon <steadmon@google.com> writes:
> 
> > +static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
> > +{
> > +	struct strbuf key = STRBUF_INIT;
> > +	char *remote;
> > +	const char *bare_ref;
> > +
> > +	bare_ref = orig_ref;
> > +	skip_prefix(orig_ref, "refs/heads/", &bare_ref);
> > +
> > +	strbuf_addf(&key, "branch.%s.remote", bare_ref);
> > +	if (git_config_get_string(key.buf, &remote)) {
> > +		warning("branch.autoSetupMerge=inherit, but could not find %s",
> > +			key.buf);
> > +		strbuf_release(&key);
> > +		return 1;
> > +	}
> > +	tracking->remote = remote;
> > +
> > +	strbuf_reset(&key);
> > +	strbuf_addf(&key, "branch.%s.merge", bare_ref);
> > +	if (git_config_get_string(key.buf, &tracking->src)) {
> > +		warning("branch.autoSetupMerge=inherit, but could not find %s",
> > +			key.buf);
> > +		strbuf_release(&key);
> > +		return 1;
> > +	}
> > +
> > +	tracking->matches = 1;
> > +	strbuf_release(&key);
> > +	return 0;
> > +}
> 
> I believe that we can get the branch remote via struct branch. Instead
> of reading the config, we could do something along the lines of:
> 
>    int *explicit;
>    struct branch *branch = branch_get();
>    char *remote = remote_for_branch(branch, explicit);
>    /* Optionally check explicit if we don't want to fall back to
>     * "origin" */
> 
> I'm not sure which is the idiomatic way to get the branch remote, feel
> free to correct me.

> @@ -139,7 +166,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  
>  	memset(&tracking, 0, sizeof(tracking));
>  	tracking.spec.dst = (char *)orig_ref;
> -	if (for_each_remote(find_tracked_branch, &tracking))
> +	if (track != BRANCH_TRACK_INHERIT) {
> +		for_each_remote(find_tracked_branch, &tracking);
> +	} else if (inherit_tracking(&tracking, orig_ref))
>  		return;
>  
>  	if (!tracking.matches)

> I've addressed Glen's feedback from V3. However, this brings up a new
> issue that was not obvious before: "branch.<name>.merge" can be
> specified more than once. On the other hand, the existing tracking setup
> code supports only a single merge entry. For now I'm defaulting to use
> the first merge entry listed in the branch struct, but I'm curious what
> people think the best solution would be. This may be another point in
> favor of Ævar's suggestion to reuse the copy-branch-config machinery.

> +int parse_opt_tracking_mode(const struct option *opt, const char *arg, int unset) {
> +	if (unset)
> +		*(enum branch_track *)opt->value = BRANCH_TRACK_NEVER;
> +	else if (!arg || !strcmp(arg, "direct"))
> +		*(enum branch_track *)opt->value = BRANCH_TRACK_EXPLICIT;
> +	else if (!strcmp(arg, "inherit"))
> +		*(enum branch_track *)opt->value = BRANCH_TRACK_INHERIT;
> +	else
> +		return error(_("option `--track' expects \"direct\" or \"inherit\""));

> Josh Steadmon <steadmon@google.com> writes:
> 
> > I've addressed Glen's feedback from V3. However, this brings up a new
> > issue that was not obvious before: "branch.<name>.merge" can be
> > specified more than once. On the other hand, the existing tracking setup
> > code supports only a single merge entry. For now I'm defaulting to use
> > the first merge entry listed in the branch struct, but I'm curious what
> > people think the best solution would be. This may be another point in
> > favor of Ævar's suggestion to reuse the copy-branch-config machinery.
> 
> Or we can extend "existing tracking setup code" to support multiple
> merge sources.
> 
> How does the "git pull" machinery react to them, by the way?  I
> think the original intention is to support pulling multiple branches
> from the (single) remote configured for the branch with a single
> invocation of "git pull", creating an octopus merge, but does it
> still work, or nobody uses such a crazy curiosity anymore and it was
> once broken and left in non-working state ever since?  What I am
> dreaming here is if we can safely ignore all but one of them, taking
> the usual "last-one-wins" rule, after some transition period.

> > +int parse_opt_tracking_mode(const struct option *opt, const char *arg, int unset) {
> > +	if (unset)
> > +		*(enum branch_track *)opt->value = BRANCH_TRACK_NEVER;
> > +	else if (!arg || !strcmp(arg, "direct"))
> > +		*(enum branch_track *)opt->value = BRANCH_TRACK_EXPLICIT;
> > +	else if (!strcmp(arg, "inherit"))
> > +		*(enum branch_track *)opt->value = BRANCH_TRACK_INHERIT;
> > +	else
> > +		return error(_("option `--track' expects \"direct\" or \"inherit\""));
> 
> According to recent discussion in another thread,
> 
> 	error(_("option '--%s` expects '%s' or '%s'"),
> 		"track", "direct", "inherit");
> 
> would be more translater friendly, as these three words are not
> subject to translation?  I am not sure if it is really worth it,
> though.

> I've addressed Glen's feedback from V3. However, this brings up a new
> issue that was not obvious before: "branch.<name>.merge" can be
> specified more than once. On the other hand, the existing tracking setup
> code supports only a single merge entry. For now I'm defaulting to use
> the first merge entry listed in the branch struct, but I'm curious what
> people think the best solution would be. This may be another point in
> favor of Ævar's suggestion to reuse the copy-branch-config machinery.

> +static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
> +{
> +	const char *bare_ref;
> +	struct branch *branch;
> +
> +	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"),
> +			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"),
> +			bare_ref);

> +		return -1;
> +	}
> +
> +	tracking->remote = xstrdup(branch->remote_name);
> +	tracking->src = xstrdup(branch->merge_name[0]);
> +	tracking->matches = 1;
> +	return 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
> @@ -139,7 +166,9 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  
>  	memset(&tracking, 0, sizeof(tracking));
>  	tracking.spec.dst = (char *)orig_ref;
> -	if (for_each_remote(find_tracked_branch, &tracking))
> +	if (track != BRANCH_TRACK_INHERIT) {
> +		for_each_remote(find_tracked_branch, &tracking);
> +	} else if (inherit_tracking(&tracking, orig_ref))
>  		return;

> @@ -632,8 +632,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		OPT__VERBOSE(&filter.verbose,
>  			N_("show hash and subject, give twice for upstream branch")),
>  		OPT__QUIET(&quiet, N_("suppress informational messages")),
> -		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))"),

> +			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
> +			parse_opt_tracking_mode),
>  		OPT_SET_INT_F(0, "set-upstream", &track, N_("do not use"),
>  			BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN),
>  		OPT_STRING('u', "set-upstream-to", &new_upstream, N_("upstream"), N_("change the upstream info")),
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b5d477919a..45dab414ea 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1532,8 +1532,10 @@ static struct option *add_common_switch_branch_options(
>  {
>  	struct option options[] = {
>  		OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")),
> -		OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info for new branch"),
> -			BRANCH_TRACK_EXPLICIT),
> +		OPT_CALLBACK_F('t', "track",  &opts->track, "direct|inherit",
> +			N_("set up tracking mode (see git-pull(1))"),
> +			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
> +			parse_opt_tracking_mode),
>  		OPT__FORCE(&opts->force, N_("force checkout (throw away local modifications)"),
>  			   PARSE_OPT_NOCOMPLETE),
>  		OPT_STRING(0, "orphan", &opts->new_orphan_branch, N_("new-branch"), N_("new unparented branch")),
> diff --git a/config.c b/config.c
> index cb4a8058bf..4bd5a18faf 100644
> --- a/config.c
> +++ b/config.c
> @@ -1580,6 +1580,9 @@ static int git_default_branch_config(const char *var, const char *value)
>  		if (value && !strcasecmp(value, "always")) {
>  			git_branch_track = BRANCH_TRACK_ALWAYS;
>  			return 0;
> +		} else if (value && !strcasecmp(value, "inherit")) {
> +			git_branch_track = BRANCH_TRACK_INHERIT;
> +			return 0;
>  		}

> +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.autoSetupMerge inherit &&
> +	# With --track=inherit, we copy the tracking config from main
> +	git checkout --track=inherit -b b1 main &&
> +	test_cmp_config origin branch.b1.remote &&
> +	test_cmp_config refs/heads/main branch.b1.merge &&
> +	# With branch.autoSetupMerge=inherit, we do the same
> +	git checkout -b b2 main &&
> +	test_cmp_config origin branch.b2.remote &&
> +	test_cmp_config refs/heads/main branch.b2.merge &&
> +	# But --track overrides this
> +	git checkout --track -b b3 main &&
> +	test_cmp_config . branch.b3.remote &&
> +	test_cmp_config refs/heads/main branch.b3.merge &&
> +	# And --track=direct does as well
> +	git checkout --track=direct -b b4 main &&
> +	test_cmp_config . branch.b4.remote &&
> +	test_cmp_config refs/heads/main branch.b4.merge
> +'
> +

> 
> On Tue, Nov 16 2021, Josh Steadmon wrote:
> 
> > I've addressed Glen's feedback from V3. However, this brings up a new
> > issue that was not obvious before: "branch.<name>.merge" can be
> > specified more than once. On the other hand, the existing tracking setup
> > code supports only a single merge entry. For now I'm defaulting to use
> > the first merge entry listed in the branch struct, but I'm curious what
> > people think the best solution would be. This may be another point in
> > favor of Ævar's suggestion to reuse the copy-branch-config machinery.
> 
> I haven't looked in any detail now at the "should we copy the config?"
> questions. Just some quick comments/nits below:

> > @@ -632,8 +632,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> >  		OPT__VERBOSE(&filter.verbose,
> >  			N_("show hash and subject, give twice for upstream branch")),
> >  		OPT__QUIET(&quiet, N_("suppress informational messages")),
> > -		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))"),
> 
> Hrm, should we say "git help pull" here, on just not reference it at all
> and have a linkgit:git-pull[1]?
> 
> Or maybe git-branch.txt and git-pull.txt should be including a template?
> As we do with Documentation/rev-list-options.txt, then this
> cross-reference wouldn't be needed.

> On 2021.11.19 07:47, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Nov 16 2021, Josh Steadmon wrote:
>> 
>> > I've addressed Glen's feedback from V3. However, this brings up a new
>> > issue that was not obvious before: "branch.<name>.merge" can be
>> > specified more than once. On the other hand, the existing tracking setup
>> > code supports only a single merge entry. For now I'm defaulting to use
>> > the first merge entry listed in the branch struct, but I'm curious what
>> > people think the best solution would be. This may be another point in
>> > favor of Ævar's suggestion to reuse the copy-branch-config machinery.
>> 
>> I haven't looked in any detail now at the "should we copy the config?"
>> questions. Just some quick comments/nits below:
>
> Thanks for the comments. They're all fixed in V5, which I'll be sending
> out soon.
>
> [snip]

>> > @@ -632,8 +632,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>> >  		OPT__VERBOSE(&filter.verbose,
>> >  			N_("show hash and subject, give twice for upstream branch")),
>> >  		OPT__QUIET(&quiet, N_("suppress informational messages")),
>> > -		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))"),
>> 
>> Hrm, should we say "git help pull" here, on just not reference it at all
>> and have a linkgit:git-pull[1]?
>> 
>> Or maybe git-branch.txt and git-pull.txt should be including a template?
>> As we do with Documentation/rev-list-options.txt, then this
>> cross-reference wouldn't be needed.
>
> Yeah, there's nothing really helpful in git-pull(1) about "--track"
> that's easily searchable (i.e. without reading it all straight through),
> so I just removed the pointer in the option help string, add added
> linkgit:git-pull(1) and linkgit:git-config(1) to git-branch.txt.
>
> I briefly looked at writing a common template for both git-branch.txt
> and git-pull.txt but I feel like the git-pull discussion of tracking is
> so spread out in that doc that it would require a significant rewrite to
> make a common template work.

> 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");

> +					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.";

>  		}
> +
> +		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) {

> +	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

> 
> 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.

> > +					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.

> >  		}
> > +
> > +		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..

> > +	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
> 

> 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.

>> > +	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!

>  {
>  	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;

> @@ -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);

> +			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);

> 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.

> @@ -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;

> @@ -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);
> +}

> @@ -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;

> @@ -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);
> +}

> 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`.

>> > @@ -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?

> 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, 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

> +		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);

> 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'.'.

> 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?

>> > 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.

> It can be helpful when creating a new branch to use the existing
> tracking configuration from the branch point. However, there is
> currently not a method to automatically do so.
>
> Teach git-{branch,checkout,switch} an "inherit" argument to the
> "--track" option. When this is set, creating a new branch will cause the
> tracking configuration to default to the configuration of the branch
> point, if set.
>
> For example, if branch "main" tracks "origin/main", and we run
> `git checkout --track=inherit -b feature main`, then branch "feature"
> will track "origin/main". Thus, `git status` will show us how far
> ahead/behind we are from origin, and `git pull` will pull from origin.
>
> This is particularly useful when creating branches across many
> submodules, such as with `git submodule foreach ...` (or if running with
> a patch such as [1], which we use at $job), as it avoids having to
> manually set tracking info for each submodule.
>
> Since we've added an argument to "--track", also add "--track=direct" as
> another way to explicitly get the original "--track" behavior ("--track"
> without an argument still works as well).
> @@ -10,7 +10,8 @@ enum branch_track {
>  	BRANCH_TRACK_REMOTE,
>  	BRANCH_TRACK_ALWAYS,
>  	BRANCH_TRACK_EXPLICIT,
> -	BRANCH_TRACK_OVERRIDE
> +	BRANCH_TRACK_OVERRIDE,
> +	BRANCH_TRACK_INHERIT
>  };

>  
>  extern enum branch_track git_branch_track;
> diff --git a/builtin/branch.c b/builtin/branch.c
> index b23b1d1752..ebde5023c3 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -632,8 +632,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		OPT__VERBOSE(&filter.verbose,
>  			N_("show hash and subject, give twice for upstream branch")),
>  		OPT__QUIET(&quiet, N_("suppress informational messages")),
> -		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 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"),
>  			BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN),

>  		OPT_STRING('u', "set-upstream-to", &new_upstream, N_("upstream"), N_("change the upstream info")),
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b5d477919a..45dab414ea 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1532,8 +1532,10 @@ static struct option *add_common_switch_branch_options(
>  {
>  	struct option options[] = {
>  		OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")),
> -		OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info for new branch"),
> -			BRANCH_TRACK_EXPLICIT),
> +		OPT_CALLBACK_F('t', "track",  &opts->track, "direct|inherit",
> +			N_("set up tracking mode (see git-pull(1))"),
> +			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
> +			parse_opt_tracking_mode),
>  		OPT__FORCE(&opts->force, N_("force checkout (throw away local modifications)"),
>  			   PARSE_OPT_NOCOMPLETE),
>  		OPT_STRING(0, "orphan", &opts->new_orphan_branch, N_("new-branch"), N_("new unparented branch")),

> +	test_must_fail git rev-parse --verify HEAD^ &&
> +	git checkout main &&
> +	git config branch.autosetupmerge inherit &&
> +	git checkout --orphan eta &&
> +	test -z "$(git config branch.eta.merge)" &&
> +	test -z "$(git config branch.eta.remote)" &&

>> @@ -10,7 +10,8 @@ enum branch_track {
>>  	BRANCH_TRACK_REMOTE,
>>  	BRANCH_TRACK_ALWAYS,
>>  	BRANCH_TRACK_EXPLICIT,
>> -	BRANCH_TRACK_OVERRIDE
>> +	BRANCH_TRACK_OVERRIDE,
>> +	BRANCH_TRACK_INHERIT
>>  };
>
> So we've got 5 items in this enum...
>
>>  
>>  extern enum branch_track git_branch_track;
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index b23b1d1752..ebde5023c3 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -632,8 +632,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>  		OPT__VERBOSE(&filter.verbose,
>>  			N_("show hash and subject, give twice for upstream branch")),
>>  		OPT__QUIET(&quiet, N_("suppress informational messages")),
>> -		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 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"),
>>  			BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN),
>
> But map --track, --track=direct --track=inherit to 3/5 of them. Will it
> ever make sense to do the oher 2/5 (I really haven't checked)....

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> >> @@ -10,7 +10,8 @@ enum branch_track {
> >>  	BRANCH_TRACK_REMOTE,
> >>  	BRANCH_TRACK_ALWAYS,
> >>  	BRANCH_TRACK_EXPLICIT,
> >> -	BRANCH_TRACK_OVERRIDE
> >> +	BRANCH_TRACK_OVERRIDE,
> >> +	BRANCH_TRACK_INHERIT
> >>  };
> >
> > So we've got 5 items in this enum...
> >
> >>  
> >>  extern enum branch_track git_branch_track;
> >> diff --git a/builtin/branch.c b/builtin/branch.c
> >> index b23b1d1752..ebde5023c3 100644
> >> --- a/builtin/branch.c
> >> +++ b/builtin/branch.c
> >> @@ -632,8 +632,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> >>  		OPT__VERBOSE(&filter.verbose,
> >>  			N_("show hash and subject, give twice for upstream branch")),
> >>  		OPT__QUIET(&quiet, N_("suppress informational messages")),
> >> -		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 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"),
> >>  			BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN),
> >
> > But map --track, --track=direct --track=inherit to 3/5 of them. Will it
> > ever make sense to do the oher 2/5 (I really haven't checked)....
> 
> Reasonable question, but I believe the answer is no, it doesn't make
> sense to map all the values:
> 
> * BRANCH_TRACK_REMOTE is just a default value as far as I can tell (I
>   don't think this does anything?)
> * BRANCH_TRACK_ALWAYS behaves like BRANCH_TRACK_EXPLICIT but it's only
>   meant to be set from config files, see 9ed36cfa35 (branch: optionally
>   setup branch.*.merge from upstream local branches, 2008-02-19). We're
>   more lenient with _ALWAYS than with _EXPLICIT; e.g. we don't die()
>   when the upstream doesn't exist.
> 
> Even one of the other options doesn't really make that much sense...
> 
> * BRANCH_TRACK_OVERRIDE used to be used to implement --set-upstream, but
>   that's not necessary any more. Now it's used to make create_branch()
>   *not* create a branch sometimes, but that's going away if I get my
>   refactor of create_branch()
>   (https://lore.kernel.org/git/xmqq1r2pcnyw.fsf@gitster.g/T/#u) :)

> 
> On Mon, Dec 06 2021, Josh Steadmon wrote:
> 
> > It can be helpful when creating a new branch to use the existing
> > tracking configuration from the branch point. However, there is
> > currently not a method to automatically do so.
> >
> > Teach git-{branch,checkout,switch} an "inherit" argument to the
> > "--track" option. When this is set, creating a new branch will cause the
> > tracking configuration to default to the configuration of the branch
> > point, if set.
> >
> > For example, if branch "main" tracks "origin/main", and we run
> > `git checkout --track=inherit -b feature main`, then branch "feature"
> > will track "origin/main". Thus, `git status` will show us how far
> > ahead/behind we are from origin, and `git pull` will pull from origin.
> >
> > This is particularly useful when creating branches across many
> > submodules, such as with `git submodule foreach ...` (or if running with
> > a patch such as [1], which we use at $job), as it avoids having to
> > manually set tracking info for each submodule.
> >
> > Since we've added an argument to "--track", also add "--track=direct" as
> > another way to explicitly get the original "--track" behavior ("--track"
> > without an argument still works as well).
> > @@ -10,7 +10,8 @@ enum branch_track {
> >  	BRANCH_TRACK_REMOTE,
> >  	BRANCH_TRACK_ALWAYS,
> >  	BRANCH_TRACK_EXPLICIT,
> > -	BRANCH_TRACK_OVERRIDE
> > +	BRANCH_TRACK_OVERRIDE,
> > +	BRANCH_TRACK_INHERIT
> >  };
> 
> So we've got 5 items in this enum...
> 
> >  
> >  extern enum branch_track git_branch_track;
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > index b23b1d1752..ebde5023c3 100644
> > --- a/builtin/branch.c
> > +++ b/builtin/branch.c
> > @@ -632,8 +632,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> >  		OPT__VERBOSE(&filter.verbose,
> >  			N_("show hash and subject, give twice for upstream branch")),
> >  		OPT__QUIET(&quiet, N_("suppress informational messages")),
> > -		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 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"),
> >  			BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN),
> 
> But map --track, --track=direct --track=inherit to 3/5 of them. Will it
> ever make sense to do the oher 2/5 (I really haven't checked)....
> 
> >  		OPT_STRING('u', "set-upstream-to", &new_upstream, N_("upstream"), N_("change the upstream info")),
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index b5d477919a..45dab414ea 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -1532,8 +1532,10 @@ static struct option *add_common_switch_branch_options(
> >  {
> >  	struct option options[] = {
> >  		OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")),
> > -		OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info for new branch"),
> > -			BRANCH_TRACK_EXPLICIT),
> > +		OPT_CALLBACK_F('t', "track",  &opts->track, "direct|inherit",
> > +			N_("set up tracking mode (see git-pull(1))"),
> > +			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
> > +			parse_opt_tracking_mode),
> >  		OPT__FORCE(&opts->force, N_("force checkout (throw away local modifications)"),
> >  			   PARSE_OPT_NOCOMPLETE),
> >  		OPT_STRING(0, "orphan", &opts->new_orphan_branch, N_("new-branch"), N_("new unparented branch")),
> 
> I wonder if this interface wouldn't be a lot simpler as:
> 
>     --track
>     --track-explicit --track-direct --track-inherit
> 
> Both because it'll work better for auto-complete, and we can (and
> presumably some will want) just make --track mean whatever configured
> --track-THING you want.

> >  	if (!strcmp(var, "branch.autosetupmerge")) {
> > -		if (value && !strcasecmp(value, "always")) {
> > +		if (value && !strcmp(value, "always")) {
> 
> ...This probably makes sense, but it seems like the behavior change of
> "let's not take this case-insensitive" should be split up into its own
> change...

> > +	test_must_fail git rev-parse --verify HEAD^ &&
> > +	git checkout main &&
> > +	git config branch.autosetupmerge inherit &&
> > +	git checkout --orphan eta &&
> > +	test -z "$(git config branch.eta.merge)" &&
> > +	test -z "$(git config branch.eta.remote)" &&
> 
> Better with the test_must_be_empty etc. helpers.
> 
> > +	test refs/heads/eta = "$(git symbolic-ref HEAD)" &&
> 
> and this with test_cmp.
> 
> (ditto occurances below)

> diff --git a/branch.h b/branch.h
> index df0be61506..6484bda8a2 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -10,7 +10,8 @@ enum branch_track {
>  	BRANCH_TRACK_REMOTE,
>  	BRANCH_TRACK_ALWAYS,
>  	BRANCH_TRACK_EXPLICIT,
> -	BRANCH_TRACK_OVERRIDE
> +	BRANCH_TRACK_OVERRIDE,
> +	BRANCH_TRACK_INHERIT
>  };

>  			git_branch_track = BRANCH_TRACK_ALWAYS;
>  			return 0;
> +		} else if (value && !strcmp(value, "inherit")) {
> +			git_branch_track = BRANCH_TRACK_INHERIT;
> +			return 0;
>  		}
>  		git_branch_track = git_config_bool(var, value);
>  		return 0;

> Josh Steadmon <steadmon@google.com> writes:
> 
> >  --no-track::
> >  	Do not set up "upstream" configuration, even if the
> > -	branch.autoSetupMerge configuration variable is true.
> > +	branch.autoSetupMerge configuration variable is set.
> 
> I guess "inherit" is different from "true".
> Nice to see an attention to the details.  
> 
> > diff --git a/branch.h b/branch.h
> > index df0be61506..6484bda8a2 100644
> > --- a/branch.h
> > +++ b/branch.h
> > @@ -10,7 +10,8 @@ enum branch_track {
> >  	BRANCH_TRACK_REMOTE,
> >  	BRANCH_TRACK_ALWAYS,
> >  	BRANCH_TRACK_EXPLICIT,
> > -	BRANCH_TRACK_OVERRIDE
> > +	BRANCH_TRACK_OVERRIDE,
> > +	BRANCH_TRACK_INHERIT
> >  };
> 
> Unless INHERIT must stay to be at the end of the enumeration even
> when we add more of these, let's leave a common at the end to help
> future developers.

> > -		if (value && !strcasecmp(value, "always")) {
> > +		if (value && !strcmp(value, "always")) {
> 
> This is belatedly correcting the mistake we made 5 years ago, which
> can break existing users who used "[branch]autosetupmerge=Always" in
> their configuration files.
> 
> I'm OK to fix it to accept only lowercase as written here, see if
> anybody screams, and tell them that the all-lowercase is the only
> documented way to spell this word.
> 
> >  			git_branch_track = BRANCH_TRACK_ALWAYS;
> >  			return 0;
> > +		} else if (value && !strcmp(value, "inherit")) {
> > +			git_branch_track = BRANCH_TRACK_INHERIT;
> > +			return 0;
> >  		}
> >  		git_branch_track = git_config_bool(var, value);
> >  		return 0;
> 
> Thanks.

> +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"),
> +			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"),
> +			bare_ref);
> +		return -1;
> +	}
> +
> +	tracking->remote = xstrdup(branch->remote_name);
> +	for (i = 0; i < branch->merge_nr; i++)
> +		string_list_append(tracking->srcs, branch->merge_name[i]);
> +	tracking->matches = 1;
> +	return 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
> @@ -189,11 +218,15 @@ 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))
> +	tracking.srcs = &tracking_srcs;
> +	if (track != BRANCH_TRACK_INHERIT)
> +		for_each_remote(find_tracked_branch, &tracking);
> +	else if (inherit_tracking(&tracking, orig_ref))
>  		return;
>  
>  	if (!tracking.matches)

>@@ -210,11 +243,13 @@ 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);
> }

> Josh Steadmon <steadmon@google.com> writes:
> 
> > +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"),
> > +			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"),
> > +			bare_ref);
> > +		return -1;
> > +	}
> > +
> > +	tracking->remote = xstrdup(branch->remote_name);
> > +	for (i = 0; i < branch->merge_nr; i++)
> > +		string_list_append(tracking->srcs, branch->merge_name[i]);
> > +	tracking->matches = 1;
> > +	return 0;
> > +}
> 
> tracking->matches is used to keep track of the number of matched remote
> refs. I believe we set tracking->matches = 1 to fulfill two specific
> conditions in setup_tracking()...
> 
> > +
> >  /*
> >   * 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
> > @@ -189,11 +218,15 @@ 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))
> > +	tracking.srcs = &tracking_srcs;
> > +	if (track != BRANCH_TRACK_INHERIT)
> > +		for_each_remote(find_tracked_branch, &tracking);
> > +	else if (inherit_tracking(&tracking, orig_ref))
> >  		return;
> >  
> >  	if (!tracking.matches)
> 
> *extra context*
> 	if (!tracking.matches)
> 		switch (track) {
> 		case BRANCH_TRACK_ALWAYS:
> 		case BRANCH_TRACK_EXPLICIT:
> 		case BRANCH_TRACK_OVERRIDE:
> 			break;
> 		default:
> 			return;
> 		}
> 
> First, tracking.matches > 0, because we want to do work if there are
> branches to track.
> 
> Secondly,
> 
> *extra context*
> 	if (tracking.matches > 1)
> 		die(_("Not tracking: ambiguous information for ref %s"),
> 		    orig_ref);
> 
> tracking.matches <= 1, because we don't want to set up tracking if it's
> not obvious what ref we want to track.
> 
> But as I understand it, BRANCH_TRACK_INHERIT should be unconditional, so
> instead of fudging this behavior by setting the correct value for
> tracking.matches (which is meant for matching remote refs), we can just
> do what the other unconditional BRANCH_TRACK_* options do, which is to
> to break instead of return, i.e.
> 
> 	if (!tracking.matches)
> 		switch (track) {
> 		case BRANCH_TRACK_ALWAYS:
> 		case BRANCH_TRACK_EXPLICIT:
> 		case BRANCH_TRACK_OVERRIDE:
> +   case BRANCH_TRACK_INHERIT:
> 			break;
> 		default:
> 			return;
> 		}
> 
> and BRANCH_TRACK_INHERIT won't have to pretend that tracking.matches is
> meaningful to it.

> >@@ -210,11 +243,13 @@ 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);
> > }
> 
> It looks like install_branch_config_multiple_remotes() can just replace
> install_branch_config() in setup_tracking(), nice. This should make it
> pretty easy for me to rebase gc/branch-recurse-submodules onto this.

> 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?

> 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 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.

> 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.

> + * `origin` is the name of the remote owning the upstream branches. NULL means
> + * the upstream branches are local to this repo.
> + *
> + * `remotes` is a list of refs that are upstream of local
> + */
> +static int install_branch_config_multiple_remotes(int flag, const char *local,
> +		const char *origin, struct string_list *remotes)
>  {
>  	const char *shortname = NULL;
>  	struct strbuf key = STRBUF_INIT;
> +	struct string_list_item *item;
>  	int rebasing = should_setup_rebase(origin);
>  
> -	if (skip_prefix(remote, "refs/heads/", &shortname)
> -	    && !strcmp(local, shortname)
> -	    && !origin) {
> -		warning(_("Not setting branch %s as its own upstream."),
> -			local);
> -		return 0;
> -	}
> +	if (!remotes->nr)
> +		BUG("must provide at least one remote for branch config");
> +	if (rebasing && remotes->nr > 1)
> +		die(_("cannot inherit upstream tracking configuration when rebasing is requested"));
> +
> +	if (!origin)
> +		for_each_string_list_item(item, remotes)
> +			if (skip_prefix(item->string, "refs/heads/", &shortname)
> +			    && !strcmp(local, shortname)) {
> +				warning(_("not setting branch '%s' as its own upstream."),
> +					local);
> +				return 0;
> +			}

> -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
> -'
> +"

> --- a/branch.c
> +++ b/branch.c
> @@ -49,25 +49,41 @@ static int should_setup_rebase(const char *origin)
>  	return 0;
>  }
>  
> -static const char tracking_advice[] =
> -N_("\n"
> -"After fixing the error cause you may try to fix up\n"
> -"the remote tracking information by invoking\n"
> -"\"git branch --set-upstream-to=%s%s%s\".");
> -
> -int install_branch_config(int flag, const char *local, const char *origin, const char *remote)
> +/**
> + * Install upstream tracking configuration for a branch; specifically, add
> + * `branch.<name>.remote` and `branch.<name>.merge` entries.
> + *
> + * `flag` contains integer flags for options; currently only
> + * BRANCH_CONFIG_VERBOSE is checked.
> + *
> + * `local` is the name of the branch whose configuration we're installing.
> + *
> + * `origin` is the name of the remote owning the upstream branches. NULL means
> + * the upstream branches are local to this repo.
> + *
> + * `remotes` is a list of refs that are upstream of local
> + */
> +static int install_branch_config_multiple_remotes(int flag, const char *local,
> +		const char *origin, struct string_list *remotes)

>  {
>  	const char *shortname = NULL;
>  	struct strbuf key = STRBUF_INIT;
> +	struct string_list_item *item;
>  	int rebasing = should_setup_rebase(origin);
>  
> -	if (skip_prefix(remote, "refs/heads/", &shortname)
> -	    && !strcmp(local, shortname)
> -	    && !origin) {
> -		warning(_("Not setting branch %s as its own upstream."),
> -			local);
> -		return 0;
> -	}
> +	if (!remotes->nr)
> +		BUG("must provide at least one remote for branch config");
> +	if (rebasing && remotes->nr > 1)
> +		die(_("cannot inherit upstream tracking configuration when rebasing is requested"));

> @@ -75,8 +91,17 @@ int install_branch_config(int flag, const char *local, const char *origin, const
>  
>  	strbuf_reset(&key);
>  	strbuf_addf(&key, "branch.%s.merge", local);
> -	if (git_config_set_gently(key.buf, remote) < 0)
> +	/*
> +	 * We want to overwrite any existing config with all the branches in
> +	 * "remotes". Override any existing config, then write our branches. If
> +	 * more than one is provided, use CONFIG_REGEX_NONE to preserve what
> +	 * we've written so far.
> +	 */
> +	if (git_config_set_gently(key.buf, NULL) < 0)
>  		goto out_err;
> +	for_each_string_list_item(item, remotes)
> +		if (git_config_set_multivar_gently(key.buf, item->string, CONFIG_REGEX_NONE, 0) < 0)
> +			goto out_err;

> @@ -87,29 +112,42 @@ int install_branch_config(int flag, const char *local, const char *origin, const
>  	strbuf_release(&key);
>  
>  	if (flag & BRANCH_CONFIG_VERBOSE) {
> -		if (shortname) {
> +		const char *name;
> +		struct strbuf ref_string = STRBUF_INIT;
> +
> +		for_each_string_list_item(item, remotes) {
> +			name = item->string;
> +			skip_prefix(name, "refs/heads/", &name);
> +			strbuf_addf(&ref_string, "  %s\n", name);
> +		}
> +
> +		if (remotes->nr == 1) {
> +			struct strbuf refname = STRBUF_INIT;
> +
>  			if (origin)
> -				printf_ln(rebasing ?
> -					  _("Branch '%s' set up to track remote branch '%s' from '%s' by rebasing.") :
> -					  _("Branch '%s' set up to track remote branch '%s' from '%s'."),
> -					  local, shortname, origin);
> -			else
> -				printf_ln(rebasing ?
> -					  _("Branch '%s' set up to track local branch '%s' by rebasing.") :
> -					  _("Branch '%s' set up to track local branch '%s'."),
> -					  local, shortname);
> +				strbuf_addf(&refname, "%s/", origin);
> +			strbuf_addstr(&refname, remotes->items[0].string);
> +
> +			/*
> +			 * Rebasing is only allowed in the case of a single
> +			 * upstream branch.
> +			 */
> +			printf_ln(rebasing ?
> +				_("branch '%s' set up to track '%s' by rebasing.") :
> +				_("branch '%s' set up to track '%s'."),
> +				local, refname.buf);
> +
> +			strbuf_release(&refname);
> +		} else if (origin) {
> +			printf_ln(_("branch '%s' set up to track from '%s':"),
> +				local, origin);
> +			printf("%s", ref_string.buf);

>  		} else {
> -			if (origin)
> -				printf_ln(rebasing ?
> -					  _("Branch '%s' set up to track remote ref '%s' by rebasing.") :
> -					  _("Branch '%s' set up to track remote ref '%s'."),
> -					  local, remote);
> -			else
> -				printf_ln(rebasing ?
> -					  _("Branch '%s' set up to track local ref '%s' by rebasing.") :
> -					  _("Branch '%s' set up to track local ref '%s'."),
> -					  local, remote);
> +			printf_ln(_("branch '%s' set up to track:"), local);
> +			printf("%s", ref_string.buf);

> @@ -118,14 +156,33 @@ int install_branch_config(int flag, const char *local, const char *origin, const
>  	strbuf_release(&key);
>  	error(_("Unable to write upstream branch configuration"));
>  
> -	advise(_(tracking_advice),
> -	       origin ? origin : "",
> -	       origin ? "/" : "",
> -	       shortname ? shortname : remote);
> +	advise(_("\nAfter fixing the error cause you may try to fix up\n"
> +		"the remote tracking information by invoking:"));
> +	if (remotes->nr == 1)
> +		advise("  git branch --set-upstream-to=%s%s%s",
> +			origin ? origin : "",
> +			origin ? "/" : "",
> +			remotes->items[0].string);
> +	else
> +		for_each_string_list_item(item, remotes)
> +			advise("  git config --add branch.\"%s\".merge %s",
> +				local, item->string);

> Josh Steadmon <steadmon@google.com> writes:
> 
> >  {
> >  	const char *shortname = NULL;
> >  	struct strbuf key = STRBUF_INIT;
> > +	struct string_list_item *item;
> >  	int rebasing = should_setup_rebase(origin);
> >  
> > -	if (skip_prefix(remote, "refs/heads/", &shortname)
> > -	    && !strcmp(local, shortname)
> > -	    && !origin) {
> > -		warning(_("Not setting branch %s as its own upstream."),
> > -			local);
> > -		return 0;
> > -	}
> > +	if (!remotes->nr)
> > +		BUG("must provide at least one remote for branch config");
> > +	if (rebasing && remotes->nr > 1)
> > +		die(_("cannot inherit upstream tracking configuration when rebasing is requested"));
> 
> Nit: if we're being pedantic, we cannot inherit upstream tracking
> configuration when rebasing is requested with multiple upstream
> branches.
> 
> But this message is already very niche and loaded with specifics, so
> adding "...with multiple upstream branches" might just be more
> confusing, so this is a nit.

> > @@ -87,29 +112,42 @@ int install_branch_config(int flag, const char *local, const char *origin, const
> >  	strbuf_release(&key);
> >  
> >  	if (flag & BRANCH_CONFIG_VERBOSE) {
> > -		if (shortname) {
> > +		const char *name;
> > +		struct strbuf ref_string = STRBUF_INIT;
> > +
> > +		for_each_string_list_item(item, remotes) {
> > +			name = item->string;
> > +			skip_prefix(name, "refs/heads/", &name);
> > +			strbuf_addf(&ref_string, "  %s\n", name);
> > +		}
> > +
> > +		if (remotes->nr == 1) {
> > +			struct strbuf refname = STRBUF_INIT;
> > +
> >  			if (origin)
> > -				printf_ln(rebasing ?
> > -					  _("Branch '%s' set up to track remote branch '%s' from '%s' by rebasing.") :
> > -					  _("Branch '%s' set up to track remote branch '%s' from '%s'."),
> > -					  local, shortname, origin);
> > -			else
> > -				printf_ln(rebasing ?
> > -					  _("Branch '%s' set up to track local branch '%s' by rebasing.") :
> > -					  _("Branch '%s' set up to track local branch '%s'."),
> > -					  local, shortname);
> > +				strbuf_addf(&refname, "%s/", origin);
> > +			strbuf_addstr(&refname, remotes->items[0].string);
> > +
> > +			/*
> > +			 * Rebasing is only allowed in the case of a single
> > +			 * upstream branch.
> > +			 */
> > +			printf_ln(rebasing ?
> > +				_("branch '%s' set up to track '%s' by rebasing.") :
> > +				_("branch '%s' set up to track '%s'."),
> > +				local, refname.buf);
> > +
> > +			strbuf_release(&refname);
> > +		} else if (origin) {
> > +			printf_ln(_("branch '%s' set up to track from '%s':"),
> > +				local, origin);
> > +			printf("%s", ref_string.buf);
> 
> It's not clear to me why the hint contains the word 'from' when it is a
> remote ref...

> >  		} else {
> > -			if (origin)
> > -				printf_ln(rebasing ?
> > -					  _("Branch '%s' set up to track remote ref '%s' by rebasing.") :
> > -					  _("Branch '%s' set up to track remote ref '%s'."),
> > -					  local, remote);
> > -			else
> > -				printf_ln(rebasing ?
> > -					  _("Branch '%s' set up to track local ref '%s' by rebasing.") :
> > -					  _("Branch '%s' set up to track local ref '%s'."),
> > -					  local, remote);
> > +			printf_ln(_("branch '%s' set up to track:"), local);
> > +			printf("%s", ref_string.buf);
> 
> but does not have the word 'from' when it is a local ref. As far as I
> can tell, this is the only difference between remote and local refs, and
> adding the word 'from' does not seem like a good enough reason to add an
> 'if' condition. Maybe I missed something here?
> 
> This motivates my answer to the question you asked in [1]:
> 
>   I removed as many distinctions as possible, as most can still be
>   inferred from context. [...] Likewise, we don't need to specify whether
>   refs are remote or local: "some-remote/some-branch" vs.
>   "a-local-branch" should be understandable without us spelling it out.
> 
> I agree that there is adequate context, so I would be ok with the
> simplification if there was corresponding code simplification e.g.
> dropping "if (origin)". But in its current form, I don't think there is
> good enough reason to simplify the message.

> Of course, IIUC, this is as simple as dropping 'from' in the "if
> (origin)" case.
> 
> > @@ -118,14 +156,33 @@ int install_branch_config(int flag, const char *local, const char *origin, const
> >  	strbuf_release(&key);
> >  	error(_("Unable to write upstream branch configuration"));
> >  
> > -	advise(_(tracking_advice),
> > -	       origin ? origin : "",
> > -	       origin ? "/" : "",
> > -	       shortname ? shortname : remote);
> > +	advise(_("\nAfter fixing the error cause you may try to fix up\n"
> > +		"the remote tracking information by invoking:"));
> > +	if (remotes->nr == 1)
> > +		advise("  git branch --set-upstream-to=%s%s%s",
> > +			origin ? origin : "",
> > +			origin ? "/" : "",
> > +			remotes->items[0].string);
> > +	else
> > +		for_each_string_list_item(item, remotes)
> > +			advise("  git config --add branch.\"%s\".merge %s",
> > +				local, item->string);
> 
> The advice is now correct, nice! Does the user also need to run "git
> config --add branch.%s.remote %s" ?
> 
> [1] https://lore.kernel.org/git/Ybk6QsMdeBl6IweW@google.com

>> > @@ -87,29 +112,42 @@ int install_branch_config(int flag, const char *local, const char *origin, const
>> >  	strbuf_release(&key);
>> >  
>> >  	if (flag & BRANCH_CONFIG_VERBOSE) {
>> > -		if (shortname) {
>> > +		const char *name;
>> > +		struct strbuf ref_string = STRBUF_INIT;
>> > +
>> > +		for_each_string_list_item(item, remotes) {
>> > +			name = item->string;
>> > +			skip_prefix(name, "refs/heads/", &name);
>> > +			strbuf_addf(&ref_string, "  %s\n", name);
>> > +		}
>> > +
>> > +		if (remotes->nr == 1) {
>> > +			struct strbuf refname = STRBUF_INIT;
>> > +
>> >  			if (origin)
>> > -				printf_ln(rebasing ?
>> > -					  _("Branch '%s' set up to track remote branch '%s' from '%s' by rebasing.") :
>> > -					  _("Branch '%s' set up to track remote branch '%s' from '%s'."),
>> > -					  local, shortname, origin);
>> > -			else
>> > -				printf_ln(rebasing ?
>> > -					  _("Branch '%s' set up to track local branch '%s' by rebasing.") :
>> > -					  _("Branch '%s' set up to track local branch '%s'."),
>> > -					  local, shortname);
>> > +				strbuf_addf(&refname, "%s/", origin);
>> > +			strbuf_addstr(&refname, remotes->items[0].string);
>> > +
>> > +			/*
>> > +			 * Rebasing is only allowed in the case of a single
>> > +			 * upstream branch.
>> > +			 */
>> > +			printf_ln(rebasing ?
>> > +				_("branch '%s' set up to track '%s' by rebasing.") :
>> > +				_("branch '%s' set up to track '%s'."),
>> > +				local, refname.buf);
>> > +
>> > +			strbuf_release(&refname);
>> > +		} else if (origin) {
>> > +			printf_ln(_("branch '%s' set up to track from '%s':"),
>> > +				local, origin);
>> > +			printf("%s", ref_string.buf);
>> 
>> It's not clear to me why the hint contains the word 'from' when it is a
>> remote ref...
>
> Because in the multiple-branch case, we don't prepend the origin to each
> ref, so we need to let users know which remote the refs are coming from.

>> >  		} else {
>> > -			if (origin)
>> > -				printf_ln(rebasing ?
>> > -					  _("Branch '%s' set up to track remote ref '%s' by rebasing.") :
>> > -					  _("Branch '%s' set up to track remote ref '%s'."),
>> > -					  local, remote);
>> > -			else
>> > -				printf_ln(rebasing ?
>> > -					  _("Branch '%s' set up to track local ref '%s' by rebasing.") :
>> > -					  _("Branch '%s' set up to track local ref '%s'."),
>> > -					  local, remote);
>> > +			printf_ln(_("branch '%s' set up to track:"), local);
>> > +			printf("%s", ref_string.buf);
>> 
>> but does not have the word 'from' when it is a local ref. As far as I
>> can tell, this is the only difference between remote and local refs, and
>> adding the word 'from' does not seem like a good enough reason to add an
>> 'if' condition. Maybe I missed something here?
>> 
>> This motivates my answer to the question you asked in [1]:
>> 
>>   I removed as many distinctions as possible, as most can still be
>>   inferred from context. [...] Likewise, we don't need to specify whether
>>   refs are remote or local: "some-remote/some-branch" vs.
>>   "a-local-branch" should be understandable without us spelling it out.
>> 
>> I agree that there is adequate context, so I would be ok with the
>> simplification if there was corresponding code simplification e.g.
>> dropping "if (origin)". But in its current form, I don't think there is
>> good enough reason to simplify the message.
>
> I think the proper point of comparison is not the original code, but the
> code from V5 where we try to preserve the same level of detail in output
> as the original code. If we are committed to both having multiple
> remotes and keeping similar styles of output as the original
> implementation, then something like the massive conditional in V5 is
> unavoidable.

> Josh Steadmon <steadmon@google.com> writes:
> 
> >> > @@ -87,29 +112,42 @@ int install_branch_config(int flag, const char *local, const char *origin, const
> >> >  	strbuf_release(&key);
> >> >  
> >> >  	if (flag & BRANCH_CONFIG_VERBOSE) {
> >> > -		if (shortname) {
> >> > +		const char *name;
> >> > +		struct strbuf ref_string = STRBUF_INIT;
> >> > +
> >> > +		for_each_string_list_item(item, remotes) {
> >> > +			name = item->string;
> >> > +			skip_prefix(name, "refs/heads/", &name);
> >> > +			strbuf_addf(&ref_string, "  %s\n", name);
> >> > +		}
> >> > +
> >> > +		if (remotes->nr == 1) {
> >> > +			struct strbuf refname = STRBUF_INIT;
> >> > +
> >> >  			if (origin)
> >> > -				printf_ln(rebasing ?
> >> > -					  _("Branch '%s' set up to track remote branch '%s' from '%s' by rebasing.") :
> >> > -					  _("Branch '%s' set up to track remote branch '%s' from '%s'."),
> >> > -					  local, shortname, origin);
> >> > -			else
> >> > -				printf_ln(rebasing ?
> >> > -					  _("Branch '%s' set up to track local branch '%s' by rebasing.") :
> >> > -					  _("Branch '%s' set up to track local branch '%s'."),
> >> > -					  local, shortname);
> >> > +				strbuf_addf(&refname, "%s/", origin);
> >> > +			strbuf_addstr(&refname, remotes->items[0].string);
> >> > +
> >> > +			/*
> >> > +			 * Rebasing is only allowed in the case of a single
> >> > +			 * upstream branch.
> >> > +			 */
> >> > +			printf_ln(rebasing ?
> >> > +				_("branch '%s' set up to track '%s' by rebasing.") :
> >> > +				_("branch '%s' set up to track '%s'."),
> >> > +				local, refname.buf);
> >> > +
> >> > +			strbuf_release(&refname);
> >> > +		} else if (origin) {
> >> > +			printf_ln(_("branch '%s' set up to track from '%s':"),
> >> > +				local, origin);
> >> > +			printf("%s", ref_string.buf);
> >> 
> >> It's not clear to me why the hint contains the word 'from' when it is a
> >> remote ref...
> >
> > Because in the multiple-branch case, we don't prepend the origin to each
> > ref, so we need to let users know which remote the refs are coming from.
> 
> I see. So if I'm reading this correctly, the error message in the remote
> case would read something like:
> 
>   branch 'main' set up to track from 'origin':
>     main
>     topic1
>     topic2
> 
> Is there any reason why we couldn't append the origin to the ref to make
> it consistent? I think this could be as simple as:
> 
> 
> 	for_each_string_list_item(item, remotes) {
> 		name = item->string;
> 		skip_prefix(name, "refs/heads/", &name);
> 			if (origin)
> +         strbuf_addf(&ref_string, "%s/", origin);
> 		strbuf_addf(&ref_string, "  %s\n", name);
> 	}
> 
> and the resulting list could look like:
> 
>   branch 'main' set up to track from 'origin':
>     origin/main
>     origin/topic1
>     origin/topic2
> 
> This looks repetitive, but I suggest this because, as I understand it,
> we are omitting the "{local,remote} ref" phrase based on conventions
> around ref names, like "origin/main" is probably a remote ref and not an
> oddly named local ref. However, when we print the list like so,
> 
>   branch 'main' set up to track from 'origin':
>     main
>     topic1
>     topic2
> 
> we now expect the user to understand that 'main', 'topic1' and 'topic2'
> to implicitly have 'origin/' prepended to them. This behavior seems
> inconsistent to me; I'd anticipate most users responding "Wait, I was
> supposed to be tracking 'origin' branches right? Why am I looking at
> local branches?". Some users would be able to recover because they can
> figure out what we mean, but others might just give up.
> 
> Prepending 'origin/' would get rid of this problem altogether, and it
> would let us drop the 'from'.

> >> >  		} else {
> >> > -			if (origin)
> >> > -				printf_ln(rebasing ?
> >> > -					  _("Branch '%s' set up to track remote ref '%s' by rebasing.") :
> >> > -					  _("Branch '%s' set up to track remote ref '%s'."),
> >> > -					  local, remote);
> >> > -			else
> >> > -				printf_ln(rebasing ?
> >> > -					  _("Branch '%s' set up to track local ref '%s' by rebasing.") :
> >> > -					  _("Branch '%s' set up to track local ref '%s'."),
> >> > -					  local, remote);
> >> > +			printf_ln(_("branch '%s' set up to track:"), local);
> >> > +			printf("%s", ref_string.buf);
> >> 
> >> but does not have the word 'from' when it is a local ref. As far as I
> >> can tell, this is the only difference between remote and local refs, and
> >> adding the word 'from' does not seem like a good enough reason to add an
> >> 'if' condition. Maybe I missed something here?
> >> 
> >> This motivates my answer to the question you asked in [1]:
> >> 
> >>   I removed as many distinctions as possible, as most can still be
> >>   inferred from context. [...] Likewise, we don't need to specify whether
> >>   refs are remote or local: "some-remote/some-branch" vs.
> >>   "a-local-branch" should be understandable without us spelling it out.
> >> 
> >> I agree that there is adequate context, so I would be ok with the
> >> simplification if there was corresponding code simplification e.g.
> >> dropping "if (origin)". But in its current form, I don't think there is
> >> good enough reason to simplify the message.
> >
> > I think the proper point of comparison is not the original code, but the
> > code from V5 where we try to preserve the same level of detail in output
> > as the original code. If we are committed to both having multiple
> > remotes and keeping similar styles of output as the original
> > implementation, then something like the massive conditional in V5 is
> > unavoidable.
> 
> I see. So for instance, post-simplification you have:
> 
>   printf_ln(rebasing ?
>     _("branch '%s' set up to track '%s' by rebasing.") :
>     _("branch '%s' set up to track '%s'."),
>     local, refname.buf);
> 
> if you preserve the same amount of detail as before, you'd have to
> distinguish between local/remote, which doubles the number of cases to
> 4, which is why the conditional v5 is so complicated.
> 
> That said, I think that it's already much simpler than v5 because you've
> split the singular and plural cases. I wonder if you have considered
> building the final string purely from format strings, like:
> 
>   char *message_format = _("branch %s set up to track %s%s%s%s");
>   char *ref_type_clause = origin ? " remote ref " : " local ref ";
>   char *rebasing_clause = rebasing ? " by rebasing." : ".";
>   char *branch_names = "<branch names>";
>   printf_ln(message_format, local, ref_type_clause, branch_names, rebasing_clause);
> 
> This sounds potentially unfriendly to i18n, but it would make the
> conditional simpler. What do you think?

> @@ -192,11 +220,15 @@ 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))
> +	tracking.srcs = &tracking_srcs;
> +	if (track != BRANCH_TRACK_INHERIT)
> +		for_each_remote(find_tracked_branch, &tracking);
> +	else if (inherit_tracking(&tracking, orig_ref))
>  		return;
>  
>  	if (!tracking.matches)
> @@ -204,6 +236,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  		case BRANCH_TRACK_ALWAYS:
>  		case BRANCH_TRACK_EXPLICIT:
>  		case BRANCH_TRACK_OVERRIDE:
> +		case BRANCH_TRACK_INHERIT:
>  			break;
>  		default:
>  			return;
> @@ -213,11 +246,13 @@ 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 && track != BRANCH_TRACK_INHERIT)
> +		string_list_append(tracking.srcs, orig_ref);

> diff --git a/t/t2027-checkout-track.sh b/t/t2027-checkout-track.sh
> index 4453741b96..49c7def21c 100755
> --- a/t/t2027-checkout-track.sh
> +++ b/t/t2027-checkout-track.sh
> @@ -24,4 +24,27 @@ test_expect_success 'checkout --track -b rejects an extra path argument' '
>  	test_i18ngrep "cannot be used with updating paths" err
>  '
>  
> +test_expect_success 'checkout --track -b overrides autoSetupMerge=inherit' '
> +	# Set up tracking config on 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 &&
> +	test_cmp_config origin branch.b1.remote &&
> +	test_cmp_config refs/heads/main branch.b1.merge &&
> +	# With branch.autoSetupMerge=inherit, we do the same
> +	git checkout -b b2 main &&
> +	test_cmp_config origin branch.b2.remote &&
> +	test_cmp_config refs/heads/main branch.b2.merge &&
> +	# But --track overrides this
> +	git checkout --track -b b3 main &&
> +	test_cmp_config . branch.b3.remote &&
> +	test_cmp_config refs/heads/main branch.b3.merge &&
> +	# And --track=direct does as well
> +	git checkout --track=direct -b b4 main &&
> +	test_cmp_config . branch.b4.remote &&
> +	test_cmp_config refs/heads/main branch.b4.merge

> Josh Steadmon <steadmon@google.com> writes:
> 
> > @@ -192,11 +220,15 @@ 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))
> > +	tracking.srcs = &tracking_srcs;
> > +	if (track != BRANCH_TRACK_INHERIT)
> > +		for_each_remote(find_tracked_branch, &tracking);
> > +	else if (inherit_tracking(&tracking, orig_ref))
> >  		return;
> >  
> >  	if (!tracking.matches)
> > @@ -204,6 +236,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
> >  		case BRANCH_TRACK_ALWAYS:
> >  		case BRANCH_TRACK_EXPLICIT:
> >  		case BRANCH_TRACK_OVERRIDE:
> > +		case BRANCH_TRACK_INHERIT:
> >  			break;
> >  		default:
> >  			return;
> > @@ -213,11 +246,13 @@ 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 && track != BRANCH_TRACK_INHERIT)
> > +		string_list_append(tracking.srcs, orig_ref);
> 
> So, in the BRANCH_TRACK_{ALWAYS,EXPLICIT,OVERRIDE} cases, we append
> orig_ref because we expect orig_ref to be a local ref that the caller
> wants to track. This is not the case with BRANCH_TRACK_INHERIT, where we
> want to inherit the configuration and we no longer care about orig_ref.
> 
> This is correct, though it's more unobvious than what I originally
> envisioned when I commented on [1]. As a small nit, it might benefit
> from a clarifying comment, but this is fine as it is :)

> > diff --git a/t/t2027-checkout-track.sh b/t/t2027-checkout-track.sh
> > index 4453741b96..49c7def21c 100755
> > --- a/t/t2027-checkout-track.sh
> > +++ b/t/t2027-checkout-track.sh
> > @@ -24,4 +24,27 @@ test_expect_success 'checkout --track -b rejects an extra path argument' '
> >  	test_i18ngrep "cannot be used with updating paths" err
> >  '
> >  
> > +test_expect_success 'checkout --track -b overrides autoSetupMerge=inherit' '
> > +	# Set up tracking config on 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 &&
> > +	test_cmp_config origin branch.b1.remote &&
> > +	test_cmp_config refs/heads/main branch.b1.merge &&
> > +	# With branch.autoSetupMerge=inherit, we do the same
> > +	git checkout -b b2 main &&
> > +	test_cmp_config origin branch.b2.remote &&
> > +	test_cmp_config refs/heads/main branch.b2.merge &&
> > +	# But --track overrides this
> > +	git checkout --track -b b3 main &&
> > +	test_cmp_config . branch.b3.remote &&
> > +	test_cmp_config refs/heads/main branch.b3.merge &&
> > +	# And --track=direct does as well
> > +	git checkout --track=direct -b b4 main &&
> > +	test_cmp_config . branch.b4.remote &&
> > +	test_cmp_config refs/heads/main branch.b4.merge
> 
> Nit: in both cases, the expected result is that branch.b*.merge is
> "refs/heads/main". so the difference between --track=direct and
> --track=inherit would be more obvious if main tracked something other
> than origin/main.

> As an side, the comments in the tests make it really readable :)
> 
> Overall this patch looks good.
> 
> [1] https://lore.kernel.org/git/kl6lfsr3c3j7.fsf@chooglen-macbookpro.roam.corp.google.com

> Changes since V5:
> * Greatly simplified BRANCH_CONFIG_VERBOSE output to not require nearly
>   so many conditionals.
> * Note that rebasing is not compatible with inheriting multiple upstream
>   branches.
> * Moved the change to case-sensitivity for branch.autosetupmerge to its
>   own commit.
> * Improve advice on failed tracking setup when multiple branches are
>   involved.
> * Make better use of string_list API.
> * Make better use of config API.
> * More straight-forward use of the `struct tracking` API.
> * Numerous style fixes.

> Josh Steadmon <steadmon@google.com> writes:
>
>> Changes since V5:
>> * Greatly simplified BRANCH_CONFIG_VERBOSE output to not require nearly
>>   so many conditionals.
>> * Note that rebasing is not compatible with inheriting multiple upstream
>>   branches.
>> * Moved the change to case-sensitivity for branch.autosetupmerge to its
>>   own commit.
>> * Improve advice on failed tracking setup when multiple branches are
>>   involved.
>> * Make better use of string_list API.
>> * Make better use of config API.
>> * More straight-forward use of the `struct tracking` API.
>> * Numerous style fixes.
>
> I've queued this, and rebased Glen's "branch --recurse-submodules"
> on top, and parked both of them near the tip of 'seen'.  I do not
> have much confidence in the conflict resolution needed during the
> rebasing or the other branch or merges into 'seen', and I would
> appreciate it if you two can take a look to sanity check the result.
>
> Thanks.

> Changes since V6:
> * Strip the refs/heads/ prefix in the verbose output when we have only a
>   single upstream branch.
> * Improve the fatal error message to note that rebasing is only
>   incompatible with multiple upstream refs.
> * Also note that `branch.<name>.remote` should be set in the manual
>   recovery advice.
> * Simplify the logic in setup_tracking() when no tracking sources match.
> * Make the difference in test cases in t2027 more obvious.
>
> Changes since V5:
> * Greatly simplified BRANCH_CONFIG_VERBOSE output to not require nearly
>   so many conditionals.
> * Note that rebasing is not compatible with inheriting multiple upstream
>   branches.
> * Moved the change to case-sensitivity for branch.autosetupmerge to its
>   own commit.
> * Improve advice on failed tracking setup when multiple branches are
>   involved.
> * Make better use of string_list API.
> * Make better use of config API.
> * More straight-forward use of the `struct tracking` API.
> * Numerous style fixes.
>
> 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 (3):
>   branch: accept multiple upstream branches for tracking
>   branch: add flags and config to inherit tracking
>   config: require lowercase for branch.*.autosetupmerge
>
>  Documentation/config/branch.txt |   3 +-
>  Documentation/git-branch.txt    |  24 ++--
>  Documentation/git-checkout.txt  |   2 +-
>  Documentation/git-switch.txt    |   2 +-
>  branch.c                        | 189 ++++++++++++++++++++++++--------
>  branch.h                        |   3 +-
>  builtin/branch.c                |   6 +-
>  builtin/checkout.c              |   6 +-
>  config.c                        |   5 +-
>  parse-options-cb.c              |  16 +++
>  parse-options.h                 |   2 +
>  t/t2017-checkout-orphan.sh      |  11 +-
>  t/t2027-checkout-track.sh       |  23 ++++
>  t/t2060-switch.sh               |  28 +++++
>  t/t3200-branch.sh               |  39 ++++++-
>  t/t7201-co.sh                   |  17 +++
>  16 files changed, 310 insertions(+), 66 deletions(-)
>
> Range-diff against v6:
> 1:  43d6f83fed ! 1:  9152367ba9 branch: accept multiple upstream branches for tracking
>     @@ branch.c: static int should_setup_rebase(const char *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 when rebasing is requested"));
>     ++		die(_("cannot inherit upstream tracking configuration of "
>     ++		      "multiple refs when rebasing is requested"));
>      +
>      +	if (!origin)
>      +		for_each_string_list_item(item, remotes)
>     @@ branch.c: int install_branch_config(int flag, const char *local, const char *ori
>      -					  _("Branch '%s' set up to track local branch '%s'."),
>      -					  local, shortname);
>      +				strbuf_addf(&refname, "%s/", origin);
>     -+			strbuf_addstr(&refname, remotes->items[0].string);
>     ++			skip_prefix(remotes->items[0].string, "refs/heads/", &name);
>     ++			strbuf_addstr(&refname, name);
>      +
>      +			/*
>      +			 * Rebasing is only allowed in the case of a single
>     @@ branch.c: int install_branch_config(int flag, const char *local, const char *ori
>      +			origin ? origin : "",
>      +			origin ? "/" : "",
>      +			remotes->items[0].string);
>     -+	else
>     ++	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;
>       }
> 2:  57e57e6e6a ! 2:  afeb84539e branch: add flags and config to inherit tracking
>     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,
>       
>      -	if (install_branch_config(config_flags, new_ref, tracking.remote,
>      -			      tracking.src ? tracking.src : orig_ref) < 0)
>     -+	if (tracking.srcs->nr < 1 && track != BRANCH_TRACK_INHERIT)
>     ++	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)
>     ++	if (install_branch_config_multiple_remotes(config_flags, new_ref,
>     ++				tracking.remote, tracking.srcs) < 0)
>       		exit(-1);
>       
>      -	free(tracking.src);
>     @@ 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
>      +	test_config branch.main.remote origin &&
>     -+	test_config branch.main.merge refs/heads/main &&
>     ++	test_config branch.main.merge refs/heads/some-branch &&
>      +	test_config branch.autoSetupMerge inherit &&
>      +	# With --track=inherit, we copy the tracking config from main
>      +	git checkout --track=inherit -b b1 main &&
>      +	test_cmp_config origin branch.b1.remote &&
>     -+	test_cmp_config refs/heads/main branch.b1.merge &&
>     ++	test_cmp_config refs/heads/some-branch branch.b1.merge &&
>      +	# With branch.autoSetupMerge=inherit, we do the same
>      +	git checkout -b b2 main &&
>      +	test_cmp_config origin branch.b2.remote &&
>     -+	test_cmp_config refs/heads/main branch.b2.merge &&
>     ++	test_cmp_config refs/heads/some-branch branch.b2.merge &&
>      +	# But --track overrides this
>      +	git checkout --track -b b3 main &&
>      +	test_cmp_config . branch.b3.remote &&
> 3:  f79d27dc24 = 3:  a818a6561b config: require lowercase for branch.*.autosetupmerge
>
> base-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee
> -- 
> 2.34.1.173.g76aa8bc2d0-goog

> +/**
> + * 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;
> +			}

> @@ -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;

> @@ -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.
> +			 */

> +			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);

>> +/**
>> + * 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 ;-)

>> @@ -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.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 4b0ef35913..a049276439 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
>  
> +test_expect_success 'tracking info copied with --track=inherit' '
> +	git branch --track=inherit foo2 my1 &&
> +	test_cmp_config local branch.foo2.remote &&
> +	test_cmp_config refs/heads/main branch.foo2.merge
> +'
> +
> +test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
> +	test_unconfig branch.autoSetupMerge &&
> +	# default config does not copy tracking info
> +	git branch foo-no-inherit my1 &&
> +	test_cmp_config "" --default "" branch.foo-no-inherit.remote &&
> +	test_cmp_config "" --default "" branch.foo-no-inherit.merge &&
> +	# with autoSetupMerge=inherit, we copy tracking info from my1
> +	test_config branch.autoSetupMerge inherit &&
> +	git branch foo3 my1 &&
> +	test_cmp_config local branch.foo3.remote &&
> +	test_cmp_config refs/heads/main branch.foo3.merge &&
> +	# no tracking info to inherit from main
> +	git branch main2 main &&
> +	test_cmp_config "" --default "" branch.main2.remote &&
> +	test_cmp_config "" --default "" branch.main2.merge
> +'
> +
> +test_expect_success '--track overrides branch.autoSetupMerge' '
> +	test_config branch.autoSetupMerge inherit &&
> +	git branch --track=direct foo4 my1 &&
> +	test_cmp_config . branch.foo4.remote &&
> +	test_cmp_config refs/heads/my1 branch.foo4.merge &&
> +	git branch --no-track foo5 my1 &&
> +	test_cmp_config "" --default "" branch.foo5.remote &&
> +	test_cmp_config "" --default "" branch.foo5.merge
> +'
> +
>  test_done
> diff --git a/t/t7201-co.sh b/t/t7201-co.sh
> index 7f6e23a4bb..4fdf88ba46 100755
> --- a/t/t7201-co.sh
> +++ b/t/t7201-co.sh
> @@ -657,4 +657,21 @@ test_expect_success 'custom merge driver with checkout -m' '
>  	test_cmp expect arm
>  '
>  
> +test_expect_success 'tracking info copied with autoSetupMerge=inherit' '
> +	git reset --hard main &&
> +	# default config does not copy tracking info
> +	git checkout -b foo-no-inherit koala/bear &&
> +	test_cmp_config "" --default "" branch.foo-no-inherit.remote &&
> +	test_cmp_config "" --default "" branch.foo-no-inherit.merge &&
> +	# with autoSetupMerge=inherit, we copy tracking info from koala/bear
> +	test_config branch.autoSetupMerge inherit &&
> +	git checkout -b foo koala/bear &&
> +	test_cmp_config origin branch.foo.remote &&
> +	test_cmp_config refs/heads/koala/bear branch.foo.merge &&
> +	# no tracking info to inherit from main
> +	git checkout -b main2 main &&
> +	test_cmp_config "" --default "" branch.main2.remote &&
> +	test_cmp_config "" --default "" branch.main2.merge
> +'
> +
>  test_done
> -- 
> 2.34.1.307.g9b7440fafd-goog

> Since we've added an argument to "--track", also add "--track=direct" as
> another way to explicitly get the original "--track" behavior ("--track"
> without an argument still works as well).
> [...]
> -'git branch' [--track | --no-track] [-f] <branchname> [<start-point>]
> +'git branch' [--track [direct|inherit] | --no-track] [-f] <branchname> [<start-point>]

> -		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 branch tracking configuration"),
> +			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
> +			parse_opt_tracking_mode),

>  	struct option options[] = {
>  		OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")),
> -		OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info for new branch"),
> -			BRANCH_TRACK_EXPLICIT),
> +		OPT_CALLBACK_F('t', "track",  &opts->track, "direct|inherit",

> As Ævar pointed out in [1], the use of PARSE_OPT_LITERAL_ARGHELP with a
> list of allowed parameters is not recommended. Both git-branch and
> git-checkout were changed in d311566 (branch: add flags and config to
> inherit tracking, 2021-12-20) to use this discouraged combination for
> their --track flags.

> The help strings for arguments are enclosed in angle brackets
> automatically.  E.g. if argh is specified as "name", "--option <name>"
> is printed, to indicate that users need to supply an actual name.  The
> flag PARSE_OPT_LITERAL_ARGHELP turns this off, so that "--option name"
> is printed instead, to indicate that the literal string needs to be
> supplied -- a rare case.
>
> This flag is enabled automatically if argh contains special characters
> like brackets.  The developer is supposed to provide any required angle
> brackets for more complicated cases.  E.g. if argh is "<start>,<end>"
> then "--option <start>,<end>" is printed.
>
> Add a comment to mention this PARSE_OPT_LITERAL_ARGHELP behavior.
>
> Also remove the flag from option definitions for which it's inferred
> automatically.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Somehow I feel this is not enough, but I can't pin down what's
> missing.

> The help strings for arguments are enclosed in angle brackets
> automatically.  E.g. if argh is specified as "name", "--option <name>"
> is printed, to indicate that users need to supply an actual name.  The
> flag PARSE_OPT_LITERAL_ARGHELP turns this off, so that "--option name"
> is printed instead, to indicate that the literal string needs to be
> supplied -- a rare case.
>
> This flag is enabled automatically if argh contains special characters
> like brackets.  The developer is supposed to provide any required angle
> brackets for more complicated cases.  E.g. if argh is "<start>,<end>"
> then "--option <start>,<end>" is printed.

>   *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets
>   *				(i.e. '<argh>') in the help message.
>   *				Useful for options with multiple parameters.
> + *				Automatically enabled if argh contains any
> + *				of the following characters: ()<>[]|
>   *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
>   *			   by git-completion.bash. This option suppresses that.
>   *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to

> René Scharfe <l.s.r@web.de> writes:
>
>> The help strings for arguments are enclosed in angle brackets
>> automatically.  E.g. if argh is specified as "name", "--option <name>"
>> is printed, to indicate that users need to supply an actual name.  The
>> flag PARSE_OPT_LITERAL_ARGHELP turns this off, so that "--option name"
>> is printed instead, to indicate that the literal string needs to be
>> supplied -- a rare case.
>>
>> This flag is enabled automatically if argh contains special characters
>> like brackets.  The developer is supposed to provide any required angle
>> brackets for more complicated cases.  E.g. if argh is "<start>,<end>"
>> then "--option <start>,<end>" is printed.
>
> The above does explain why we want to have this change very well,
> but at least some of it would help those who are reading the comment
> we touch.
>
>> Add a comment to mention this PARSE_OPT_LITERAL_ARGHELP behavior.
>
> But instead, the addition is only about when the flag bit is turned
> on automatically.  Without understanding your
>
>     E.g. if argh is specified as "name", "--option <name>" is
>     printed, to indicate that users need to supply an actual name.
>
> readers would not quite get from the current description "says that
> argh shouldn't be enclosed in brackets" when/why it is a good idea
> to add the option, I am afraid.
>
>> Also remove the flag from option definitions for which it's inferred
>> automatically.
>
> I am not sure if this is a good move.
>
> Because these places explicitly gave PARSE_OPT_LITERAL_ARGHELP, it
> was easy to grep for them when I was trying to find an existing
> practice.
>
> Imagine, after we remove these redundant flags, we see a patch that
> wants to change the set of characters that automatically flips the
> flag bit on.  It is clear from the patch text why it helps one
> particular OPT_STRING() or whatever the same patch adds, but how
> would you make sure it will not regress _existing_ OPT_WHATEVER()
> that do not use PARSE_OPT_LITERAL_ARGHELP because their argh happens
> to use the character that wasn't special before?

>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> Somehow I feel this is not enough, but I can't pin down what's
>> missing.
>
> Let me try.
>
>>   *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets
>>   *				(i.e. '<argh>') in the help message.
>>   *				Useful for options with multiple parameters.
>> + *				Automatically enabled if argh contains any
>> + *				of the following characters: ()<>[]|
>>   *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
>>   *			   by git-completion.bash. This option suppresses that.
>>   *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to
>
> PARSE_OPT_LITERAL_ARGHELP: in short-help given by "git cmd -h", the
> 			   "argh" member of the struct option is
> 			   shown in a pair of angle brackets (e.g.
> 			   "--option=<argh>"); this flag tells the
> 			   machinery not to add these brackets, to
> 			   give more control to the developer.  E.g.
> 			   "<start>,<end>" given to argh is shown as
> 			   "--option=<start>,<end>".
>
> That may be a bit too much, but on the other hand, among PARSE_OPT_X
> descriptions, this is the only one that needs to talk about help
> text on the argument to the option.
>
> Or we can flip it the other way, perhaps?
>
> 	Tell the machinery to give "argh" member literally in the
> 	short-help in "git cmd -h" output for the option.  E.g. {
> 	.argh = "(diff|raw)", .long_name = "show" } would give
> 	"--show=(diff|raw)".  Without the flag, "argh" is enclosed
> 	in a pair of angle brackets.
>
> I dunno.

> Now that I read the whole comment, I think the right place to introduce
> the automatic brackets is the description of argh some lines up.
>
> --- >8 ---
> Subject: [PATCH 5/5] parse-options: document bracketing of argh
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  parse-options.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/parse-options.h b/parse-options.h
> index e22846d3b7..88d589d159 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -85,6 +85,11 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
>   *   token to explain the kind of argument this option wants. Does not
>   *   begin in capital letter, and does not end with a full stop.
>   *   Should be wrapped by N_() for translation.
> + *   Is automatically enclosed in brackets when printed, unless it
> + *   contains any of the following characters: ()<>[]|
> + *   E.g. "name" is shown as "<name>" to indicate that a name value
> + *   needs to be supplied, not the literal string "name", but
> + *   "<start>,<end>" and "(this|that)" are printed verbatim.
>   *
>   * `help`::
>   *   the short help associated to what the option does.

> René Scharfe <l.s.r@web.de> writes:
>
>> Now that I read the whole comment, I think the right place to introduce
>> the automatic brackets is the description of argh some lines up.
>>
>> --- >8 ---
>> Subject: [PATCH 5/5] parse-options: document bracketing of argh
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  parse-options.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/parse-options.h b/parse-options.h
>> index e22846d3b7..88d589d159 100644
>> --- a/parse-options.h
>> +++ b/parse-options.h
>> @@ -85,6 +85,11 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
>>   *   token to explain the kind of argument this option wants. Does not
>>   *   begin in capital letter, and does not end with a full stop.
>>   *   Should be wrapped by N_() for translation.
>> + *   Is automatically enclosed in brackets when printed, unless it
>> + *   contains any of the following characters: ()<>[]|
>> + *   E.g. "name" is shown as "<name>" to indicate that a name value
>> + *   needs to be supplied, not the literal string "name", but
>> + *   "<start>,<end>" and "(this|that)" are printed verbatim.
>>   *
>>   * `help`::
>>   *   the short help associated to what the option does.
>
> Very nice.
>
> This version gives the necessary information in (almost) one place.
>
> I said (almost) because "it contains any of ..." is not the only way
> to decline the <automatic angle brackets>, and am wondering if it is
> more helpful to say something like
>
> 	... when printed, unless PARSE_OPT_LITERAL_ARGHELP is set in
> 	the flags, or it contains any of the following characters: ...
>
> and then shrink the description of the flag bit to
>
>     PARSE_OPT_LITERAL_ARGHELP: controls if `argh` is enclosed in in
>     brackets when shown (see the description on `argh` above).
>

> The idea was to document the base behavior at the top and the effects of
> flags at the bottom.  Blurring this distinction and cross-referencing
> gives a better whole, though, I agree.  It would have helped me find the
> right place to put the list of special chars, for one thing.  So how
> about this?
>
> --- >8 ---
> Subject: [PATCH v2] parse-options: document bracketing of argh
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  parse-options.h | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/parse-options.h b/parse-options.h
> index e22846d3b7..2e801b3c9a 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -85,6 +85,11 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
>   *   token to explain the kind of argument this option wants. Does not
>   *   begin in capital letter, and does not end with a full stop.
>   *   Should be wrapped by N_() for translation.
> + *   Is enclosed in brackets when printed, unless PARSE_OPT_LITERAL_ARGHELP
> + *   is set in `flags` or it contains any of the following characters: ()<>[]|
> + *   E.g. "name" is shown as "<name>" to indicate that a name value
> + *   needs to be supplied, not the literal string "name", but
> + *   "<start>,<end>" and "(this|that)" are printed verbatim.

>   *
>   * `help`::
>   *   the short help associated to what the option does.
> @@ -105,9 +110,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
>   *				not last it will require an argument.
>   *				Should not be used with PARSE_OPT_OPTARG.
>   *   PARSE_OPT_NODASH: this option doesn't start with a dash.
> - *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets
> - *				(i.e. '<argh>') in the help message.
> - *				Useful for options with multiple parameters.
> + *   PARSE_OPT_LITERAL_ARGHELP: says that `argh` shouldn't be enclosed in
> + *				brackets (see `argh` description above).
>   *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
>   *			   by git-completion.bash. This option suppresses that.
>   *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to
> --
> 2.34.1

> As Ævar pointed out in [1], the use of PARSE_OPT_LITERAL_ARGHELP with a
> list of allowed parameters is not recommended. Both git-branch and
> git-checkout were changed in d311566 (branch: add flags and config to
> inherit tracking, 2021-12-20) to use this discouraged combination for
> their --track flags.
>
> Fix this by removing PARSE_OPT_LITERAL_ARGHELP, and changing the arghelp
> to simply be "mode". Users may discover allowed values in the manual
> pages.
>
> [1]: https://lore.kernel.org/git/220111.86a6g3yqf9.gmgdl@evledraar.gmail.com/
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  builtin/branch.c   | 4 ++--
>  builtin/checkout.c | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 2251e6a54f..0c8d8a8827 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -638,9 +638,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		OPT__VERBOSE(&filter.verbose,
>  			N_("show hash and subject, give twice for upstream branch")),
>  		OPT__QUIET(&quiet, N_("suppress informational messages")),
> -		OPT_CALLBACK_F('t', "track",  &track, "direct|inherit",
> +		OPT_CALLBACK_F('t', "track",  &track, N_("mode"),
>  			N_("set branch tracking configuration"),
> -			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
> +			PARSE_OPT_OPTARG,
>  			parse_opt_tracking_mode),
>  		OPT_SET_INT_F(0, "set-upstream", &track, N_("do not use"),
>  			BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN),
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 94814c37b4..6a5dd2a2a2 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1549,9 +1549,9 @@ static struct option *add_common_switch_branch_options(
>  {
>  	struct option options[] = {
>  		OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")),
> -		OPT_CALLBACK_F('t', "track",  &opts->track, "direct|inherit",
> -			N_("set up tracking mode (see git-pull(1))"),
> -			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
> +		OPT_CALLBACK_F('t', "track",  &opts->track, N_("mode"),
> +			N_("set branch tracking configuration"),
> +			PARSE_OPT_OPTARG,
>  			parse_opt_tracking_mode),
>  		OPT__FORCE(&opts->force, N_("force checkout (throw away local modifications)"),
>  			   PARSE_OPT_NOCOMPLETE),
>
> base-commit: b56bd95bbc8f716cb8cbb5fdc18b9b0f00323c6a

> On Jan 20 2022, Ævar Arnfjörð Bjarmason wrote:
>
>> With that we'll now emit:
>>     
>>     $ ./git add -h 2>&1|grep chmod
>>         --chmod[=](+|-)x      override the executable bit of the listed files
>
> That looks like --chmod+x is valid, which isn't.

> With that we'll now emit:
>     
>     $ ./git add -h 2>&1|grep chmod
>         --chmod[=](+|-)x      override the executable bit of the listed files
> ...
> But the usage output stated that the "=" was mandatory before.

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> With that we'll now emit:
>>     
>>     $ ./git add -h 2>&1|grep chmod
>>         --chmod[=](+|-)x      override the executable bit of the listed files
>> ...
>> But the usage output stated that the "=" was mandatory before.
>
> I am not sure if it is healthy to be _that_ strict when interpreting
> the boilerplate elements in the output.  Between "git add -h" that
> gives
>
>     (1) git add --chmod( |=)(+|-)x
>     (2) git add --chmod=(+|-)x
>
> I would prefer the latter 10x as much as the former.  The choice
> "You can give either plus or minus" is very much what the reader
> must understand and it is worth reminding in the help.  Compared to
> that, "You can use the stuck form that is recommended by gitcli
> documentation when giving the argument to the --chmod option, or you
> can give the argument to the option as a separate command line
> argument", while technically correct, is not a choice that is worth
> cluttering the output and making it harder to read.
>
> To put it differently, the choice (+|-) is something the user needs
> to pick correctly to make what they want to happen happen.  On the
> other hand, the choice ( |=) is not.  As this is a boilerplate
> choice that is shared by any and all options that take an argument,
> once you are aware that stuck form is recommended but that separate
> form is also accepted, you'd see "git add --chmod=blah" in the help
> and would not hesitate to type "git add --chmod blah".  And if you
> are not aware of the existence of the alternative, nothing is lost.
> You can type '=' and see what you want to see happen happen.
>
> Not cluttering the help text with an extra choice that the user does
> not have to make has a value.

> Also, with the sh completion if you do "git add --chm<TAB>" it expands
> it to "git add --chmod=", i.e. the cursor is left after the "=" that's
> not shown in the "git add -h". So always including it would be
> consistent with that.
>     
>     diff --git a/parse-options.c b/parse-options.c
>     index a8283037be9..75c345bb738 100644
>     --- a/parse-options.c
>     +++ b/parse-options.c
>     @@ -916,7 +916,7 @@ static int usage_argh(const struct option *opts, FILE *outfile)
>                     else
>                             s = literal ? "[%s]" : "[<%s>]";
>             else
>     -               s = literal ? " %s" : " <%s>";
>     +               s = literal ? "=%s" : "=<%s>";

>
> On Mon, Dec 20 2021, Josh Steadmon wrote:
>
>> Since we've added an argument to "--track", also add "--track=direct" as
>> another way to explicitly get the original "--track" behavior ("--track"
>> without an argument still works as well).
>> [...]
>> -'git branch' [--track | --no-track] [-f] <branchname> [<start-point>]
>> +'git branch' [--track [direct|inherit] | --no-track] [-f] <branchname> [<start-point>]
>
> The usage info here is correct...

>
>> ---track::
>> +--track [inherit|direct]::
>
> ...as is this...

>
>> -		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 branch tracking configuration"),
>> +			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>> +			parse_opt_tracking_mode),
> ....
>
>>  	struct option options[] = {
>>  		OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")),
>> -		OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info for new branch"),
>> -			BRANCH_TRACK_EXPLICIT),
>> +		OPT_CALLBACK_F('t', "track",  &opts->track, "direct|inherit",
>
> ... but these are not. I.e. we'll emit:
>
>     -t, --track[=direct|inherit]
>                           set branch tracking configuration
>
> I.e. implying that the valid uses are --track, --track=direct, and
> --trackinherit.

>
>> +			N_("set up tracking mode (see git-pull(1))"),
>
> Aside: In v5 the other reference was changed from using the
> "git-pull(1)" manpage syntax. See
> https://lore.kernel.org/git/cover.1638859949.git.steadmon@google.com/;
> It looks like this one should be changed too, but was missed.
>
>> +			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>> +			parse_opt_tracking_mode),
>
>
>

> Am 11.01.22 um 02:57 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Mon, Dec 20 2021, Josh Steadmon wrote:
>>
>>> Since we've added an argument to "--track", also add "--track=direct" as
>>> another way to explicitly get the original "--track" behavior ("--track"
>>> without an argument still works as well).
>>> [...]
>>> -'git branch' [--track | --no-track] [-f] <branchname> [<start-point>]
>>> +'git branch' [--track [direct|inherit] | --no-track] [-f] <branchname> [<start-point>]
>>
>> The usage info here is correct...
>
> Actually it isn't, because optional arguments need the equal sign.  I.e.
> this works as expected:
>
>    git branch --track=direct branch
>
> But this here interprets "direct" as a branch name (and branch as a
> start point):
>
>    git branch --track direct branch
>
> The usage string could start with:
>
>   'git branch' [--track | --track=direct | --track=inherit | --no-track]
>
> ... or the less repetitive:
>
>   'git branch' [--track[=(direct|inherit)] | --no-track]
>
> Options with required arguments accept either whitespace or an equal
> sign between option name and arg.  With PARSE_OPT_OPTARG we cannot
> accept whitespace because we cannot decide whether the next thing after
> the option name is an argument or the next parameter.

>>
>>> ---track::
>>> +--track [inherit|direct]::
>>
>> ...as is this...
>
> Same here:
>
>    --track[=(direct|inherit)]
>
>>
>>> -		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 branch tracking configuration"),
>>> +			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>>> +			parse_opt_tracking_mode),
>> ....
>>
>>>  	struct option options[] = {
>>>  		OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")),
>>> -		OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info for new branch"),
>>> -			BRANCH_TRACK_EXPLICIT),
>>> +		OPT_CALLBACK_F('t', "track",  &opts->track, "direct|inherit",
>>
>> ... but these are not. I.e. we'll emit:
>>
>>     -t, --track[=direct|inherit]
>>                           set branch tracking configuration
>>
>> I.e. implying that the valid uses are --track, --track=direct, and
>> --trackinherit.
>
> Well spotted.  It should be specified as "(direct|inherit)" (i.e. with
> parens).

>> It looks like the problem is (ab)use of PARSE_OPT_OPTARG, i.e. it was
>> never meant for an enumeration of possible values, but for something
>> like N_("mode"). It could be made to support that, but it would require
>> some light patching of the releant bits of parse-options.c.
>
> Could you please elaborate that point?  AFAIU PARSE_OPT_OPTARG just
> requires arguments to be attached with equal signs and there is no
> limitation regarding the number of possible argument values.

>> The PARSE_OPT_LITERAL_ARGHELP should also be dropped if it's fixed to
>> use a string like "mode".
>
> That's true.  And it's also enabled automatically if the argument help
> string contains any of the following characters: ()<>[]|.  So basically
> it's never needed explicitly..

> Document that the accepted variants of the --track option are --track,
> --track=direct, and --track=inherit.  The equal sign in the latter two
> cannot be replaced with whitespace; in general optional arguments need
> to be attached firmly to their option.
>
> Put "direct" consistently before "inherit", if only for the reasons
> that the former is the default, explained first in the documentation,
> and comes before the latter alphabetically.
>
> Mention both modes in the short help so that readers don't have to look
> them up in the full documentation.  They are literal strings and thus
> untranslatable.  PARSE_OPT_LITERAL_ARGHELP is inferred due to the pipe
> and parenthesis characters, so we don't have to provide that flag
> explicitly.
>
> Mention that -t has the same effect as --track and --track=direct.
> There is no way to specify inherit mode using the short option, because
> short options generally don't accept optional arguments.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  Documentation/git-branch.txt   | 12 ++++++------
>  Documentation/git-checkout.txt |  2 +-
>  builtin/branch.c               |  2 +-
>  builtin/checkout.c             |  2 +-
>  4 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 2d52ae396b..731e340cbc 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -16,7 +16,7 @@ SYNOPSIS
>  	[--points-at <object>] [--format=<format>]
>  	[(-r | --remotes) | (-a | --all)]
>  	[--list] [<pattern>...]
> -'git branch' [--track [direct|inherit] | --no-track] [-f] <branchname> [<start-point>]
> +'git branch' [--track[=(direct|inherit)] | --no-track] [-f] <branchname> [<start-point>]
>  'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
>  'git branch' --unset-upstream [<branchname>]
>  'git branch' (-m | -M) [<oldbranch>] <newbranch>
> @@ -206,7 +206,7 @@ This option is only applicable in non-verbose mode.
>  	Display the full sha1s in the output listing rather than abbreviating them.
>
>  -t::
> ---track [inherit|direct]::
> +--track[=(direct|inherit)]::
>  	When creating a new branch, set up `branch.<name>.remote` and
>  	`branch.<name>.merge` configuration entries to set "upstream" tracking
>  	configuration for the new branch. This
> @@ -216,11 +216,11 @@ This option is only applicable in non-verbose mode.
>  	upstream when the new branch is checked out.
>  +
>  The exact upstream branch is chosen depending on the optional argument:
> -`--track` or `--track direct` means to use the start-point branch itself as the
> -upstream; `--track inherit` means to copy the upstream configuration of the
> -start-point branch.
> +`-t`, `--track`, or `--track=direct` means to use the start-point branch
> +itself as the upstream; `--track=inherit` means to copy the upstream
> +configuration of the start-point branch.
>  +
> -`--track direct` is the default when the start point is a remote-tracking branch.
> +`--track=direct` is the default when the start point is a remote-tracking branch.
>  Set the branch.autoSetupMerge configuration variable to `false` if you
>  want `git switch`, `git checkout` and `git branch` to always behave as if `--no-track`
>  were given. Set it to `always` if you want this behavior when the
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index 2a90ea6cd0..9f37e22e13 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -156,7 +156,7 @@ of it").
>  	linkgit:git-branch[1] for details.
>
>  -t::
> ---track [direct|inherit]::
> +--track[=(direct|inherit)]::

> Document that the accepted variants of the --track option are --track,
> --track=direct, and --track=inherit.  The equal sign in the latter two
> cannot be replaced with whitespace; in general optional arguments need
> to be attached firmly to their option.
>
> Put "direct" consistently before "inherit", if only for the reasons
> that the former is the default, explained first in the documentation,
> and comes before the latter alphabetically.

> @@ -216,11 +216,11 @@ This option is only applicable in non-verbose mode.
>  	upstream when the new branch is checked out.
>  +
>  The exact upstream branch is chosen depending on the optional argument:
> -`--track` or `--track direct` means to use the start-point branch itself as the
> -upstream; `--track inherit` means to copy the upstream configuration of the
> -start-point branch.
> +`-t`, `--track`, or `--track=direct` means to use the start-point branch
> +itself as the upstream; `--track=inherit` means to copy the upstream
> +configuration of the start-point branch.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 0c8d8a8827..4ce2a24754 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -638,7 +638,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		OPT__VERBOSE(&filter.verbose,
>  			N_("show hash and subject, give twice for upstream branch")),
>  		OPT__QUIET(&quiet, N_("suppress informational messages")),
> -		OPT_CALLBACK_F('t', "track",  &track, N_("mode"),
> +		OPT_CALLBACK_F('t', "track",  &track, "(direct|inherit)",
>  			N_("set branch tracking configuration"),
>  			PARSE_OPT_OPTARG,
>  			parse_opt_tracking_mode),

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 6a5dd2a2a2..0bc2e63510 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1549,7 +1549,7 @@ static struct option *add_common_switch_branch_options(
>  {
>  	struct option options[] = {
>  		OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")),
> -		OPT_CALLBACK_F('t', "track",  &opts->track, N_("mode"),
> +		OPT_CALLBACK_F('t', "track",  &opts->track, "(direct|inherit)",
>  			N_("set branch tracking configuration"),
>  			PARSE_OPT_OPTARG,
>  			parse_opt_tracking_mode),

> Changes since V7:
> * Further simplify verbose output by adding an "<origin>/" prefix for
>   remote-tracking upstream refs.
> * Add a comment explaining the self-tracking check & early exit.
>
> Changes since V6:
> * Strip the refs/heads/ prefix in the verbose output when we have only a
>   single upstream branch.
> * Improve the fatal error message to note that rebasing is only
>   incompatible with multiple upstream refs.
> * Also note that `branch.<name>.remote` should be set in the manual
>   recovery advice.
> * Simplify the logic in setup_tracking() when no tracking sources match.
> * Make the difference in test cases in t2027 more obvious.
>
> Changes since V5:
> * Greatly simplified BRANCH_CONFIG_VERBOSE output to not require nearly
>   so many conditionals.
> * Note that rebasing is not compatible with inheriting multiple upstream
>   branches.
> * Moved the change to case-sensitivity for branch.autosetupmerge to its
>   own commit.
> * Improve advice on failed tracking setup when multiple branches are
>   involved.
> * Make better use of string_list API.
> * Make better use of config API.
> * More straight-forward use of the `struct tracking` API.
> * Numerous style fixes.
>
> 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 (3):
>   branch: accept multiple upstream branches for tracking
>   branch: add flags and config to inherit tracking
>   config: require lowercase for branch.*.autosetupmerge
>
>  Documentation/config/branch.txt |   3 +-
>  Documentation/git-branch.txt    |  24 ++--
>  Documentation/git-checkout.txt  |   2 +-
>  Documentation/git-switch.txt    |   2 +-
>  branch.c                        | 192 ++++++++++++++++++++++++--------
>  branch.h                        |   3 +-
>  builtin/branch.c                |   6 +-
>  builtin/checkout.c              |   6 +-
>  config.c                        |   5 +-
>  parse-options-cb.c              |  16 +++
>  parse-options.h                 |   2 +
>  t/t2017-checkout-orphan.sh      |  11 +-
>  t/t2027-checkout-track.sh       |  23 ++++
>  t/t2060-switch.sh               |  28 +++++
>  t/t3200-branch.sh               |  39 ++++++-
>  t/t7201-co.sh                   |  17 +++
>  16 files changed, 312 insertions(+), 67 deletions(-)
>
> Range-diff against v7:
> 1:  9152367ba9 ! 1:  a5265e1c7f branch: accept multiple upstream branches for tracking
>     @@ branch.c: static int should_setup_rebase(const char *origin)
>      +		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)
>     @@ branch.c: int install_branch_config(int flag, const char *local, const char *ori
>       
>       	if (flag & BRANCH_CONFIG_VERBOSE) {
>      -		if (shortname) {
>     -+		const char *name;
>     -+		struct strbuf ref_string = STRBUF_INIT;
>     -+
>     -+		for_each_string_list_item(item, remotes) {
>     -+			name = item->string;
>     -+			skip_prefix(name, "refs/heads/", &name);
>     -+			strbuf_addf(&ref_string, "  %s\n", name);
>     -+		}
>     -+
>     -+		if (remotes->nr == 1) {
>     -+			struct strbuf refname = STRBUF_INIT;
>     -+
>     - 			if (origin)
>     +-			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'."),
>     @@ branch.c: int install_branch_config(int flag, const char *local, const char *ori
>      -					  _("Branch '%s' set up to track local branch '%s' by rebasing.") :
>      -					  _("Branch '%s' set up to track local branch '%s'."),
>      -					  local, shortname);
>     -+				strbuf_addf(&refname, "%s/", origin);
>     -+			skip_prefix(remotes->items[0].string, "refs/heads/", &name);
>     -+			strbuf_addstr(&refname, name);
>     ++		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.
>     @@ branch.c: int install_branch_config(int flag, const char *local, const char *ori
>      +			printf_ln(rebasing ?
>      +				_("branch '%s' set up to track '%s' by rebasing.") :
>      +				_("branch '%s' set up to track '%s'."),
>     -+				local, refname.buf);
>     -+
>     -+			strbuf_release(&refname);
>     -+		} else if (origin) {
>     -+			printf_ln(_("branch '%s' set up to track from '%s':"),
>     -+				local, origin);
>     -+			printf("%s", ref_string.buf);
>     ++				local, friendly_ref_names.items[0].string);
>       		} else {
>      -			if (origin)
>      -				printf_ln(rebasing ?
>     @@ branch.c: int install_branch_config(int flag, const char *local, const char *ori
>      -					  _("Branch '%s' set up to track local ref '%s'."),
>      -					  local, remote);
>      +			printf_ln(_("branch '%s' set up to track:"), local);
>     -+			printf("%s", ref_string.buf);
>     ++			for_each_string_list_item(item, &friendly_ref_names)
>     ++				printf_ln("  %s", item->string);
>       		}
>      +
>     -+		strbuf_release(&ref_string);
>     ++		string_list_clear(&friendly_ref_names, 0);
>       	}
>       
>       	return 0;
> 2:  afeb84539e = 2:  dcba40e2c4 branch: add flags and config to inherit tracking
> 3:  a818a6561b = 3:  ae7d27b4be config: require lowercase for branch.*.autosetupmerge
>
> base-commit: 6c40894d2466d4e7fddc047a05116aa9d14712ee
> -- 
> 2.34.1.307.g9b7440fafd-goog

> On Wed, Jan 19, 2022 at 2:57 AM René Scharfe <l.s.r@web.de <mailto:l.s.r@web.de>> wrote:
>
>     The help strings for arguments are enclosed in angle brackets
>     automatically.  E.g. if argh is specified as "name", "--option <name>"
>     is printed, to indicate that users need to supply an actual name.  The
>     flag PARSE_OPT_LITERAL_ARGHELP turns this off, so that "--option name"
>     is printed instead, to indicate that the literal string needs to be
>     supplied -- a rare case.
>
>     This flag is enabled automatically if argh contains special characters
>     like brackets.  The developer is supposed to provide any required angle
>     brackets for more complicated cases.  E.g. if argh is "<start>,<end>"
>     then "--option <start>,<end>" is printed.
>
>     Add a comment to mention this PARSE_OPT_LITERAL_ARGHELP behavior.
>
>     Also remove the flag from option definitions for which it's inferred
>     automatically.
>
>     Signed-off-by: René Scharfe <l.s.r@web.de <mailto:l.s.r@web.de>>
>     ---
>     Somehow I feel this is not enough, but I can't pin down what's
>     missing.
>
>      builtin/am.c    | 2 +-
>      builtin/push.c  | 2 +-
>      parse-options.h | 2 ++
>      3 files changed, 4 insertions(+), 2 deletions(-)
>
>     diff --git a/builtin/am.c b/builtin/am.c
>     index b6be1f1cb1..fa8d28794a 100644
>     --- a/builtin/am.c
>     +++ b/builtin/am.c
>     @@ -2402,7 +2402,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
>                     { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
>                       "(diff|raw)",

>                       N_("show the patch being applied"),
>     -                 PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>     +                 PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
>                       parse_opt_show_current_patch, RESUME_SHOW_PATCH },
>                     OPT_CMDMODE(0, "allow-empty", &resume.mode,
>                             N_("record the empty patch as an empty commit"),
>     diff --git a/builtin/push.c b/builtin/push.c
>     index 359db90321..4fa6dfbd09 100644
>     --- a/builtin/push.c
>     +++ b/builtin/push.c
>     @@ -552,7 +552,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>                     OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
>                     OPT_CALLBACK_F(0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),

>                                    N_("require old value of ref to be at this value"),
>     -                              PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, parseopt_push_cas_option),
>     +                              PARSE_OPT_OPTARG, parseopt_push_cas_option),
>                     OPT_BIT(0, TRANS_OPT_FORCE_IF_INCLUDES, &flags,
>                             N_("require remote updates to be integrated locally"),
>                             TRANSPORT_PUSH_FORCE_IF_INCLUDES),
>     diff --git a/parse-options.h b/parse-options.h
>     index e22846d3b7..8d089fb3ae 100644
>     --- a/parse-options.h
>     +++ b/parse-options.h

>     @@ -108,6 +108,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
>       *   PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets
>       *                             (i.e. '<argh>') in the help message.
>       *                             Useful for options with multiple parameters.
>     + *                             Automatically enabled if argh contains any
>     + *                             of the following characters: ()<>[]|
>       *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
>       *                        by git-completion.bash. This option suppresses that.
>       *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to
>     --
>     2.34.1
>