git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>
Subject: Re: [PATCH v3 13/13] format-patch: learn --infer-cover-subject option
Date: Wed, 21 Aug 2019 12:32:19 -0700
Message-ID: <xmqq8srms4ak.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <de599f7ca9b5fe7e298bba0bb8c5d05f2f5cf34f.1566285151.git.liu.denton@gmail.com> (Denton Liu's message of "Tue, 20 Aug 2019 03:19:16 -0400")

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.

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

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

  reply	other threads:[~2019-08-21 19:32 UTC|newest]

Thread overview: 91+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-19 23:52 [PATCH v2 0/4] " Denton Liu
2019-08-19 23:52 ` [PATCH v2 1/4] t4014: clean up style Denton Liu
2019-08-20  2:41   ` Eric Sunshine
2019-08-19 23:52 ` [PATCH v2 2/4] Doc: add more detail for git-format-patch Denton Liu
2019-08-20  2:44   ` Eric Sunshine
2019-08-20  7:07     ` Denton Liu
2019-08-19 23:52 ` [PATCH v2 3/4] config/format.txt: make clear the default value of format.coverLetter Denton Liu
2019-08-20  2:47   ` Eric Sunshine
2019-08-19 23:52 ` [PATCH v2 4/4] format-patch: learn --infer-cover-letter option Denton Liu
2019-08-20  3:46   ` Eric Sunshine
2019-08-20  7:18 ` [PATCH v3 00/13] format-patch: learn --infer-cover-subject option (also t4014 cleanup) Denton Liu
2019-08-20  7:18   ` [PATCH v3 01/13] t4014: drop unnecessary blank lines from test cases Denton Liu
2019-08-20  7:18   ` [PATCH v3 02/13] t4014: s/expected/expect/ Denton Liu
2019-08-20 21:31     ` Eric Sunshine
2019-08-20  7:18   ` [PATCH v3 03/13] t4014: move closing sq onto its own line Denton Liu
2019-08-20  7:18   ` [PATCH v3 04/13] t4014: use sq for test case names Denton Liu
2019-08-20  7:18   ` [PATCH v3 05/13] t4014: remove spaces after redirect operators Denton Liu
2019-08-20  7:18   ` [PATCH v3 06/13] t4014: use indentable here-docs Denton Liu
2019-08-20  7:19   ` [PATCH v3 07/13] t4014: drop redirections to /dev/null Denton Liu
2019-08-20  7:19   ` [PATCH v3 08/13] t4014: use test_line_count() where possible Denton Liu
2019-08-20  7:19   ` [PATCH v3 09/13] t4014: remove confusing pipe in check_threading() Denton Liu
2019-08-20  7:19   ` [PATCH v3 10/13] t4014: stop losing return codes of git commands Denton Liu
2019-08-20  7:31     ` Denton Liu
2019-08-20 19:04       ` Johannes Sixt
2019-08-20  7:19   ` [PATCH v3 11/13] Doc: add more detail for git-format-patch Denton Liu
2019-08-21 18:26     ` Junio C Hamano
2019-08-20  7:19   ` [PATCH v3 12/13] config/format.txt: specify default value of format.coverLetter Denton Liu
2019-08-20  7:19   ` [PATCH v3 13/13] format-patch: learn --infer-cover-subject option Denton Liu
2019-08-21 19:32     ` Junio C Hamano [this message]
2019-08-23 18:15       ` Denton Liu
2019-08-23 18:46         ` Philip Oakley
2019-08-23 20:18           ` Junio C Hamano
2019-08-24  8:03             ` Denton Liu
2019-08-24 13:59               ` Philip Oakley
2019-08-26 14:30                 ` Junio C Hamano
2019-08-26 14:26               ` Junio C Hamano
2019-08-26 16:05                 ` Junio C Hamano
2019-08-22 20:18   ` [PATCH v3 00/13] format-patch: learn --infer-cover-subject option (also t4014 cleanup) Junio C Hamano
2019-08-23 18:19     ` Denton Liu
2019-08-23 20:25       ` Junio C Hamano
2019-08-24  8:25   ` [PATCH 00/13] format-patch: clean up tests and documentation Denton Liu
2019-08-24  8:26     ` [PATCH 01/13] t4014: drop unnecessary blank lines from test cases Denton Liu
2019-08-24  8:26     ` [PATCH 02/13] t4014: s/expected/expect/ Denton Liu
2019-08-24  8:26     ` [PATCH 03/13] t4014: move closing sq onto its own line Denton Liu
2019-08-24  8:26     ` [PATCH 04/13] t4014: use sq for test case names Denton Liu
2019-08-24  8:26     ` [PATCH 05/13] t4014: remove spaces after redirect operators Denton Liu
2019-08-24  8:27     ` [PATCH 06/13] t4014: use indentable here-docs Denton Liu
2019-08-24  8:27     ` [PATCH 07/13] t4014: drop redirections to /dev/null Denton Liu
2019-08-24  8:27     ` [PATCH 08/13] t4014: let sed open its own files Denton Liu
2019-08-26  0:42       ` Eric Sunshine
2019-08-24  8:27     ` [PATCH 09/13] t4014: use test_line_count() where possible Denton Liu
2019-08-24  8:27     ` [PATCH 10/13] t4014: remove confusing pipe in check_threading() Denton Liu
2019-08-24  8:27     ` [PATCH 11/13] t4014: stop losing return codes of git commands Denton Liu
2019-08-24  8:27     ` [PATCH 12/13] Doc: add more detail for git-format-patch Denton Liu
2019-08-26 15:20       ` Junio C Hamano
2019-08-26 16:07         ` Junio C Hamano
2019-08-24  8:27     ` [PATCH 13/13] config/format.txt: specify default value of format.coverLetter Denton Liu
2019-08-24  8:28     ` [PATCH 00/13] format-patch: clean up tests and documentation Denton Liu
2019-08-26 15:21       ` Junio C Hamano
2019-08-27  4:04     ` [PATCH v2 " Denton Liu
2019-08-27  4:04       ` [PATCH v2 01/13] t4014: drop unnecessary blank lines from test cases Denton Liu
2019-08-27  4:04       ` [PATCH v2 02/13] t4014: s/expected/expect/ Denton Liu
2019-08-27  4:04       ` [PATCH v2 03/13] t4014: move closing sq onto its own line Denton Liu
2019-08-27  4:04       ` [PATCH v2 04/13] t4014: use sq for test case names Denton Liu
2019-08-27  4:05       ` [PATCH v2 05/13] t4014: remove spaces after redirect operators Denton Liu
2019-08-27  4:05       ` [PATCH v2 06/13] t4014: use indentable here-docs Denton Liu
2019-08-27  4:05       ` [PATCH v2 07/13] t4014: drop redirections to /dev/null Denton Liu
2019-08-27  4:05       ` [PATCH v2 08/13] t4014: let sed open its own files Denton Liu
2019-08-27  4:05       ` [PATCH v2 09/13] t4014: use test_line_count() where possible Denton Liu
2019-08-27  4:05       ` [PATCH v2 10/13] t4014: remove confusing pipe in check_threading() Denton Liu
2019-08-27  4:05       ` [PATCH v2 11/13] t4014: stop losing return codes of git commands Denton Liu
2019-08-27  4:05       ` [PATCH v2 12/13] Doc: add more detail for git-format-patch Denton Liu
2019-08-27  4:05       ` [PATCH v2 13/13] config/format.txt: specify default value of format.coverLetter Denton Liu
2019-09-04 11:21       ` [PATCH v2 00/13] format-patch: clean up tests and documentation Denton Liu
2019-09-05 19:56         ` Junio C Hamano
2019-09-05 21:40           ` Denton Liu
2019-10-11 19:12   ` [PATCH v4 0/3] format-patch: learn --cover-from-description option Denton Liu
2019-10-11 19:12     ` [PATCH v4 1/3] format-patch: remove erroneous and condition Denton Liu
2019-10-11 19:12     ` [PATCH v4 2/3] format-patch: use enum variables Denton Liu
2019-10-12  2:16       ` Junio C Hamano
2019-10-11 19:12     ` [PATCH v4 3/3] format-patch: teach --cover-from-description option Denton Liu
2019-10-12  2:36       ` Junio C Hamano
2019-10-11 19:23     ` [PATCH v4 4/3] fixup! " Denton Liu
2019-10-12  4:18     ` [PATCH v4 0/3] format-patch: learn " Junio C Hamano
2019-10-14 20:46     ` [PATCH v5 " Denton Liu
2019-10-14 20:46       ` [PATCH v5 1/3] format-patch: change erroneous and condition Denton Liu
2019-10-15  2:16         ` Junio C Hamano
2019-10-15  3:45           ` Denton Liu
2019-10-14 20:47       ` [PATCH v5 2/3] format-patch: use enum variables Denton Liu
2019-10-14 20:47       ` [PATCH v5 3/3] format-patch: teach --cover-from-description option Denton Liu
2019-10-15  2:25         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq8srms4ak.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ http://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git