git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v5 1/2] branch: accept multiple upstream branches for tracking

From
Josh Steadmon <steadmon@google.com>
Date
Dec 9, 2021, 23:03 UTC
Message-ID
<YbKLL2cQCxXQeQ5J@google.com>
In-Reply-To
<211207.86mtlcpyu4.gmgdl@evledraar.gmail.com>
On 2021.12.07 09:57, Ævar Arnfjörð Bjarmason wrote:
Show 47 quoted lines
> 
> On Mon, Dec 06 2021, Josh Steadmon wrote:
> 
> > Add a new static variant of install_branch_config() that accepts
> > multiple remote branch names for tracking. This will be used in an
> > upcoming commit that enables inheriting the tracking configuration from
> > a parent branch.
> >
> > Currently, all callers of install_branch_config() pass only a single
> > remote. Make install_branch_config() a small wrapper around
> > install_branch_config_multiple_remotes() so that existing callers do not
> > need to be changed.
> >
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> >  branch.c | 120 ++++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 87 insertions(+), 33 deletions(-)
> >
> > diff --git a/branch.c b/branch.c
> > index 7a88a4861e..1aabef4de0 100644
> > --- a/branch.c
> > +++ b/branch.c
> > @@ -55,19 +55,24 @@ N_("\n"
> >  "the remote tracking information by invoking\n"
> >  "\"git branch --set-upstream-to=%s%s%s\".");
> >  
> > -int install_branch_config(int flag, const char *local, const char *origin, const char *remote)
> > +static int install_branch_config_multiple_remotes(int flag, const char *local, const char *origin,
> > +		struct string_list *remotes)
> >  {
> >  	const char *shortname = NULL;
> >  	struct strbuf key = STRBUF_INIT;
> > -	int rebasing = should_setup_rebase(origin);
> > -
> > -	if (skip_prefix(remote, "refs/heads/", &shortname)
> > -	    && !strcmp(local, shortname)
> > -	    && !origin) {
> > -		warning(_("Not setting branch %s as its own upstream."),
> > -			local);
> > -		return 0;
> > -	}
> > +	int i, rebasing = should_setup_rebase(origin);
> > +
> > +	if (remotes->nr < 1)
> > +		BUG("must provide at least one remote for branch config");
> 
> Since it's unsigned IMO this would be clearer: if (!remotes->nr)
Fixed in v6.
Show 13 quoted lines
> > +
> > +	if (!origin)
> > +		for (i = 0; i < remotes->nr; i++)
> > +			if (skip_prefix(remotes->items[i].string, "refs/heads/", &shortname)
> 
> For this and others, since you don't use the [i] for anything except
> getting the current item using for_each_string_list_item() would be
> better.
> 
> Partially a nit, partially that I've got another WIP
> soon-to-be-submitted topic to fix overflow bugs in that API, and not
> having "int i" etc. hardcoded in various places
> helps. I.e. for_each_string_list_item() is future-proof.

Thanks. I somehow missed for_each_string_list_item() when I checked the header file. Fixed in v6.

> > +			    && !strcmp(local, shortname)) {
> > +				warning(_("Not setting branch %s as its own upstream."),
> 
> Better to quote '%s', also s/Not/not/ (lower-case) for all error/warning/die etc.

Done (for now) in v6, but this might become moot as I address Junio's review comments.

Show 66 quoted lines
> > +					local);
> > +				return 0;
> > +			}
> >  
> >  	strbuf_addf(&key, "branch.%s.remote", local);
> >  	if (git_config_set_gently(key.buf, origin ? origin : ".") < 0)
> > @@ -75,8 +80,17 @@ int install_branch_config(int flag, const char *local, const char *origin, const
> >  
> >  	strbuf_reset(&key);
> >  	strbuf_addf(&key, "branch.%s.merge", local);
> > -	if (git_config_set_gently(key.buf, remote) < 0)
> > +	/*
> > +	 * We want to overwrite any existing config with all the branches in
> > +	 * "remotes". Override any existing config with the first branch, but if
> > +	 * more than one is provided, use CONFIG_REGEX_NONE to preserve what
> > +	 * we've written so far.
> > +	 */
> > +	if (git_config_set_gently(key.buf, remotes->items[0].string) < 0)
> >  		goto out_err;
> > +	for (i = 1; i < remotes->nr; i++)
> > +		if (git_config_set_multivar_gently(key.buf, remotes->items[i].string, CONFIG_REGEX_NONE, 0) < 0)
> > +			goto out_err;
> >  
> >  	if (rebasing) {
> >  		strbuf_reset(&key);
> > @@ -87,29 +101,62 @@ int install_branch_config(int flag, const char *local, const char *origin, const
> >  	strbuf_release(&key);
> >  
> >  	if (flag & BRANCH_CONFIG_VERBOSE) {
> > -		if (shortname) {
> > -			if (origin)
> > -				printf_ln(rebasing ?
> > -					  _("Branch '%s' set up to track remote branch '%s' from '%s' by rebasing.") :
> > -					  _("Branch '%s' set up to track remote branch '%s' from '%s'."),
> > -					  local, shortname, origin);
> > -			else
> > -				printf_ln(rebasing ?
> > -					  _("Branch '%s' set up to track local branch '%s' by rebasing.") :
> > -					  _("Branch '%s' set up to track local branch '%s'."),
> > -					  local, shortname);
> > +		int plural = remotes->nr > 1;
> 
> This....
> 
> > +		int all_shortnames = 1;
> > +		const char *msg_fmt;
> > +		struct strbuf ref_string = STRBUF_INIT;
> > +
> > +		for (i = 0; i < remotes->nr; i++)
> > +			if (skip_prefix(remotes->items[i].string, "refs/heads/", &shortname)) {
> > +				strbuf_addf(&ref_string, "'%s', ", shortname);
> > +			} else {
> > +				all_shortnames = 0;
> > +				strbuf_addf(&ref_string, "'%s', ", remotes->items[i].string);
> > +			}
> > +		/* The last two characters are an extraneous ", ", so trim those. */
> > +		strbuf_setlen(&ref_string, ref_string.len - 2);
> 
> languages RTL in around way wrong the be to going is thing of sort This.
> 
> :)
> 
> We deal with this in most other places by just formatting a list of
> branches. E.g. in the ambiguous object output:
> 
>     https://lore.kernel.org/git/patch-v5-4.6-36b6b440c37-20211125T215529Z-avarab@gmail.com/
Done, thanks for the suggestion.
Show 62 quoted lines
> > +
> > +		if (all_shortnames && origin) {
> > +			if (rebasing && plural)
> > +				msg_fmt = "Branch '%s' set up to track remote branches %s from '%s' by rebasing.";
> > +			else if (rebasing && !plural)
> > +				msg_fmt = "Branch '%s' set up to track remote branch %s from '%s' by rebasing.";
> > +			else if (!rebasing && plural)
> > +				msg_fmt = "Branch '%s' set up to track remote branches %s from '%s'.";
> > +			else if (!rebasing && !plural)
> > +				msg_fmt = "Branch '%s' set up to track remote branch %s from '%s'.";
> 
> ...and this is hardcoding plural rules used in English that don't apply
> in a lot of other languages...
> 
> > +
> > +			printf_ln(_(msg_fmt), local, ref_string, origin);
> >  		} else {
> > -			if (origin)
> > -				printf_ln(rebasing ?
> > -					  _("Branch '%s' set up to track remote ref '%s' by rebasing.") :
> > -					  _("Branch '%s' set up to track remote ref '%s'."),
> > -					  local, remote);
> > -			else
> > -				printf_ln(rebasing ?
> > -					  _("Branch '%s' set up to track local ref '%s' by rebasing.") :
> > -					  _("Branch '%s' set up to track local ref '%s'."),
> > -					  local, remote);
> > +			if (all_shortnames && !origin && rebasing && plural)
> > +				msg_fmt = "Branch '%s' set up to track local branches %s by rebasing.";
> > +			if (all_shortnames && !origin && rebasing && !plural)
> > +				msg_fmt = "Branch '%s' set up to track local branch %s by rebasing.";
> > +			if (all_shortnames && !origin && !rebasing && plural)
> > +				msg_fmt = "Branch '%s' set up to track local branches %s.";
> > +			if (all_shortnames && !origin && !rebasing && !plural)
> > +				msg_fmt = "Branch '%s' set up to track local branch %s.";
> > +			if (!all_shortnames && origin && rebasing && plural)
> > +				msg_fmt = "Branch '%s' set up to track remote refs %s by rebasing.";
> > +			if (!all_shortnames && origin && rebasing && !plural)
> > +				msg_fmt = "Branch '%s' set up to track remote ref %s by rebasing.";
> > +			if (!all_shortnames && origin && !rebasing && plural)
> > +				msg_fmt = "Branch '%s' set up to track remote refs %s.";
> > +			if (!all_shortnames && origin && !rebasing && !plural)
> > +				msg_fmt = "Branch '%s' set up to track remote ref %s.";
> > +			if (!all_shortnames && !origin && rebasing && plural)
> > +				msg_fmt = "Branch '%s' set up to track local refs %s by rebasing.";
> > +			if (!all_shortnames && !origin && rebasing && !plural)
> > +				msg_fmt = "Branch '%s' set up to track local ref %s by rebasing.";
> > +			if (!all_shortnames && !origin && !rebasing && plural)
> > +				msg_fmt = "Branch '%s' set up to track local refs %s.";
> > +			if (!all_shortnames && !origin && !rebasing && !plural)
> > +				msg_fmt = "Branch '%s' set up to track local ref %s.";
> 
> ...in English you've got one dog, then dogs, so == 1 and >1, but in
> various other languages it's:
> 
>     git grep Plural-Forms -- po
> 
> Anyway, this is easily solved, and even with less verbosity, see:
> 
>     git grep -E -W '\bQ_\('
> 
> For examples of how to use the magic of libintl to do this for you.

Thank you for the pointer. I looked specifically for dealing with plural forms in our docs, but the referenced "Preparing Strings" gettext docs were not helpful for this. (Although I see now I should have read further in po/README.md to find the relevant advice). I may send a separate change to po/README.md to make it easier to find in the future.

> > +
> > +			printf_ln(_(msg_fmt), local, ref_string);
> 
> ...also s/Branch/branch/ for all of them/.
Done.
Show 19 quoted lines
> >  		}
> > +
> > +		strbuf_release(&ref_string);
> >  	}
> >  
> >  	return 0;
> > @@ -121,11 +168,18 @@ int install_branch_config(int flag, const char *local, const char *origin, const
> >  	advise(_(tracking_advice),
> >  	       origin ? origin : "",
> >  	       origin ? "/" : "",
> > -	       shortname ? shortname : remote);
> > +	       remotes->items[0].string);
> >  
> >  	return -1;
> >  }
> >  
> > +int install_branch_config(int flag, const char *local, const char *origin, const char *remote) {
> 
> nit: overly long line..
Fixed.
> > +	struct string_list remotes = STRING_LIST_INIT_DUP;
> 
> nit: an extra \n after variable decls...
Fixed.
Show 9 quoted lines
> > +	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!
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 26 of 103 in “branch: add "inherit" option for branch.autoSetupMerge”
  1. branch: add "inherit" option for branch.autoSetupMergeJosh Steadmon, Sep 8, 2021
  2. Josh SteadmonSep 8, 2021
  3. branch: add "inherit" option for branch.autoSetupMergeJosh Steadmon, Sep 11, 2021
  4. Junio C HamanoSep 11, 2021
  5. Josh SteadmonOct 17, 2021
  6. Junio C HamanoOct 17, 2021
  7. Josh SteadmonNov 15, 2021
  8. branch: add flags and config to inherit trackingJosh Steadmon, Oct 17, 2021
  9. Ævar Arnfjörð BjarmasonOct 18, 2021
  10. Junio C HamanoOct 18, 2021
  11. Ævar Arnfjörð BjarmasonOct 18, 2021
  12. Josh SteadmonNov 15, 2021
  13. Glen ChooOct 18, 2021
  14. Glen ChooOct 18, 2021
  15. Josh SteadmonNov 15, 2021
  16. branch: add flags and config to inherit trackingJosh Steadmon, Nov 16, 2021
  17. Glen ChooNov 17, 2021
  18. Junio C HamanoNov 18, 2021
  19. Josh SteadmonNov 30, 2021
  20. Ævar Arnfjörð BjarmasonNov 19, 2021
  21. Josh SteadmonNov 30, 2021
  22. Ævar Arnfjörð BjarmasonDec 1, 2021
  23. 0/2 branch: inherit tracking configsJosh Steadmon, Dec 7, 2021
  24. 1/2 branch: accept multiple upstream branches for trackingJosh Steadmon, Dec 7, 2021
  25. Ævar Arnfjörð BjarmasonDec 7, 2021
  26. Josh SteadmonDec 9, 2021
  27. Ævar Arnfjörð BjarmasonDec 10, 2021
  28. Junio C HamanoDec 7, 2021
  29. Josh SteadmonDec 14, 2021
  30. Glen ChooDec 8, 2021
  31. Glen ChooDec 8, 2021
  32. Josh SteadmonDec 9, 2021
  33. Glen ChooDec 9, 2021
  34. Ævar Arnfjörð BjarmasonDec 10, 2021
  35. Glen ChooDec 10, 2021
  36. Ævar Arnfjörð BjarmasonDec 11, 2021
  37. Glen ChooDec 8, 2021
  38. Glen ChooDec 9, 2021
  39. Josh SteadmonDec 9, 2021
  40. Glen ChooDec 9, 2021
  41. 2/2 branch: add flags and config to inherit trackingJosh Steadmon, Dec 7, 2021
  42. Ævar Arnfjörð BjarmasonDec 7, 2021
  43. Glen ChooDec 8, 2021
  44. Josh SteadmonDec 14, 2021
  45. Josh SteadmonDec 14, 2021
  46. Junio C HamanoDec 7, 2021
  47. Josh SteadmonDec 14, 2021
  48. Glen ChooDec 8, 2021
  49. Josh SteadmonDec 14, 2021
  50. Junio C HamanoDec 7, 2021
  51. Glen ChooDec 8, 2021
  52. Johannes SchindelinDec 10, 2021
  53. Josh SteadmonDec 14, 2021
  54. 0/3 branch: inherit tracking configsJosh Steadmon, Dec 14, 2021
  55. 1/3 branch: accept multiple upstream branches for trackingJosh Steadmon, Dec 14, 2021
  56. Junio C HamanoDec 15, 2021
  57. Glen ChooDec 16, 2021
  58. Josh SteadmonDec 17, 2021
  59. Glen ChooDec 20, 2021
  60. Josh SteadmonDec 21, 2021
  61. 2/3 branch: add flags and config to inherit trackingJosh Steadmon, Dec 14, 2021
  62. Glen ChooDec 16, 2021
  63. Josh SteadmonDec 17, 2021
  64. 3/3 config: require lowercase for branch.autosetupmergeJosh Steadmon, Dec 14, 2021
  65. Josh SteadmonDec 15, 2021
  66. Junio C HamanoDec 16, 2021
  67. Glen ChooDec 16, 2021
  68. Junio C HamanoDec 16, 2021
  69. 0/3 branch: inherit tracking configsJosh Steadmon, Dec 17, 2021
  70. 1/3 branch: accept multiple upstream branches for trackingJosh Steadmon, Dec 17, 2021
  71. 2/3 branch: add flags and config to inherit trackingJosh Steadmon, Dec 17, 2021
  72. 3/3 config: require lowercase for branch.*.autosetupmergeJosh Steadmon, Dec 17, 2021
  73. Glen ChooDec 20, 2021
  74. 0/3 branch: inherit tracking configsJosh Steadmon, Dec 21, 2021
  75. 1/3 branch: accept multiple upstream branches for trackingJosh Steadmon, Dec 21, 2021
  76. Junio C HamanoDec 21, 2021
  77. Glen ChooDec 21, 2021
  78. 2/3 branch: add flags and config to inherit trackingJosh Steadmon, Dec 21, 2021
  79. Glen ChooDec 21, 2021
  80. incorrect 'git (checkout|branch) -h' output with new --track modes (was: [PATCH v8 2/3] branch: add flags and config to inherit tracking)Ævar Arnfjörð Bjarmason, Jan 11, 2022
  81. branch,checkout: fix --track usage stringsJosh Steadmon, Jan 18, 2022
  82. Junio C HamanoJan 18, 2022
  83. parse-options: document automatic PARSE_OPT_LITERAL_ARGHELPRené Scharfe, Jan 19, 2022
  84. Ævar Arnfjörð BjarmasonJan 19, 2022
  85. Junio C HamanoJan 19, 2022
  86. René ScharfeJan 20, 2022
  87. Junio C HamanoJan 20, 2022
  88. René ScharfeJan 21, 2022
  89. Junio C HamanoJan 21, 2022
  90. Ævar Arnfjörð BjarmasonJan 20, 2022
  91. Andreas SchwabJan 20, 2022
  92. Ævar Arnfjörð BjarmasonJan 20, 2022
  93. Junio C HamanoJan 20, 2022
  94. Ævar Arnfjörð BjarmasonJan 21, 2022
  95. Junio C HamanoJan 21, 2022
  96. René ScharfeJan 19, 2022
  97. Ævar Arnfjörð BjarmasonJan 20, 2022
  98. branch,checkout: fix --track documentationRené Scharfe, Jan 20, 2022
  99. Ævar Arnfjörð BjarmasonJan 20, 2022
  100. Junio C HamanoJan 20, 2022
  101. 3/3 config: require lowercase for branch.*.autosetupmergeJosh Steadmon, Dec 21, 2021
  102. Glen ChooDec 21, 2021
  103. René ScharfeJan 19, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.