# [PATCH 4/4] Add a --cover-letter option to format-patch 7 messages from 2008-02-17 to 2008-02-18. Participants: Daniel Barkalow, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/12152 ## Daniel Barkalow, 2008-02-17 18:35 Subject: [PATCH 4/4] Add a --cover-letter option to format-patch Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0802171335520.5816%40iabervon.org ``` If --cover-letter is provided, generate a cover letter message before the patches, numbered 0. Original patch thanks to Johannes Schindelin Signed-off-by: Daniel Barkalow --- Documentation/git-format-patch.txt | 6 + builtin-log.c | 220 ++++++++++++++----- t/t4013-diff-various.sh | 1 + ...tch_--stdout_--cover-letter_-n_initial..master^ | 101 +++++++++ 4 files changed, 270 insertions(+), 58 deletions(-) create mode 100644 t/t4013/diff.format-patch_--stdout_--cover-letter_-n_initial..master^ diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 651efe6..53e6d2e 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -17,6 +17,7 @@ SYNOPSIS [--in-reply-to=Message-Id] [--suffix=.] [--ignore-if-in-upstream] [--subject-prefix=Subject-Prefix] + [--cover-letter] [ | ] DESCRIPTION @@ -135,6 +136,11 @@ include::diff-options.txt[] allows for useful naming of a patch series, and can be combined with the --numbered option. +--cover-letter:: + Generate a cover letter template. You still have to fill in + a description, but the shortlog and the diffstat will be + generated for you. + --suffix=.:: Instead of using `.patch` as the suffix for generated filenames, use specified suffix. A common alternative is diff --git a/builtin-log.c b/builtin-log.c index 867cc13..bfefb67 100644 --- a/builtin-log.c +++ b/builtin-log.c @@ -14,6 +14,7 @@ #include "reflog-walk.h" #include "patch-ids.h" #include "refs.h" +#include "run-command.h" static int default_show_root = 1; static const char *fmt_patch_subject_prefix = "PATCH"; @@ -452,74 +453,77 @@ static int git_format_config(const char *var, const char *value) } +static const char *get_oneline_for_filename(struct commit *commit, + int keep_subject) +{ + static char filename[PATH_MAX]; + char *sol; + int len = 0; + + sol = strstr(commit->buffer, "\n\n"); + if (!sol) + filename[0] = '\0'; + else { + int j, space = 0; + + sol += 2; + /* strip [PATCH] or [PATCH blabla] */ + if (!keep_subject && !prefixcmp(sol, "[PATCH")) { + char *eos = strchr(sol + 6, ']'); + if (eos) { + while (isspace(*eos)) + eos++; + sol = eos; + } + } + + for (j = 0; len < sizeof(filename) && + sol[j] && sol[j] != '\n'; j++) { + if (istitlechar(sol[j])) { + if (space) { + filename[len++] = '-'; + space = 0; + } + filename[len++] = sol[j]; + if (sol[j] == '.') + while (sol[j + 1] == '.') + j++; + } else + space = 1; + } + while (filename[len - 1] == '.' + || filename[len - 1] == '-') + len--; + filename[len] = '\0'; + } + return filename; +} + static FILE *realstdout = NULL; static const char *output_directory = NULL; -static int reopen_stdout(struct commit *commit, int nr, int keep_subject, - int numbered_files) +static int reopen_stdout(const char *oneline, int nr, int total) { char filename[PATH_MAX]; - char *sol; int len = 0; int suffix_len = strlen(fmt_patch_suffix) + 1; if (output_directory) { - if (strlen(output_directory) >= + len = snprintf(filename, sizeof(filename), "%s", + output_directory); + if (len >= sizeof(filename) - FORMAT_PATCH_NAME_MAX - suffix_len) return error("name of output directory is too long"); - strlcpy(filename, output_directory, sizeof(filename) - suffix_len); - len = strlen(filename); if (filename[len - 1] != '/') filename[len++] = '/'; } - if (numbered_files) { - sprintf(filename + len, "%d", nr); - len = strlen(filename); - - } else { - sprintf(filename + len, "%04d", nr); - len = strlen(filename); - - sol = strstr(commit->buffer, "\n\n"); - if (sol) { - int j, space = 1; - - sol += 2; - /* strip [PATCH] or [PATCH blabla] */ - if (!keep_subject && !prefixcmp(sol, "[PATCH")) { - char *eos = strchr(sol + 6, ']'); - if (eos) { - while (isspace(*eos)) - eos++; - sol = eos; - } - } - - for (j = 0; - j < FORMAT_PATCH_NAME_MAX - suffix_len - 5 && - len < sizeof(filename) - suffix_len && - sol[j] && sol[j] != '\n'; - j++) { - if (istitlechar(sol[j])) { - if (space) { - filename[len++] = '-'; - space = 0; - } - filename[len++] = sol[j]; - if (sol[j] == '.') - while (sol[j + 1] == '.') - j++; - } else - space = 1; - } - while (filename[len - 1] == '.' - || filename[len - 1] == '-') - len--; - filename[len] = 0; - } - if (len + suffix_len >= sizeof(filename)) - return error("Patch pathname too long"); + if (!oneline) + len += sprintf(filename + len, "%d", nr); + else { + len += sprintf(filename + len, "%04d-", nr); + len += snprintf(filename + len, sizeof(filename) - len - 1 + - suffix_len, "%s", oneline); strcpy(filename + len, fmt_patch_suffix); } @@ -590,6 +594,74 @@ static void gen_message_id(struct rev_info *info, char *base) info->message_id = strbuf_detach(&buf, NULL); } +static void make_cover_letter(struct rev_info *rev, + int use_stdout, int numbered, int numbered_files, + struct commit *origin, struct commit *head) +{ + const char *committer; + const char *origin_sha1, *head_sha1; + const char *argv[7]; + const char *subject_start = NULL; + const char *body = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n"; + const char *msg; + const char *extra_headers = rev->extra_headers; + struct strbuf sb; + const char *encoding = "utf-8"; + + if (rev->commit_format != CMIT_FMT_EMAIL) + die("Cover letter needs email format"); + + if (!use_stdout && reopen_stdout(numbered_files ? + NULL : "cover-letter", 0, rev->total)) + return; + + origin_sha1 = sha1_to_hex(origin ? origin->object.sha1 : null_sha1); + head_sha1 = sha1_to_hex(head->object.sha1); + + write_email_headers(rev, head_sha1, &subject_start, &extra_headers); + + committer = git_committer_info(0); + + msg = body; + strbuf_init(&sb, 0); + add_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822, + encoding); + pp_title_line(CMIT_FMT_EMAIL, &msg, &sb, subject_start, extra_headers, + encoding, 0); + pp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0); + printf("%s\n", sb.buf); + + strbuf_release(&sb); + + /* + * We can only do diffstat with a unique reference point, and + * log is a bit tricky, so just skip it. + */ + if (!origin) + return; + + argv[0] = "shortlog"; + argv[1] = head_sha1; + argv[2] = "--not"; + argv[3] = origin_sha1; + argv[4] = NULL; + fflush(stdout); + run_command_v_opt(argv, RUN_GIT_CMD); + + argv[0] = "diff"; + argv[1] = "--stat"; + argv[2] = "--summary"; + argv[3] = head_sha1; + argv[4] = "--not"; + argv[5] = origin_sha1; + argv[6] = NULL; + fflush(stdout); + run_command_v_opt(argv, RUN_GIT_CMD); + + fflush(stdout); + printf("\n"); +} + static const char *clean_message_id(const char *msg_id) { char ch; @@ -625,6 +697,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) int subject_prefix = 0; int ignore_if_in_upstream = 0; int thread = 0; + int cover_letter = 0; + struct commit *origin = NULL, *head = NULL; const char *in_reply_to = NULL; struct patch_ids ids; char *add_signoff = NULL; @@ -724,6 +798,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) rev.subject_prefix = argv[i] + 17; } else if (!prefixcmp(argv[i], "--suffix=")) fmt_patch_suffix = argv[i] + 9; + else if (!strcmp(argv[i], "--cover-letter")) + cover_letter = 1; else argv[j++] = argv[i]; } @@ -775,6 +851,25 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) * get_revision() to do the usual traversal. */ } + if (cover_letter) { + /* remember the range */ + int negative_count = 0; + int i; + for (i = 0; i < rev.pending.nr; i++) { + struct object *o = rev.pending.objects[i].item; + if (o->flags & UNINTERESTING) { + origin = (struct commit *)o; + negative_count++; + } else + head = (struct commit *)o; + } + /* Multiple origins don't work for diffstat. */ + if (negative_count > 1) + origin = NULL; + /* We can't generate a cover letter without any patches */ + if (!head) + return 0; + } if (ignore_if_in_upstream) get_patch_ids(&rev, &ids, prefix); @@ -801,9 +896,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) numbered = 1; if (numbered) rev.total = total + start_number - 1; - rev.add_signoff = add_signoff; if (in_reply_to) rev.ref_message_id = clean_message_id(in_reply_to); + if (cover_letter) { + if (thread) { + gen_message_id(&rev, "cover"); + } + make_cover_letter(&rev, use_stdout, numbered, numbered_files, + origin, head); + total++; + start_number--; + } + rev.add_signoff = add_signoff; while (0 <= --nr) { int shown; commit = list[nr]; @@ -818,10 +922,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) } gen_message_id(&rev, sha1_to_hex(commit->object.sha1)); } - if (!use_stdout) - if (reopen_stdout(commit, rev.nr, keep_subject, - numbered_files)) - die("Failed to create output files"); + if (!use_stdout && reopen_stdout(numbered_files ? NULL : + get_oneline_for_filename(commit, keep_subject), + rev.nr, rev.total)) + die("Failed to create output files"); shown = log_tree_commit(&rev, commit); free(commit->buffer); commit->buffer = NULL; diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 9eec754..6b4d1c5 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -245,6 +245,7 @@ format-patch --inline --stdout initial..master format-patch --inline --stdout --subject-prefix=TESTCASE initial..master config format.subjectprefix DIFFERENT_PREFIX format-patch --inline --stdout initial..master^^ +format-patch --stdout --cover-letter -n initial..master^ diff --abbrev initial..side diff -r initial..side diff --git a/t/t4013/diff.format-patch_--stdout_--cover-letter_-n_initial..master^ b/t/t4013/diff.format-patch_--stdout_--cover-letter_-n_initial..master^ new file mode 100644 index 0000000..311e207 --- /dev/null +++ b/t/t4013/diff.format-patch_--stdout_--cover-letter_-n_initial..master^ @@ -0,0 +1,101 @@ +$ git format-patch --stdout --cover-letter -n initial..master^ +From 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 Mon Sep 17 00:00:00 2001 +From: C O Mitter +Date: Mon, 26 Jun 2006 00:05:00 +0000 +Subject: [DIFFERENT_PREFIX 0/2] *** SUBJECT HERE *** + + +*** BLURB HERE *** + +A U Thor (2): + Second + Third + + dir/sub | 4 ++++ + file0 | 3 +++ + file1 | 3 +++ + file2 | 3 --- + 4 files changed, 10 insertions(+), 3 deletions(-) + create mode 100644 file1 + delete mode 100644 file2 + +From 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44 Mon Sep 17 00:00:00 2001 +From: A U Thor +Date: Mon, 26 Jun 2006 00:01:00 +0000 +Subject: [DIFFERENT_PREFIX 1/2] Second + +This is the second commit. +--- + dir/sub | 2 ++ + file0 | 3 +++ + file2 | 3 --- + 3 files changed, 5 insertions(+), 3 deletions(-) + delete mode 100644 file2 + +diff --git a/dir/sub b/dir/sub +index 35d242b..8422d40 100644 +--- a/dir/sub ++++ b/dir/sub +@@ -1,2 +1,4 @@ + A + B ++C ++D +diff --git a/file0 b/file0 +index 01e79c3..b414108 100644 +--- a/file0 ++++ b/file0 +@@ -1,3 +1,6 @@ + 1 + 2 + 3 ++4 ++5 ++6 +diff --git a/file2 b/file2 +deleted file mode 100644 +index 01e79c3..0000000 +--- a/file2 ++++ /dev/null +@@ -1,3 +0,0 @@ +-1 +-2 +-3 +-- +g-i-t--v-e-r-s-i-o-n + + +From 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 Mon Sep 17 00:00:00 2001 +From: A U Thor +Date: Mon, 26 Jun 2006 00:02:00 +0000 +Subject: [DIFFERENT_PREFIX 2/2] Third + +--- + dir/sub | 2 ++ + file1 | 3 +++ + 2 files changed, 5 insertions(+), 0 deletions(-) + create mode 100644 file1 + +diff --git a/dir/sub b/dir/sub +index 8422d40..cead32e 100644 +--- a/dir/sub ++++ b/dir/sub +@@ -2,3 +2,5 @@ A + B + C + D ++E ++F +diff --git a/file1 b/file1 +new file mode 100644 +index 0000000..b1e6722 +--- /dev/null ++++ b/file1 +@@ -0,0 +1,3 @@ ++A ++B ++C +-- +g-i-t--v-e-r-s-i-o-n + +$ -- 1.5.4.1.1350.g2b9ee ``` ## Junio C Hamano, 2008-02-18 03:45 Subject: Re: [PATCH 4/4] Add a --cover-letter option to format-patch Message-ID: <7v4pc7udux.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4pc7udux.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Daniel Barkalow writes: > + argv[0] = "shortlog"; > + argv[1] = head_sha1; > + argv[2] = "--not"; > + argv[3] = origin_sha1; > + argv[4] = NULL; > + fflush(stdout); > + run_command_v_opt(argv, RUN_GIT_CMD); Please make it a habit to always append "--" disambiguator for such command lines generated internally. In this case you are not usinging short and common name like "master" or "HEAD" but 40-letter-long hex string, so it is much less likely to get hit by the "ambiguous rev vs file" rule, but even then it is still possible that a work tree happens to have a file with the same name. > @@ -0,0 +1,101 @@ > +$ git format-patch --stdout --cover-letter -n initial..master^ > +From 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 Mon Sep 17 00:00:00 2001 > +From: C O Mitter > +Date: Mon, 26 Jun 2006 00:05:00 +0000 > +Subject: [DIFFERENT_PREFIX 0/2] *** SUBJECT HERE *** > + > + > +*** BLURB HERE *** > + > +A U Thor (2): > + Second > + Third > + This probably falls within the "personal taste" category, but I'd rather not to see two extra blank lines between Subject: and BLURB HERE. ``` ## Johannes Schindelin, 2008-02-18 13:19 Subject: Re: [PATCH 4/4] Add a --cover-letter option to format-patch Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802181245270.30505%40racer.site In-Reply-To: ``` Hi, On Sun, 17 Feb 2008, Daniel Barkalow wrote: > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > index 651efe6..53e6d2e 100644 > --- a/Documentation/git-format-patch.txt > +++ b/Documentation/git-format-patch.txt > @@ -17,6 +17,7 @@ SYNOPSIS > [--in-reply-to=Message-Id] [--suffix=.] > [--ignore-if-in-upstream] > [--subject-prefix=Subject-Prefix] > + [--cover-letter] > [ | ] Since you are already there, and it is really a small change, why not fix the whitespace? > diff --git a/builtin-log.c b/builtin-log.c > index 867cc13..bfefb67 100644 > --- a/builtin-log.c > +++ b/builtin-log.c > @@ -14,6 +14,7 @@ > #include "reflog-walk.h" > #include "patch-ids.h" > #include "refs.h" > +#include "run-command.h" > > static int default_show_root = 1; > static const char *fmt_patch_subject_prefix = "PATCH"; > @@ -452,74 +453,77 @@ static int git_format_config(const char *var, const char *value) > } > > > +static const char *get_oneline_for_filename(struct commit *commit, > + int keep_subject) > +{ > + static char filename[PATH_MAX]; > + char *sol; > + int len = 0; > + > + sol = strstr(commit->buffer, "\n\n"); > + if (!sol) > + filename[0] = '\0'; > + else { > + int j, space = 0; > + > + sol += 2; > + /* strip [PATCH] or [PATCH blabla] */ > + if (!keep_subject && !prefixcmp(sol, "[PATCH")) { > + char *eos = strchr(sol + 6, ']'); > + if (eos) { > + while (isspace(*eos)) > + eos++; > + sol = eos; > + } > + } > + > + for (j = 0; len < sizeof(filename) && > + sol[j] && sol[j] != '\n'; j++) { > + if (istitlechar(sol[j])) { > + if (space) { > + filename[len++] = '-'; > + space = 0; > + } > + filename[len++] = sol[j]; > + if (sol[j] == '.') > + while (sol[j + 1] == '.') > + j++; > + } else > + space = 1; > + } > + while (filename[len - 1] == '.' > + || filename[len - 1] == '-') > + len--; > + filename[len] = '\0'; > + } > + return filename; > +} Where has the FORMAT_PATCH_NAME_MAX handling gone? See c06d2daa(Limit filename for format-patch)... BTW it is not only because format-patch would fail. The maximum length was chosen such that it does not clutter the window when ls'ing the files. > @@ -590,6 +594,74 @@ static void gen_message_id(struct rev_info *info, char *base) > info->message_id = strbuf_detach(&buf, NULL); > } > > +static void make_cover_letter(struct rev_info *rev, > + int use_stdout, int numbered, int numbered_files, > + struct commit *origin, struct commit *head) > +{ > + const char *committer; > + const char *origin_sha1, *head_sha1; > + const char *argv[7]; > + const char *subject_start = NULL; > + const char *body = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n"; > + const char *msg; > + const char *extra_headers = rev->extra_headers; > + struct strbuf sb; > + const char *encoding = "utf-8"; > + > + if (rev->commit_format != CMIT_FMT_EMAIL) > + die("Cover letter needs email format"); > + > + if (!use_stdout && reopen_stdout(numbered_files ? > + NULL : "cover-letter", 0, rev->total)) > + return; > + > + origin_sha1 = sha1_to_hex(origin ? origin->object.sha1 : null_sha1); > + head_sha1 = sha1_to_hex(head->object.sha1); > + > + write_email_headers(rev, head_sha1, &subject_start, &extra_headers); > + > + committer = git_committer_info(0); > + > + msg = body; > + strbuf_init(&sb, 0); > + add_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822, > + encoding); > + pp_title_line(CMIT_FMT_EMAIL, &msg, &sb, subject_start, extra_headers, > + encoding, 0); > + pp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0); > + printf("%s\n", sb.buf); > + > + strbuf_release(&sb); > + > + /* > + * We can only do diffstat with a unique reference point, and > + * log is a bit tricky, so just skip it. > + */ > + if (!origin) > + return; > + > + argv[0] = "shortlog"; > + argv[1] = head_sha1; > + argv[2] = "--not"; > + argv[3] = origin_sha1; Should this not output a complete shortlog, if the user said git format-patch --root --cover-letter HEAD Hmm? > + argv[4] = NULL; > + fflush(stdout); > + run_command_v_opt(argv, RUN_GIT_CMD); > + > + argv[0] = "diff"; > + argv[1] = "--stat"; > + argv[2] = "--summary"; > + argv[3] = head_sha1; > + argv[4] = "--not"; > + argv[5] = origin_sha1; Likewise. > + argv[6] = NULL; > + fflush(stdout); > + run_command_v_opt(argv, RUN_GIT_CMD); > + > + fflush(stdout); > + printf("\n"); > +} > + > static const char *clean_message_id(const char *msg_id) > { > char ch; > @@ -775,6 +851,25 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > * get_revision() to do the usual traversal. > */ > } > + if (cover_letter) { > + /* remember the range */ > + int negative_count = 0; > + int i; > + for (i = 0; i < rev.pending.nr; i++) { > + struct object *o = rev.pending.objects[i].item; > + if (o->flags & UNINTERESTING) { > + origin = (struct commit *)o; > + negative_count++; > + } else > + head = (struct commit *)o; > + } > + /* Multiple origins don't work for diffstat. */ > + if (negative_count > 1) > + origin = NULL; Oh, and here it gets interesting. If you say git format-patch --cover-letter ^commit1 ^commit2 HEAD then origin will be set to NULL, and in the diffstat, instead of showing _no_ diffstat, you show the same as if no negative ref was given (which _should_ show a diff against the empty tree, but shows nothing currently). So I suggest changing "origin" to a commit_list, and have cover-letter _not_ output a diffstat if more than one commits were given. Or as many diffstats. Whatever. > + /* We can't generate a cover letter without any patches */ > + if (!head) > + return 0; Should this not return error("You want a cover letter for what," "exactly?"); Hmm? > @@ -801,9 +896,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > numbered = 1; > if (numbered) > rev.total = total + start_number - 1; > - rev.add_signoff = add_signoff; > if (in_reply_to) > rev.ref_message_id = clean_message_id(in_reply_to); > + if (cover_letter) { > + if (thread) { > + gen_message_id(&rev, "cover"); > + } These curly brackets are sure distracting. > + make_cover_letter(&rev, use_stdout, numbered, numbered_files, > + origin, head); > + total++; > + start_number--; > + } > + rev.add_signoff = add_signoff; Why not sign off the cover letter, too? BTW I had to chew over the thread handling quite a few minutes... maybe you want to add a test for --cover-letter --thread, just to make sure it does not get broken (and of course, to prove that it works with your patches). Thanks, Dscho ``` ## Daniel Barkalow, 2008-02-18 17:15 Subject: Re: [PATCH 4/4] Add a --cover-letter option to format-patch Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0802181203080.5816%40iabervon.org In-Reply-To: <7v4pc7udux.fsf@gitster.siamese.dyndns.org> ``` On Sun, 17 Feb 2008, Junio C Hamano wrote: > Daniel Barkalow writes: > > > + argv[0] = "shortlog"; > > + argv[1] = head_sha1; > > + argv[2] = "--not"; > > + argv[3] = origin_sha1; > > + argv[4] = NULL; > > + fflush(stdout); > > + run_command_v_opt(argv, RUN_GIT_CMD); > > Please make it a habit to always append "--" disambiguator for > such command lines generated internally. In this case you are > not usinging short and common name like "master" or "HEAD" but > 40-letter-long hex string, so it is much less likely to get hit > by the "ambiguous rev vs file" rule, but even then it is still > possible that a work tree happens to have a file with the same > name. Sure. > > @@ -0,0 +1,101 @@ > > +$ git format-patch --stdout --cover-letter -n initial..master^ > > +From 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 Mon Sep 17 00:00:00 2001 > > +From: C O Mitter > > +Date: Mon, 26 Jun 2006 00:05:00 +0000 > > +Subject: [DIFFERENT_PREFIX 0/2] *** SUBJECT HERE *** > > + > > + > > +*** BLURB HERE *** > > + > > +A U Thor (2): > > + Second > > + Third > > + > > This probably falls within the "personal taste" category, but > I'd rather not to see two extra blank lines between Subject: and > BLURB HERE. The file is ugly with the double blank line, but I like it for what I think is the normal case, where you import the series into your mail reader, edit the cover letter there (where it appears as a message with some space devoted to the blurb by virtue of starting with now one blank line), and then send. In that context, I think it's nicer. -Daniel *This .sig left intentionally blank* ``` ## Daniel Barkalow, 2008-02-18 18:54 Subject: Re: [PATCH 4/4] Add a --cover-letter option to format-patch Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0802181254060.5816%40iabervon.org In-Reply-To: ``` On Mon, 18 Feb 2008, Johannes Schindelin wrote: > Hi, > > On Sun, 17 Feb 2008, Daniel Barkalow wrote: > > > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > > index 651efe6..53e6d2e 100644 > > --- a/Documentation/git-format-patch.txt > > +++ b/Documentation/git-format-patch.txt > > @@ -17,6 +17,7 @@ SYNOPSIS > > [--in-reply-to=Message-Id] [--suffix=.] > > [--ignore-if-in-upstream] > > [--subject-prefix=Subject-Prefix] > > + [--cover-letter] > > [ | ] > > Since you are already there, and it is really a small change, why not fix > the whitespace? What *is* the correct whitespace for alignment in documentation? > > diff --git a/builtin-log.c b/builtin-log.c > > index 867cc13..bfefb67 100644 > > --- a/builtin-log.c > > +++ b/builtin-log.c > > @@ -14,6 +14,7 @@ > > #include "reflog-walk.h" > > #include "patch-ids.h" > > #include "refs.h" > > +#include "run-command.h" > > > > static int default_show_root = 1; > > static const char *fmt_patch_subject_prefix = "PATCH"; > > @@ -452,74 +453,77 @@ static int git_format_config(const char *var, const char *value) > > } > > > > > > +static const char *get_oneline_for_filename(struct commit *commit, > > + int keep_subject) > > +{ > > + static char filename[PATH_MAX]; > > + char *sol; > > + int len = 0; > > + > > + sol = strstr(commit->buffer, "\n\n"); > > + if (!sol) > > + filename[0] = '\0'; > > + else { > > + int j, space = 0; > > + > > + sol += 2; > > + /* strip [PATCH] or [PATCH blabla] */ > > + if (!keep_subject && !prefixcmp(sol, "[PATCH")) { > > + char *eos = strchr(sol + 6, ']'); > > + if (eos) { > > + while (isspace(*eos)) > > + eos++; > > + sol = eos; > > + } > > + } > > + > > + for (j = 0; len < sizeof(filename) && > > + sol[j] && sol[j] != '\n'; j++) { > > + if (istitlechar(sol[j])) { > > + if (space) { > > + filename[len++] = '-'; > > + space = 0; > > + } > > + filename[len++] = sol[j]; > > + if (sol[j] == '.') > > + while (sol[j + 1] == '.') > > + j++; > > + } else > > + space = 1; > > + } > > + while (filename[len - 1] == '.' > > + || filename[len - 1] == '-') > > + len--; > > + filename[len] = '\0'; > > + } > > + return filename; > > +} > > Where has the FORMAT_PATCH_NAME_MAX handling gone? See c06d2daa(Limit > filename for format-patch)... Wow, beats me. I'd have thought I'd have gotten some conflicts while rebasing. Anyway, fixed, and I'll add a test for this case. > > @@ -590,6 +594,74 @@ static void gen_message_id(struct rev_info *info, char *base) > > info->message_id = strbuf_detach(&buf, NULL); > > } > > > > +static void make_cover_letter(struct rev_info *rev, > > + int use_stdout, int numbered, int numbered_files, > > + struct commit *origin, struct commit *head) > > +{ > > + const char *committer; > > + const char *origin_sha1, *head_sha1; > > + const char *argv[7]; > > + const char *subject_start = NULL; > > + const char *body = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n"; > > + const char *msg; > > + const char *extra_headers = rev->extra_headers; > > + struct strbuf sb; > > + const char *encoding = "utf-8"; > > + > > + if (rev->commit_format != CMIT_FMT_EMAIL) > > + die("Cover letter needs email format"); > > + > > + if (!use_stdout && reopen_stdout(numbered_files ? > > + NULL : "cover-letter", 0, rev->total)) > > + return; > > + > > + origin_sha1 = sha1_to_hex(origin ? origin->object.sha1 : null_sha1); > > + head_sha1 = sha1_to_hex(head->object.sha1); > > + > > + write_email_headers(rev, head_sha1, &subject_start, &extra_headers); > > + > > + committer = git_committer_info(0); > > + > > + msg = body; > > + strbuf_init(&sb, 0); > > + add_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822, > > + encoding); > > + pp_title_line(CMIT_FMT_EMAIL, &msg, &sb, subject_start, extra_headers, > > + encoding, 0); > > + pp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0); > > + printf("%s\n", sb.buf); > > + > > + strbuf_release(&sb); > > + > > + /* > > + * We can only do diffstat with a unique reference point, and > > + * log is a bit tricky, so just skip it. > > + */ > > + if (!origin) > > + return; > > + > > + argv[0] = "shortlog"; > > + argv[1] = head_sha1; > > + argv[2] = "--not"; > > + argv[3] = origin_sha1; > > Should this not output a complete shortlog, if the user said > > git format-patch --root --cover-letter HEAD > > Hmm? It should, but that's in the "tricky" category. For future work, I want to make it possible to have the log mechanism able to output the same batch of commits twice with different settings, so that the cover letter's short log will be an exact match for the contents of the series no matter what, but that's a lot more work and not needed for the common case, which I'd like to get in first. > > + argv[4] = NULL; > > + fflush(stdout); > > + run_command_v_opt(argv, RUN_GIT_CMD); > > + > > + argv[0] = "diff"; > > + argv[1] = "--stat"; > > + argv[2] = "--summary"; > > + argv[3] = head_sha1; > > + argv[4] = "--not"; > > + argv[5] = origin_sha1; > > Likewise. Here, we really want to be able to ask the log-output stuff if we have exactly one non-included parent of included commits, which is what we should diff against. In any case, again depends on being able to ask for the right info from the library in order to do more than just the easy case without reimplementing a lot or having a mess of special cases. > > + argv[6] = NULL; > > + fflush(stdout); > > + run_command_v_opt(argv, RUN_GIT_CMD); > > + > > + fflush(stdout); > > + printf("\n"); > > +} > > + > > static const char *clean_message_id(const char *msg_id) > > { > > char ch; > > @@ -775,6 +851,25 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > > * get_revision() to do the usual traversal. > > */ > > } > > + if (cover_letter) { > > + /* remember the range */ > > + int negative_count = 0; > > + int i; > > + for (i = 0; i < rev.pending.nr; i++) { > > + struct object *o = rev.pending.objects[i].item; > > + if (o->flags & UNINTERESTING) { > > + origin = (struct commit *)o; > > + negative_count++; > > + } else > > + head = (struct commit *)o; > > + } > > + /* Multiple origins don't work for diffstat. */ > > + if (negative_count > 1) > > + origin = NULL; > > Oh, and here it gets interesting. If you say > > git format-patch --cover-letter ^commit1 ^commit2 HEAD > > then origin will be set to NULL, and in the diffstat, instead of showing > _no_ diffstat, you show the same as if no negative ref was given (which > _should_ show a diff against the empty tree, but shows nothing currently). > > So I suggest changing "origin" to a commit_list, and have cover-letter > _not_ output a diffstat if more than one commits were given. > > Or as many diffstats. Whatever. I think it's better for now to leave the cover letter bare in all of the complicated cases, and add it when the core code can provide more info. > > + /* We can't generate a cover letter without any patches */ > > + if (!head) > > + return 0; > > Should this not > > return error("You want a cover letter for what," > "exactly?"); > > Hmm? I'm not sure. I decided to use the rule that, if there's nothing to format, you get nothing with no error, which is true before this series. But I think I'd rather get an error message if there's nothing to do. In any case, I don't think the result for no commits should depend on the cover letter option, in any case; the fact that these situations differ on whether the result is technically what you asked for or not isn't that significant. > > @@ -801,9 +896,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > > numbered = 1; > > if (numbered) > > rev.total = total + start_number - 1; > > - rev.add_signoff = add_signoff; > > if (in_reply_to) > > rev.ref_message_id = clean_message_id(in_reply_to); > > + if (cover_letter) { > > + if (thread) { > > + gen_message_id(&rev, "cover"); > > + } > > These curly brackets are sure distracting. Yup, fixed. > > + make_cover_letter(&rev, use_stdout, numbered, numbered_files, > > + origin, head); > > + total++; > > + start_number--; > > + } > > + rev.add_signoff = add_signoff; > > Why not sign off the cover letter, too? I think cover letters aren't normally signed off, since they don't contain any content that goes into the history. > BTW I had to chew over the thread handling quite a few minutes... maybe > you want to add a test for --cover-letter --thread, just to make sure it > does not get broken (and of course, to prove that it works with your > patches). Yeah, I probably want a comment explaining it, too, since I just chewed over it myself for a few minutes. Don't forget --in-reply-to=, which also works right. Actually, I guess it's a bit odd that --thread causes all of your messages to get Message-Id headers, of which at most one is referenced, while not having --thread means that none of them have IDs, even if they reference some other message by way of --in-reply-to=. I suppose that's because we might want to support having each message be a reply to the previous, which is not a style any of us like (I think), but some people somewhere like it, IIRC. -Daniel *This .sig left intentionally blank* ``` ## Junio C Hamano, 2008-02-18 21:09 Subject: Re: [PATCH 4/4] Add a --cover-letter option to format-patch Message-ID: <7vejbanf8l.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vejbanf8l.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Daniel Barkalow writes: >> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt >> > index 651efe6..53e6d2e 100644 >> > --- a/Documentation/git-format-patch.txt >> > +++ b/Documentation/git-format-patch.txt >> > @@ -17,6 +17,7 @@ SYNOPSIS >> > [--in-reply-to=Message-Id] [--suffix=.] >> > [--ignore-if-in-upstream] >> > [--subject-prefix=Subject-Prefix] >> > + [--cover-letter] >> > [ | ] >> >> Since you are already there, and it is really a small change, why not fix >> the whitespace? > > What *is* the correct whitespace for alignment in documentation? The last time I checked, AsciiDoc output did not change before and after running "expand" and then "unexpand" in all *.txt files in Documentation/ directory, so if it is indenting to multiple of 8, HT is fine. Don't we have Documentation/.gitattributes that says indentation to 8*N-th column must be done with N HTs these days? >> Why not sign off the cover letter, too? > > I think cover letters aren't normally signed off, since they don't contain > any content that goes into the history. Right. They do not contain any content that needs DCO. ``` ## Daniel Barkalow, 2008-02-18 21:17 Subject: Re: [PATCH 4/4] Add a --cover-letter option to format-patch Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0802181616530.5816%40iabervon.org In-Reply-To: <7vejbanf8l.fsf@gitster.siamese.dyndns.org> ``` On Mon, 18 Feb 2008, Junio C Hamano wrote: > Daniel Barkalow writes: > > >> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt > >> > index 651efe6..53e6d2e 100644 > >> > --- a/Documentation/git-format-patch.txt > >> > +++ b/Documentation/git-format-patch.txt > >> > @@ -17,6 +17,7 @@ SYNOPSIS > >> > [--in-reply-to=Message-Id] [--suffix=.] > >> > [--ignore-if-in-upstream] > >> > [--subject-prefix=Subject-Prefix] > >> > + [--cover-letter] > >> > [ | ] > >> > >> Since you are already there, and it is really a small change, why not fix > >> the whitespace? > > > > What *is* the correct whitespace for alignment in documentation? > > The last time I checked, AsciiDoc output did not change before > and after running "expand" and then "unexpand" in all *.txt > files in Documentation/ directory, so if it is indenting to > multiple of 8, HT is fine. > > Don't we have Documentation/.gitattributes that says indentation > to 8*N-th column must be done with N HTs these days? Ah, yes, that's right; I'll fix that block. -Daniel *This .sig left intentionally blank* ```