[PATCH RESEND] format-patch: add --description-file option

[PATCH RESEND] format-patch: add --description-file option

[Oswald Buddenhagen]

When formatting patches from a detached HEAD, there is no branch
description to derive the cover letter from. While with format-patch
one could post-process the generated file (which would be ugly enough),
scripting that with send-email would be *really* ugly. So add an option
to feed a description directly.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---

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(-)

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
-- 
2.40.0.152.g15d061e6df

Re: [PATCH RESEND] format-patch: add --description-file option

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> When formatting patches from a detached HEAD, there is no branch
> description to derive the cover letter from. While with format-patch
> one could post-process the generated file (which would be ugly enough),
> scripting that with send-email would be *really* ugly. So add an option
> to feed a description directly.

I think it makes sense to give the same set of features to those who
run format-patch from a detached HEAD as to those who run it on a
branch.  But personally I am not interested in a new feature that
encourages use of send-email as a front-end to format-patch, which I
consider is a misfeature, to make it easier for a set of patches
without final proofreading to be sent out.

Having said that, with my maintainer hat on, if we were to add a new
feature to format-patch, it makes sense to allow it passed through
send-email as well, since the (mis)feature already exists.

Please elaborate a bit more on the use case, though.

 * "there is no branch description to derive from" makes a reader
   wonder what the workflow would become if you could do "git branch
   --add-description HEAD" to prepare a description, which would
   imply that what is more desirable might be a feature enhancement
   of the "branch" command, not "format-patch" or "send-email", to
   allow you to describe what you are doing on the HEAD.

 * Or does the end-user have a branch with description already
   prepared, but for some untold reason the tip of the branch is
   checked out on a detached HEAD?

   If so, an obviously better alternative design would be to add a
   feature that passes a branch name to format-patch and tell it to
   pretend that the user is working on the branch.  That way, not
   just "description", any feature that makes the command use "which
   branch are we on?" information to enhance its behaviour we have
   right now or we will add to the command will all benefit.  For
   example, builtin/log.c::cmd_format_match() uses branch_name only
   for calling read_branch_desc() via prepare_cover_text(), but it
   is perfectly reasonable for us to make the range-diff default
   derived based on the reflog of the "current branch" on, and
   "pretend we were on this branch" may help you in such a case.

In other words, if a particular solution proposed (or not proposed)
is sensible or not heavily depends on how the end-user ends up
running format-patch (and sending the output out) on a detached
HEAD, and where does the end-user want to take the description
information from. No, the answer to the latter is not "the file
specified with the --description-file option"; that is not a valid
answer.  The question is about how the contents of that file is
populated and maintained.

A feature to specify the template used when generating the cover
letter may also work well for such a use case.  Among placeholders
to specify where to place auto-generated things like:

 - shortlog information
 - other ways to list commits in the series (e.g. listing of commit
   titles from "git log --oneline -r" may be more appropriate and
   readable than "shortlog" output especially when the series was
   written by multiple authors),
 - diffstat

there would be a placeholder to stuff branch description output (for
the normal case), and in your detached HEAD use case, you'd prepare
such a template without using branch description placeholder, but
instead prepare the description in place in the template before
running format-patch.  Which might actually be a better alternative.

But all of that depends on what the expected use case to support is.

Thanks.

[PATCH v2] format-patch: add --description-file option

[Oswald Buddenhagen]

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(-)

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
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v2] format-patch: add --description-file option

[Junio C Hamano]

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.

Re: [PATCH v2] format-patch: add --description-file option

[Oswald Buddenhagen]

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

Re: [PATCH v2] format-patch: add --description-file option

[Junio C Hamano]

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.

Re: [PATCH v2] format-patch: add --description-file option

[Oswald Buddenhagen]

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

[PATCH v3] format-patch: add --description-file option

[Oswald Buddenhagen]

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.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
i didn't implement --description for now, after all, as i'm not
confident that passing pre-formatted text on the command line works
reliably on windows (the calling and the called program need to agree
on the quoting conventions, which is a pretty sizable can of worms),
and passing it piece-wise is no particularly good fit for my use case.
---
v3:
- beautify test
- trim commit message
- use default strbuf_read_file() size hint
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            | 14 ++++++++++++++
 3 files changed, 36 insertions(+), 3 deletions(-)

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..967415e193 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, 0) < 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..e1d660130f 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1991,6 +1991,20 @@ 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" &&
+	cat >description.txt <<\-EOF &&
+	subject from file
+
+	body from file
+	EOF
+	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
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v3] format-patch: add --description-file option

[Junio C Hamano]

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.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---

The changed parts from the previous version all looked sensible.

Will replace and requeue.

Thanks.

Re: [PATCH v3] format-patch: add --description-file option

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> +test_expect_success 'cover letter with --description-file' '
> +	test_when_finished "rm -f description.txt" &&
> +	cat >description.txt <<\-EOF &&

"<<\-EOF"  ->  "<<-\EOF"

No need to resend as I've fixed it up locally already.

"make test" would have caught this, by the way.

Thanks.

Re: [PATCH v3] format-patch: add --description-file option

[Taylor Blau]

On Mon, Aug 21, 2023 at 03:05:35PM -0700, Junio C Hamano wrote:
> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
> > +test_expect_success 'cover letter with --description-file' '
> > +	test_when_finished "rm -f description.txt" &&
> > +	cat >description.txt <<\-EOF &&
>
> "<<\-EOF"  ->  "<<-\EOF"

I'm late to the party, but this was the only thing that I noticed with
the patch, too. The rest LGTM.

Thanks,
Taylor

Re: [PATCH v3] format-patch: add --description-file option

[Kristoffer Haugsbakk]

On Mon, Aug 21, 2023, at 19:07, Oswald Buddenhagen wrote:
> 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.

Thanks for implementing this. I've just written cover letter text in
separate files and copied them into the generated files every time. (I
don't use branch descriptions.) I've wanted some convenient way to feed
these messages in, and if I end up writing a cover letter again I'll most
probably be using this new option.

Cheers

-- 
Kristoffer Haugsbakk

Re: [PATCH v3] format-patch: add --description-file option

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> On Mon, Aug 21, 2023, at 19:07, Oswald Buddenhagen wrote:
>> 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.
>
> Thanks for implementing this. I've just written cover letter text in
> separate files and copied them into the generated files every time. (I
> don't use branch descriptions.) I've wanted some convenient way to feed
> these messages in, and if I end up writing a cover letter again I'll most
> probably be using this new option.

Thanks for a positive feedback.  The changes is already in 'master'
since the beginning of this month or so and its way to be part of
the next release, I believe.

Re: [PATCH v3] format-patch: add --description-file option

[Kristoffer Haugsbakk]

On Mon, Sep 25, 2023, at 21:01, Junio C Hamano wrote:
> Thanks for a positive feedback.  The changes is already in 'master'
> since the beginning of this month or so and its way to be part of
> the next release, I believe.

Yes, I tested it yesterday and it works (in conjunction with
`--cover-from-description=subject`) exactly like I want it to. :D

-- 
Kristoffer Haugsbakk