git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / 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>,
	Johannes Sixt <j6t@kdbg.org>,
	Philip Oakley <philipoakley@iee.email>
Subject: Re: [PATCH v4 3/3] format-patch: teach --cover-from-description option
Date: Sat, 12 Oct 2019 11:36:25 +0900
Message-ID: <xmqqd0f2elli.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <9b8c3dcb539054ba483fc34c6ff509e4ca73517c.1570821015.git.liu.denton@gmail.com>

Denton Liu <liu.denton@gmail.com> writes:

> +format.coverFromDescription::
> +	The default mode for format-patch to determine which parts of
> +	the cover letter will be populated using the branch's
> +	description. See the `--cover-from-description` option in
> +	linkgit:git-format-patch[1].
> +
>  format.signature::
>  	The default for format-patch is to output a signature containing
>  	the Git version number. Use this variable to change that default.
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 0ac56f4b70..86114e4c22 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -19,6 +19,7 @@ SYNOPSIS
>  		   [--start-number <n>] [--numbered-files]
>  		   [--in-reply-to=<message id>] [--suffix=.<sfx>]
>  		   [--ignore-if-in-upstream]
> +		   [--cover-from-description=<mode>]
>  		   [--rfc] [--subject-prefix=<subject prefix>]
>  		   [(--reroll-count|-v) <n>]
>  		   [--to=<email>] [--cc=<email>]
> @@ -171,6 +172,26 @@ will want to ensure that threading is disabled for `git send-email`.
>  	patches being generated, and any patch that matches is
>  	ignored.
>  
> +--cover-from-description=<mode>::
> +	Controls which parts of the cover letter will be automatically
> +	populated using the branch's description.
> ++
> +If `<mode>` is `message` or `default`, the cover letter subject will be
> +populated with placeholder text. The body of the cover letter will be
> +populated with the branch's description.

I understand that this is what we do now, so those who want to live
in the past can set the configuration variable to 'message'.

> +If `<mode>` is `subject`, the beginning of the branch description (up to
> +the first blank line) will populate the cover letter subject. The
> +remainder of the description will populate the body of the cover
> +letter.

s/the beginning of .*blank line)/the first paragraph of the branch description/
may be shorter, but the above is OK, too.

When description is prepared appropriately, this mode would fill
both subject and body, which sounds sensible.

> +If `<mode>` is `auto`, if the beginning of the branch description (up to
> +the first line) is greater than 100 characters then the mode will be
> +`message`, otherwise `subject` will be used.

I understand that this is a more clever and safer variant of
'subject'.  Do you want to say 100 characters or 100 bytes?

> +If `<mode>` is `none`, both the cover letter subject and body will be
> +populated with placeholder text.

OK, this is done for completeness?  I wonder who finds it useful to
set it to 'none' *AND* set the branch description.  Not a rhetorical
question that suggests removing this choice, but purely soliciting
opinions from others.

It is unclear (other than the mode word being 'default' for one of
the choices) what the new default mode of operation is after the
patch is applied among the four presented mode.  "This is the
default when no configuration nor command line option specifies the
desired mode" or something may want to be added to one of these
paragraphs.

> @@ -1061,13 +1076,16 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  			      struct commit *origin,
>  			      int nr, struct commit **list,
>  			      const char *branch_name,
> +			      enum cover_from_description cover_from_description_mode,
>  			      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 ***";
>  	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;
> @@ -1095,17 +1113,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);

It may not matter in practice but strictly speaking there is no need
to read the description if we know that the mode is NONE.  Removing
the support for the NONE mode may be an easier fix than adding "&&
mode != NONE" to the if () condition guarding this call---I dunno.

> +	if (cover_from_description_mode != COVER_FROM_NONE && description_sb.len) {
> +		if (cover_from_description_mode == COVER_FROM_SUBJECT ||
> +				cover_from_description_mode == COVER_FROM_AUTO)
> +			body = format_subject(&subject_sb, description_sb.buf, " ");
> +
> +		if (cover_from_description_mode == COVER_FROM_MESSAGE ||
> +				(cover_from_description_mode == COVER_FROM_AUTO &&
> +				 subject_sb.len > COVER_FROM_AUTO_MAX_SUBJECT_LEN))
> +			body = description_sb.buf;
> +		else
> +			subject = subject_sb.buf;
> +	}

I wonder if it make the end result cleaner and easier to follow to
replace all of the above with a single line:

	cover_from_desc(&subject, &body, branch_name, desc_mode);

in this caller, and move the logic (and a handful of strbuf used as
its implementation detail) into the helper function, including the
choice of the default "*** SOMETHING HERE ***", etc., and make the
helper *always* return allocated piece of memory in subject and body
so that this caller can unconditionally free them.

Thanks.

  reply index

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-19 23:52 [PATCH v2 0/4] format-patch: learn --infer-cover-subject option 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
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 [this message]
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

Reply instructions:

You may reply publically 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=xmqqd0f2elli.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=liu.denton@gmail.com \
    --cc=philipoakley@iee.email \
    --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.org/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