| Message ID | de599f7ca9b5fe7e298bba0bb8c5d05f2f5cf34f.1566285151.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,01/13] t4014: drop unnecessary blank lines from test cases | expand |
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 &&
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 &&
On 23/08/2019 19:15, Denton Liu wrote: >> 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? Perhaps the `--infer-cover-subject` the config option needs to be multi-valued to include: "subject" (always expect short first lines) or "message" (always the long paragraph description, still use ***Subject Here***), with the "true" being used when expecting both as previously described.
Philip Oakley <philipoakley@iee.email> writes: > Perhaps the `--infer-cover-subject` the config option needs to be > multi-valued to include: > "subject" (always expect short first lines) or > "message" (always the long paragraph description, still use > ***Subject Here***), > with the "true" being used when expecting both as previously > described. The idea to have three choices feels that this is getting better, but I notice that the choice is no longer about "subject". I've always felt that the name of this option is way suboptimal. One reason is because the option only says it is about the subject of the cover (letter), and the verb "infer" conveys almost no information---especially it does not say anything about what affects the inference (hint: the branch description value gets used, in a single hardcoded ways right now, but now with the patch we have a choice to control how it gets used).
On Fri, Aug 23, 2019 at 01:18:44PM -0700, Junio C Hamano wrote: > Philip Oakley <philipoakley@iee.email> writes: > > > Perhaps the `--infer-cover-subject` the config option needs to be > > multi-valued to include: > > "subject" (always expect short first lines) or > > "message" (always the long paragraph description, still use > > ***Subject Here***), > > with the "true" being used when expecting both as previously > > described. Good idea, I like this a lot! > > The idea to have three choices feels that this is getting better, > but I notice that the choice is no longer about "subject". > > I've always felt that the name of this option is way suboptimal. > One reason is because the option only says it is about the subject > of the cover (letter), and the verb "infer" conveys almost no > information---especially it does not say anything about what affects > the inference (hint: the branch description value gets used, in a > single hardcoded ways right now, but now with the patch we have a > choice to control how it gets used). Perhaps something like --cover-subject-from-description={true,auto,false}?
On 24/08/2019 09:03, Denton Liu wrote: > On Fri, Aug 23, 2019 at 01:18:44PM -0700, Junio C Hamano wrote: >> Philip Oakley <philipoakley@iee.email> writes: >> >>> Perhaps the `--infer-cover-subject` the config option needs to be >>> multi-valued to include: >>> "subject" (always expect short first lines) or >>> "message" (always the long paragraph description, still use >>> ***Subject Here***), >>> with the "true" being used when expecting both as previously >>> described. > Good idea, I like this a lot! > >> The idea to have three choices feels that this is getting better, >> but I notice that the choice is no longer about "subject". >> >> I've always felt that the name of this option is way suboptimal. >> One reason is because the option only says it is about the subject >> of the cover (letter), and the verb "infer" conveys almost no >> information---especially it does not say anything about what affects >> the inference (hint: the branch description value gets used, in a >> single hardcoded ways right now, but now with the patch we have a >> choice to control how it gets used). > Perhaps something like > --cover-subject-from-description={true,auto,false}? maybe --cover-letter-from-description={true,auto,subject,message,false}? to cover most eventualities (i.e. letter rather than subject). I haven't looked at what happens on Windows (CRLF usage?) for multi-line descriptions. The common assumption is LF in repo, with attributes etc, but the branch description is a bit free format in terms of guidance ;-) Philip
Denton Liu <liu.denton@gmail.com> writes: > Perhaps something like > --cover-subject-from-description={true,auto,false}? Is it still only about "subject"? I thought one of the improved behaviour was to populate the subject header with the title (i.e. the first, often single-line, paragraph) and use the remainder in the body? --use-description-in-cover={both,auto,body} meaning "both subject and body gets filled" (among the three this name is I am least happy with), "automatically decide---if the first paragraph is overly big, it is unwise to use it on the subject", "use it only in body" (implying "use the *** SUBJECT HERE *** placeholder on the subject header), perhaps?
Philip Oakley <philipoakley@iee.email> writes: > I haven't looked at what happens on Windows (CRLF usage?) for > multi-line descriptions. The common assumption is LF in repo, with > attributes etc, but the branch description is a bit free format in > terms of guidance ;-) The same approach taken by the format-patch to use a commit-log message to form the subject and the body should be applicable; as long as the resulting cover letters are made the same way as the normal patch e-mails, it is OK. If both are broken wrt CRLF usage or whatever, as long as they are broken in the same way, we can correct both at the same time ;-)
Junio C Hamano <gitster@pobox.com> writes: > Denton Liu <liu.denton@gmail.com> writes: > >> Perhaps something like >> --cover-subject-from-description={true,auto,false}? > > Is it still only about "subject"? I thought one of the improved > behaviour was to populate the subject header with the title > (i.e. the first, often single-line, paragraph) and use the remainder > in the body? --use-description-in-cover={both,auto,body} meaning > "both subject and body gets filled" (among the three this name is I > am least happy with), "automatically decide---if the first paragraph > is overly big, it is unwise to use it on the subject", "use it only > in body" (implying "use the *** SUBJECT HERE *** placeholder on the > subject header), perhaps? Ah, I see Philip has suggested "--cover-letter-from-description", which also sounds better by not focusing too much on the 'subject', and I have no strong preference between the two.
diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt index cb629fa769..2723566289 100644 --- a/Documentation/config/format.txt +++ b/Documentation/config/format.txt @@ -36,6 +36,12 @@ format.subjectPrefix:: The default for format-patch is to output files with the '[PATCH]' subject prefix. Use this variable to change that prefix. +format.inferCoverSubject:: + A boolean that controls whether or not to take the first line of + the branch description as the subject for the cover letter. See the + `--infer-cover-subject` option in linkgit:git-format-patch[1]. + Default is false. + 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 95bc4d53ca..c5bc0bf5c6 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] + [--[no-]infer-cover-subject] [--rfc] [--subject-prefix=<Subject-Prefix>] [(--reroll-count|-v) <n>] [--to=<email>] [--cc=<email>] @@ -171,6 +172,11 @@ will want to ensure that threading is disabled for `git send-email`. patches being generated, and any patch that matches is ignored. +--[no-]infer-cover-subject:: + Use the beginning of the branch description (up to the first + blank line) as the cover letter subject instead of the default + "*** SUBJECT HERE ***". + --subject-prefix=<Subject-Prefix>:: Instead of the standard '[PATCH]' prefix in the subject line, instead use '[<Subject-Prefix>]'. This @@ -347,6 +353,7 @@ with configuration variables. signOff = true outputDirectory = <directory> coverLetter = auto + inferCoverSubject = true ------------ diff --git a/builtin/log.c b/builtin/log.c index 44b10b3415..a19b746495 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -774,6 +774,7 @@ static const char *signature = git_version_string; static const char *signature_file; static int config_cover_letter; static const char *config_output_directory; +static int infer_cover_subject; enum { COVER_UNSET, @@ -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 &&
Teach format-patch to use the first line of the branch description as the Subject: of the generated cover letter, rather than "*** SUBJECT 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. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Documentation/config/format.txt | 6 ++++ Documentation/git-format-patch.txt | 7 ++++ builtin/log.c | 56 +++++++++++++++++++----------- t/t4014-format-patch.sh | 35 ++++++++++++++++++- 4 files changed, 82 insertions(+), 22 deletions(-)