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 [thread overview]
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 &&
next prev parent reply other threads:[~2019-08-23 18:15 UTC|newest]
Thread overview: 91+ 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 [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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).