| Message ID | 20230809171530.2564724-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] format-patch: add --description-file option | expand |
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > This patch makes it possible to directly feed a branch description to > derive the cover letter from. The use case is formatting dynamically > created temporary commits which are not referenced anywhere. > > The most obvious alternative would be creating a temporary branch and > setting a description on it, but that doesn't seem particularly elegant. Elegance is quite subjective, but not having to create a temporary branch is a good thing in itself. Everything after ", but that doesn't" is better left out of the message here. In the longer term, the templating system is the way to go to achieve more flexibility that is more general than the single problem this patch is trying to solve, but we do not always have to wait for such a most general solution. > +--description-file=<file>:: > + Use the contents of <file> instead of the branch's description > + for generating the cover letter. OK. > diff --git a/builtin/log.c b/builtin/log.c > index 1b119eaf0b..9c4738bbde 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1255,7 +1255,15 @@ static void show_diffstat(struct rev_info *rev, > fprintf(rev->diffopt.file, "\n"); > } > > +static void read_desc_file(struct strbuf *buf, const char *desc_file) > +{ > + if (strbuf_read_file(buf, desc_file, 2000) < 0) > + die_errno(_("unable to read branch description file '%s'"), > + desc_file); > +} You would probably want to do "2000" -> "0" here. > static void prepare_cover_text(struct pretty_print_context *pp, > + const char *description_file, > const char *branch_name, This is kind of suboptimal, but let's let it pass. A better design is to pass the description string itself to this function and the make_cover_letter() function, and have the higher level callers of the callchain prepare the either read_desc_file() or read_branch_desc() to prepare that string before calling into the callchain. Such a division of labor between the callers and this function will allow us to more easily add another option to the command, to feed the description string itself (instead of having to create a temporary file and storing the description in it). But such a clean-up can be safely left for now and be done after the dust settles. > @@ -1269,7 +1277,9 @@ static void prepare_cover_text(struct pretty_print_context *pp, > if (cover_from_description_mode == COVER_FROM_NONE) > goto do_pp; > > - if (branch_name && *branch_name) > + if (description_file && *description_file) > + read_desc_file(&description_sb, description_file); > + else if (branch_name && *branch_name) > read_branch_desc(&description_sb, branch_name); This allows use of a custom description to override the branch description, which makes quite a lot of sense. > @@ -1315,6 +1325,7 @@ static void get_notes_args(struct strvec *arg, struct rev_info *rev) > static void make_cover_letter(struct rev_info *rev, int use_separate_file, > struct commit *origin, > int nr, struct commit **list, > + const char *description_file, > const char *branch_name, I've already touched on this. > int quiet) > { > @@ -1354,7 +1365,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, > pp.rev = rev; > pp.print_email_subject = 1; > pp_user_info(&pp, NULL, &sb, committer, encoding); > - prepare_cover_text(&pp, branch_name, &sb, encoding, need_8bit_cte); > + prepare_cover_text(&pp, description_file, branch_name, &sb, > + encoding, need_8bit_cte); > fprintf(rev->diffopt.file, "%s\n", sb.buf); > > strbuf_release(&sb); > @@ -1895,6 +1907,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > int quiet = 0; > const char *reroll_count = NULL; > char *cover_from_description_arg = NULL; > + char *description_file = NULL; > char *branch_name = NULL; > char *base_commit = NULL; > struct base_tree_info bases; > @@ -1938,6 +1951,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > OPT_STRING(0, "cover-from-description", &cover_from_description_arg, > N_("cover-from-description-mode"), > N_("generate parts of a cover letter based on a branch's description")), > + OPT_FILENAME(0, "description-file", &description_file, > + N_("use branch description from file")), > OPT_CALLBACK_F(0, "subject-prefix", &rev, N_("prefix"), > N_("use [<prefix>] instead of [PATCH]"), > PARSE_OPT_NONEG, subject_prefix_callback), > @@ -2323,7 +2338,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > if (thread) > gen_message_id(&rev, "cover"); > make_cover_letter(&rev, !!output_directory, > - origin, nr, list, branch_name, quiet); > + origin, nr, list, description_file, branch_name, 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 3cf2b7a7fb..b31401876b 100755 > --- a/t/t4014-format-patch.sh > +++ b/t/t4014-format-patch.sh > @@ -1991,6 +1991,18 @@ test_expect_success 'cover letter using branch description (6)' ' > grep hello actual > ' > > +test_expect_success 'cover letter with --description-file' ' > + test_when_finished "rm -f description.txt" && > + echo "subject from file > + > +body from file" > description.txt && It is more conventional to write a multi-line file like so: cat >description.txt <<\-EOF && subject from file body from file EOF which would allow readers not to get distracted by the unindented second line. Also redirection operator ">" (or "<") sticks to the target file in our coding style. > + git checkout rebuild-1 && > + git format-patch --stdout --cover-letter --cover-from-description auto \ > + --description-file description.txt main >actual && > + grep "^Subject: \[PATCH 0/2\] subject from file$" actual && > + grep "^body from file$" actual Nice. > +' > + > test_expect_success 'cover letter with nothing' ' > git format-patch --stdout --cover-letter >actual && > test_line_count = 0 actual Thanks.
On Fri, Aug 11, 2023 at 02:38:05PM -0700, Junio C Hamano wrote: >> + if (strbuf_read_file(buf, desc_file, 2000) < 0) > >You would probably want to do "2000" -> "0" here. > hmm, yeah, i wonder where i got it from, given that there is no precedent. i suppose i simply thought that 2k is a reasonably expectable max size for a description. if you think the default 8k hint is a better idea, then let's go with it. >> static void prepare_cover_text(struct pretty_print_context *pp, >> + const char *description_file, >> const char *branch_name, > >This is kind of suboptimal, but let's let it pass. > >A better design is to pass the description string itself to this >function and the make_cover_letter() function, and have the higher >level callers of the callchain prepare the either read_desc_file() >or read_branch_desc() to prepare that string before calling into the >callchain. > there is only one caller, and doing this change would essentially result in inlining prepare_cover_text(). probably not the best outcome. >Such a division of labor between the callers and this >function will allow us to more easily add another option to the >command, to feed the description string itself (instead of having to >create a temporary file and storing the description in it). > that's a good point. in fact, passing in the description directly would probably fit my use case better ... i just happened to already have the code for creating that temp file anyway (for editing), so i didn't give it a second thought. i can add both options in the same go, given that it's almost no code. regards
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > On Fri, Aug 11, 2023 at 02:38:05PM -0700, Junio C Hamano wrote: >>> + if (strbuf_read_file(buf, desc_file, 2000) < 0) >> >>You would probably want to do "2000" -> "0" here. >> > hmm, yeah, i wonder where i got it from, given that there is no > precedent. i suppose i simply thought that 2k is a reasonably > expectable max size for a description. if you think the default 8k > hint is a better idea, then let's go with it. The suggestion was not about 2000 vs 8kiB, though it seems we stick to power of 2 everywhere we are explicit. Unless we know the exact size from .st_size, that is. It was primarily about this code not having any need to express its own preference and go with whatever is the default. > that's a good point. in fact, passing in the description directly > would probably fit my use case better ... i just happened to already > have the code for creating that temp file anyway (for editing), so i > didn't give it a second thought. i can add both options in the same > go, given that it's almost no code. One thing that you may have to be careful about, if you also take strings directly from the command line, is what to do with multiple of them. "git commit -m A -m B" that makes A and B separate paragraphs with a break between them, I would think, would serve as a good model that end-users already understand well. Thanks.
On Sat, Aug 12, 2023 at 01:21:46AM -0700, Junio C Hamano wrote: >Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > >> On Fri, Aug 11, 2023 at 02:38:05PM -0700, Junio C Hamano wrote: >>>> + if (strbuf_read_file(buf, desc_file, 2000) < 0) >>> >>>You would probably want to do "2000" -> "0" here. >>> >> hmm, yeah, i wonder where i got it from, given that there is no >> precedent. i suppose i simply thought that 2k is a reasonably >> expectable max size for a description. if you think the default 8k >> hint is a better idea, then let's go with it. > >The suggestion was not about 2000 vs 8kiB, > i know. i just mentioned the default for reference. it seems "severely" oversized for the task - not that it would actually matter. >though it seems we stick >to power of 2 everywhere we are explicit. Unless we know the exact >size from .st_size, that is. > >It was primarily about this code not having any need to express its >own preference and go with whatever is the default. > arguably, just about every other instance which uses a fixed hint doesn't need to, yet some of them do. it's somewhat obvious in the case of "tiny" hints, but there are also some cases of 1k. and the sequencer's do_commit() uses 2k for the message file, which is "a funny coincidence". so i wonder whether there is some standard to go by. >> that's a good point. in fact, passing in the description directly >> would probably fit my use case better ... i just happened to already >> have the code for creating that temp file anyway (for editing), so i >> didn't give it a second thought. i can add both options in the same >> go, given that it's almost no code. > >One thing that you may have to be careful about, if you also take >strings directly from the command line, is what to do with multiple >of them. "git commit -m A -m B" that makes A and B separate >paragraphs with a break between them, I would think, would serve as >a good model that end-users already understand well. > no worries about that, i'd just copy from commit anyway. however, this points out a potential problem, which makes me have second thoughts about my use case ... i would want to pass the entire contents in one argument, newlines and quotes included. i know that this is inherently ok on unix, but i wonder whether it would work reliably on windows (the wrapper script is written in perl)? regards
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 373b46fc0d..8e515c7dbb 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -215,6 +215,10 @@ is greater than 100 bytes, then the mode will be `message`, otherwise If `<mode>` is `none`, both the cover letter subject and body will be populated with placeholder text. +--description-file=<file>:: + Use the contents of <file> instead of the branch's description + for generating the cover letter. + --subject-prefix=<subject prefix>:: Instead of the standard '[PATCH]' prefix in the subject line, instead use '[<subject prefix>]'. This diff --git a/builtin/log.c b/builtin/log.c index 1b119eaf0b..9c4738bbde 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1255,7 +1255,15 @@ static void show_diffstat(struct rev_info *rev, fprintf(rev->diffopt.file, "\n"); } +static void read_desc_file(struct strbuf *buf, const char *desc_file) +{ + if (strbuf_read_file(buf, desc_file, 2000) < 0) + die_errno(_("unable to read branch description file '%s'"), + desc_file); +} + static void prepare_cover_text(struct pretty_print_context *pp, + const char *description_file, const char *branch_name, struct strbuf *sb, const char *encoding, @@ -1269,7 +1277,9 @@ static void prepare_cover_text(struct pretty_print_context *pp, if (cover_from_description_mode == COVER_FROM_NONE) goto do_pp; - if (branch_name && *branch_name) + if (description_file && *description_file) + read_desc_file(&description_sb, description_file); + else if (branch_name && *branch_name) read_branch_desc(&description_sb, branch_name); if (!description_sb.len) goto do_pp; @@ -1315,6 +1325,7 @@ static void get_notes_args(struct strvec *arg, struct rev_info *rev) static void make_cover_letter(struct rev_info *rev, int use_separate_file, struct commit *origin, int nr, struct commit **list, + const char *description_file, const char *branch_name, int quiet) { @@ -1354,7 +1365,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, pp.rev = rev; pp.print_email_subject = 1; pp_user_info(&pp, NULL, &sb, committer, encoding); - prepare_cover_text(&pp, branch_name, &sb, encoding, need_8bit_cte); + prepare_cover_text(&pp, description_file, branch_name, &sb, + encoding, need_8bit_cte); fprintf(rev->diffopt.file, "%s\n", sb.buf); strbuf_release(&sb); @@ -1895,6 +1907,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) int quiet = 0; const char *reroll_count = NULL; char *cover_from_description_arg = NULL; + char *description_file = NULL; char *branch_name = NULL; char *base_commit = NULL; struct base_tree_info bases; @@ -1938,6 +1951,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) OPT_STRING(0, "cover-from-description", &cover_from_description_arg, N_("cover-from-description-mode"), N_("generate parts of a cover letter based on a branch's description")), + OPT_FILENAME(0, "description-file", &description_file, + N_("use branch description from file")), OPT_CALLBACK_F(0, "subject-prefix", &rev, N_("prefix"), N_("use [<prefix>] instead of [PATCH]"), PARSE_OPT_NONEG, subject_prefix_callback), @@ -2323,7 +2338,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (thread) gen_message_id(&rev, "cover"); make_cover_letter(&rev, !!output_directory, - origin, nr, list, branch_name, quiet); + origin, nr, list, description_file, branch_name, 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 3cf2b7a7fb..b31401876b 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -1991,6 +1991,18 @@ test_expect_success 'cover letter using branch description (6)' ' grep hello actual ' +test_expect_success 'cover letter with --description-file' ' + test_when_finished "rm -f description.txt" && + echo "subject from file + +body from file" > description.txt && + git checkout rebuild-1 && + git format-patch --stdout --cover-letter --cover-from-description auto \ + --description-file description.txt main >actual && + grep "^Subject: \[PATCH 0/2\] subject from file$" actual && + grep "^body from file$" actual +' + test_expect_success 'cover letter with nothing' ' git format-patch --stdout --cover-letter >actual && test_line_count = 0 actual
This patch makes it possible to directly feed a branch description to derive the cover letter from. The use case is formatting dynamically created temporary commits which are not referenced anywhere. The most obvious alternative would be creating a temporary branch and setting a description on it, but that doesn't seem particularly elegant. One could also post-process the cover letter. This would be conceptually ugly due to having to make assumptions about its format, and hooking this into send-email would be *really* ugly. As a variation of that, one could make it possible to feed a template for the cover letter, into which the description could be embedded for the given use case. This would still need to make assumptions about the template, so while not as fragile, it would be still conceptually ugly, and simply not a good fit for the given use case. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- v2: - improve commit message Cc: Jeff King <peff@peff.net> Cc: Taylor Blau <me@ttaylorr.com> Cc: Derrick Stolee <derrickstolee@github.com> Cc: Junio C Hamano <gitster@pobox.com> --- Documentation/git-format-patch.txt | 4 ++++ builtin/log.c | 21 ++++++++++++++++++--- t/t4014-format-patch.sh | 12 ++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-)