git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Denton Liu <liu.denton@gmail.com>
To: Junio C Hamano <gitster@pobox.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: Fri, 23 Aug 2019 14:15:45 -0400
Message-ID: <20190823181545.GA14048@archbookpro.localdomain> (raw)
In-Reply-To: <xmqq8srms4ak.fsf@gitster-ct.c.googlers.com>

On Wed, Aug 21, 2019 at 12:32:19PM -0700, Junio C Hamano wrote:
> 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.

The reason why I chose to make this an "opt-in" option was because there
currently doesn't exist a standard on how to write branch descriptions
like there does for commit messages (i.e. subject then body, subject
less than x characters). However, against best practices, some
developers like to have really long subjects. As a result, there's no
"real" way of telling whether the first paragraph is a long subject or a
short paragraph.

As a result, we should allow the cover subject to be read from the
branch description only if the developer explicitly chooses this (either
with `--infer-cover-subject` the config option). This way, we won't have
to deal with the ambiguity of deciding whether or not the first
paragraph is truly a subject and stepping on users' toes if we end up
deciding wrong.

Thoughts?

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

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
2019-08-23 18:15       ` Denton Liu [this message]
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=20190823181545.GA14048@archbookpro.localdomain \
    --to=liu.denton@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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)

Archives are clonable:
	git clone --mirror https://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

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/

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