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

Re: [PATCH v3 13/13] format-patch: learn --infer-cover-subject option

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 21, 2019, 19:32 UTC
Message-ID
<xmqq8srms4ak.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<de599f7ca9b5fe7e298bba0bb8c5d05f2f5cf34f.1566285151.git.liu.denton@gmail.com>
Denton Liu <liu.denton@gmail.com> writes:
> Teach format-patch to use the first line of the branch description as
> the Subject: of the generated cover letter, rather than "*** SUBJECT

I would not say "the first line", as I do not think that is what happens by calling pretty.c::format_subject(). The function is designed to take the first paragraph, and the behaviour is in line with how the subject is formed from the log message in a commit object. I'd say "the first paragraph" instead.

Show 7 quoted lines
> HERE ***" if `--infer-cover-subject` is specified or the corresponding
> `format.inferCoverSubject` option is enabled. This complements the
> existing inclusion of the branch description in the cover letter body.
>
> The reason why this behaviour is not made default is because this change
> is not backwards compatible and may break existing tooling that may rely
> on the default template subject.

I'd suggest writing it more assertively, rather than appearing to be making lame excuses. Perhaps like

	The new behaviour is not made default; doing so would
	surprise existing users, which is not a good idea.

Or just drop the excuse of not changing the default altogether. It is pretty much the standard practice for us to keep the existing behaviour the same and to make the new behaviour opt-in.

Having said that, I suspect that in the longer term, people would want to see this new behaviour with a bit of tweak become the new default.

The "tweak" I suspect is needed is to behave sensibly when "the first line" ends up to be too long a subject. Whether we make this the new default or keep this optional, the issue exists either way.

One way to make it behave sensibly with overly long first paragraph is to fall back to the current behaviour. We can think about the way an ideally "tweaked" version of this patch uses the branch description like this:

 1. Preprocess and prepare the branch description string for use in
    the next step.
    - If there is no branch description, then pretend as if "***
      Subject Here ***" followed by a blank line and "*** Blurb here
      ***" were given as the branch description in the step 2.
    - If the first paragraph of the description is overly long, then
      prepend "*** Subject Here ***" followed by a blank line before
      the branch description, and use that the branch description
      string in the step 2 (this is the "tweak to make it behave
      sensibly" change I suggested above).
    - Otherwise, use the given branch description in the step 2.
      Optionally, when a backward-compatibility knob is in effect,
      always prepend the "Subject Here" paragraph.  That way, step
      2. would end up keeping the traditional behaviour.
 2. Split the first pragraph out of the branch description.  Use it
    as the subject, and use the remainder in the body.

And if we view the behaviour that way, it becomes clear that the "--infer-cover-subject" is a fairly meaningless name for the option. We unconditionally use the branch description to fill in the subject and the body, but the traditional way and the updated one when the first paragraph is overly long use placeholder string for the subject instead. I.e. a better name for the option may be something like --placeholder-subject-in-cover (as opposed to taking the subject in cover from the branch description), and it can be negated i.e. --no-placeholder-subject-in-cover, to force keeping the old behaviour.

And I suspect that the approach would allow the implementation to become simple and straight-forward. The "branch description" needs to be prepared in a few different ways (i.e. if there is no branch.*.description, you'd fill a fixed string; after reading branch.*.description and measuring the first paragraph, you may prepend another fixed string), but after that is done, the actual generation of the cover letter will need NO conditional logic. It just needs to split that into the first paragraph to be used as the subject, and the remainder used in the body.

Hmm?
Show 162 quoted lines
> @@ -887,6 +888,10 @@ static int git_format_config(const char *var, const char *value, void *cb)
>  		}
>  		return 0;
>  	}
> +	if (!strcmp(var, "format.infercoversubject")) {
> +		infer_cover_subject = git_config_bool(var, value);
> +		return 0;
> +	}
>  
>  	return git_log_config(var, value, cb);
>  }
> @@ -993,20 +998,6 @@ static void print_signature(FILE *file)
>  	putc('\n', file);
>  }
>  
> -static void add_branch_description(struct strbuf *buf, const char *branch_name)
> -{
> -	struct strbuf desc = STRBUF_INIT;
> -	if (!branch_name || !*branch_name)
> -		return;
> -	read_branch_desc(&desc, branch_name);
> -	if (desc.len) {
> -		strbuf_addch(buf, '\n');
> -		strbuf_addbuf(buf, &desc);
> -		strbuf_addch(buf, '\n');
> -	}
> -	strbuf_release(&desc);
> -}
> -
>  static char *find_branch_name(struct rev_info *rev)
>  {
>  	int i, positive = -1;
> @@ -1057,13 +1048,17 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  			      struct commit *origin,
>  			      int nr, struct commit **list,
>  			      const char *branch_name,
> +			      int infer_subject,
>  			      int quiet)
>  {
>  	const char *committer;
> -	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
> -	const char *msg;
> +	const char *subject = "*** SUBJECT HERE ***";
> +	const char *body = "*** BLURB HERE ***";
> +	const char *description = NULL;
>  	struct shortlog log;
>  	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf description_sb = STRBUF_INIT;
> +	struct strbuf subject_sb = STRBUF_INIT;
>  	int i;
>  	const char *encoding = "UTF-8";
>  	int need_8bit_cte = 0;
> @@ -1091,17 +1086,34 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	if (!branch_name)
>  		branch_name = find_branch_name(rev);
>  
> -	msg = body;
> +	if (branch_name && *branch_name)
> +		read_branch_desc(&description_sb, branch_name);
> +
> +	if (description_sb.len) {
> +		if (infer_subject) {
> +			description = format_subject(&subject_sb, description_sb.buf, " ");
> +			subject = subject_sb.buf;
> +		} else {
> +			description = description_sb.buf;
> +		}
> +	}
> +
>  	pp.fmt = CMIT_FMT_EMAIL;
>  	pp.date_mode.type = DATE_RFC2822;
>  	pp.rev = rev;
>  	pp.print_email_subject = 1;
>  	pp_user_info(&pp, NULL, &sb, committer, encoding);
> -	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
> -	pp_remainder(&pp, &msg, &sb, 0);
> -	add_branch_description(&sb, branch_name);
> +	pp_title_line(&pp, &subject, &sb, encoding, need_8bit_cte);
> +	pp_remainder(&pp, &body, &sb, 0);
> +	if (description) {
> +		strbuf_addch(&sb, '\n');
> +		strbuf_addstr(&sb, description);
> +		strbuf_addch(&sb, '\n');
> +	}
>  	fprintf(rev->diffopt.file, "%s\n", sb.buf);
>  
> +	strbuf_release(&description_sb);
> +	strbuf_release(&subject_sb);
>  	strbuf_release(&sb);
>  
>  	shortlog_init(&log);
> @@ -1577,6 +1589,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		{ OPTION_CALLBACK, 0, "rfc", &rev, NULL,
>  			    N_("Use [RFC PATCH] instead of [PATCH]"),
>  			    PARSE_OPT_NOARG | PARSE_OPT_NONEG, rfc_callback },
> +		OPT_BOOL(0, "infer-cover-subject", &infer_cover_subject,
> +			    N_("infer a cover letter subject from branch description")),
>  		{ OPTION_CALLBACK, 0, "subject-prefix", &rev, N_("prefix"),
>  			    N_("Use [<prefix>] instead of [PATCH]"),
>  			    PARSE_OPT_NONEG, subject_prefix_callback },
> @@ -1916,7 +1930,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		if (thread)
>  			gen_message_id(&rev, "cover");
>  		make_cover_letter(&rev, use_stdout,
> -				  origin, nr, list, branch_name, quiet);
> +				  origin, nr, list, branch_name, infer_cover_subject, quiet);
>  		print_bases(&bases, rev.diffopt.file);
>  		print_signature(rev.diffopt.file);
>  		total++;
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 7b8c8fe136..94a3191aca 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -822,7 +822,7 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '
>  '
>  
>  test_expect_success 'get git version' '
> -	git_version="$(git --version | sed "s/.* //")"
> +	git_version="$(git --version >version && sed "s/.* //" <version)"
>  '
>  
>  signature() {
> @@ -1516,6 +1516,39 @@ test_expect_success 'format patch ignores color.ui' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'cover letter with config subject' '
> +	test_config branch.rebuild-1.description "config subject
> +
> +body" &&
> +	test_config format.inferCoverSubject true &&
> +	git checkout rebuild-1 &&
> +	git format-patch --stdout --cover-letter master >actual &&
> +	grep "^Subject: \[PATCH 0/2\] config subject$" actual &&
> +	grep "^body" actual
> +'
> +
> +test_expect_success 'cover letter with command-line subject' '
> +	test_config branch.rebuild-1.description "command-line subject
> +
> +body" &&
> +	git checkout rebuild-1 &&
> +	git format-patch --stdout --cover-letter --infer-cover-subject master >actual &&
> +	grep "^Subject: \[PATCH 0/2\] command-line subject$" actual &&
> +	grep "^body" actual
> +'
> +
> +test_expect_success 'cover letter with command-line --no-infer-cover-subject overrides config' '
> +	test_config branch.rebuild-1.description "config subject
> +
> +body" &&
> +	test_config format.inferCoverSubject true &&
> +	git checkout rebuild-1 &&
> +	git format-patch --stdout --cover-letter --no-infer-cover-subject master >actual &&
> +	grep "^Subject: \[PATCH 0/2\] \*\*\* SUBJECT HERE \*\*\*$" actual &&
> +	grep "^config subject" actual &&
> +	grep "^body" actual
> +'
> +
>  test_expect_success 'cover letter using branch description (1)' '
>  	git checkout rebuild-1 &&
>  	test_config branch.rebuild-1.description hello &&
Previous: Denton LiuNext: Denton Liu
Message 29 of 91 in “format-patch: learn --infer-cover-subject option”
  1. 0/4 format-patch: learn --infer-cover-subject optionDenton Liu, Aug 19, 2019
  2. 1/4 t4014: clean up styleDenton Liu, Aug 19, 2019
  3. Eric SunshineAug 20, 2019
  4. 2/4 Doc: add more detail for git-format-patchDenton Liu, Aug 19, 2019
  5. Eric SunshineAug 20, 2019
  6. Denton LiuAug 20, 2019
  7. 3/4 config/format.txt: make clear the default value of format.coverLetterDenton Liu, Aug 19, 2019
  8. Eric SunshineAug 20, 2019
  9. 4/4 format-patch: learn --infer-cover-letter optionDenton Liu, Aug 19, 2019
  10. Eric SunshineAug 20, 2019
  11. 00/13 format-patch: learn --infer-cover-subject option (also t4014 cleanup)Denton Liu, Aug 20, 2019
  12. 01/13 t4014: drop unnecessary blank lines from test casesDenton Liu, Aug 20, 2019
  13. 02/13 t4014: s/expected/expect/Denton Liu, Aug 20, 2019
  14. Eric SunshineAug 20, 2019
  15. 03/13 t4014: move closing sq onto its own lineDenton Liu, Aug 20, 2019
  16. 04/13 t4014: use sq for test case namesDenton Liu, Aug 20, 2019
  17. 05/13 t4014: remove spaces after redirect operatorsDenton Liu, Aug 20, 2019
  18. 06/13 t4014: use indentable here-docsDenton Liu, Aug 20, 2019
  19. 07/13 t4014: drop redirections to /dev/nullDenton Liu, Aug 20, 2019
  20. 08/13 t4014: use test_line_count() where possibleDenton Liu, Aug 20, 2019
  21. 09/13 t4014: remove confusing pipe in check_threading()Denton Liu, Aug 20, 2019
  22. 10/13 t4014: stop losing return codes of git commandsDenton Liu, Aug 20, 2019
  23. Denton LiuAug 20, 2019
  24. Johannes SixtAug 20, 2019
  25. 11/13 Doc: add more detail for git-format-patchDenton Liu, Aug 20, 2019
  26. Junio C HamanoAug 21, 2019
  27. 12/13 config/format.txt: specify default value of format.coverLetterDenton Liu, Aug 20, 2019
  28. 13/13 format-patch: learn --infer-cover-subject optionDenton Liu, Aug 20, 2019
  29. Junio C HamanoAug 21, 2019
  30. Denton LiuAug 23, 2019
  31. Philip OakleyAug 23, 2019
  32. Junio C HamanoAug 23, 2019
  33. Denton LiuAug 24, 2019
  34. Philip OakleyAug 24, 2019
  35. Junio C HamanoAug 26, 2019
  36. Junio C HamanoAug 26, 2019
  37. Junio C HamanoAug 26, 2019
  38. Junio C HamanoAug 22, 2019
  39. Denton LiuAug 23, 2019
  40. Junio C HamanoAug 23, 2019
  41. 00/13 format-patch: clean up tests and documentationDenton Liu, Aug 24, 2019
  42. 01/13 t4014: drop unnecessary blank lines from test casesDenton Liu, Aug 24, 2019
  43. 02/13 t4014: s/expected/expect/Denton Liu, Aug 24, 2019
  44. 03/13 t4014: move closing sq onto its own lineDenton Liu, Aug 24, 2019
  45. 04/13 t4014: use sq for test case namesDenton Liu, Aug 24, 2019
  46. 05/13 t4014: remove spaces after redirect operatorsDenton Liu, Aug 24, 2019
  47. 06/13 t4014: use indentable here-docsDenton Liu, Aug 24, 2019
  48. 07/13 t4014: drop redirections to /dev/nullDenton Liu, Aug 24, 2019
  49. 08/13 t4014: let sed open its own filesDenton Liu, Aug 24, 2019
  50. Eric SunshineAug 26, 2019
  51. 09/13 t4014: use test_line_count() where possibleDenton Liu, Aug 24, 2019
  52. 10/13 t4014: remove confusing pipe in check_threading()Denton Liu, Aug 24, 2019
  53. 11/13 t4014: stop losing return codes of git commandsDenton Liu, Aug 24, 2019
  54. 12/13 Doc: add more detail for git-format-patchDenton Liu, Aug 24, 2019
  55. Junio C HamanoAug 26, 2019
  56. Junio C HamanoAug 26, 2019
  57. 13/13 config/format.txt: specify default value of format.coverLetterDenton Liu, Aug 24, 2019
  58. Denton LiuAug 24, 2019
  59. Junio C HamanoAug 26, 2019
  60. 00/13 format-patch: clean up tests and documentationDenton Liu, Aug 27, 2019
  61. 01/13 t4014: drop unnecessary blank lines from test casesDenton Liu, Aug 27, 2019
  62. 02/13 t4014: s/expected/expect/Denton Liu, Aug 27, 2019
  63. 03/13 t4014: move closing sq onto its own lineDenton Liu, Aug 27, 2019
  64. 04/13 t4014: use sq for test case namesDenton Liu, Aug 27, 2019
  65. 05/13 t4014: remove spaces after redirect operatorsDenton Liu, Aug 27, 2019
  66. 06/13 t4014: use indentable here-docsDenton Liu, Aug 27, 2019
  67. 07/13 t4014: drop redirections to /dev/nullDenton Liu, Aug 27, 2019
  68. 08/13 t4014: let sed open its own filesDenton Liu, Aug 27, 2019
  69. 09/13 t4014: use test_line_count() where possibleDenton Liu, Aug 27, 2019
  70. 10/13 t4014: remove confusing pipe in check_threading()Denton Liu, Aug 27, 2019
  71. 11/13 t4014: stop losing return codes of git commandsDenton Liu, Aug 27, 2019
  72. 12/13 Doc: add more detail for git-format-patchDenton Liu, Aug 27, 2019
  73. 13/13 config/format.txt: specify default value of format.coverLetterDenton Liu, Aug 27, 2019
  74. Denton LiuSep 4, 2019
  75. Junio C HamanoSep 5, 2019
  76. Denton LiuSep 5, 2019
  77. 0/3 format-patch: learn --cover-from-description optionDenton Liu, Oct 11, 2019
  78. 1/3 format-patch: remove erroneous and conditionDenton Liu, Oct 11, 2019
  79. 2/3 format-patch: use enum variablesDenton Liu, Oct 11, 2019
  80. Junio C HamanoOct 12, 2019
  81. 3/3 format-patch: teach --cover-from-description optionDenton Liu, Oct 11, 2019
  82. Junio C HamanoOct 12, 2019
  83. 4/3 fixup! format-patch: teach --cover-from-description optionDenton Liu, Oct 11, 2019
  84. Junio C HamanoOct 12, 2019
  85. 0/3 format-patch: learn --cover-from-description optionDenton Liu, Oct 14, 2019
  86. 1/3 format-patch: change erroneous and conditionDenton Liu, Oct 14, 2019
  87. Junio C HamanoOct 15, 2019
  88. Denton LiuOct 15, 2019
  89. 2/3 format-patch: use enum variablesDenton Liu, Oct 14, 2019
  90. 3/3 format-patch: teach --cover-from-description optionDenton Liu, Oct 14, 2019
  91. Junio C HamanoOct 15, 2019

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.