Re: [PATCH v8 2/4] format-patch: add ability to use alt cover format

[Junio C Hamano <gitster@pobox.com>]

Mirko Faina <mroik@delayed.space> writes:

> Often when sending patch series there's a need to clarify to the
> reviewer what's the purpose of said series, since it might be difficult
> to understand it from reading the commits messages one by one.

Yes, that is the whole point of having a cover letter.

> "git format-patch" provides the useful "--cover-letter" flag to declare
> if we want it to generate a template for us to use. By default it will
> generate a "git shortlog" of the changes, which developers find less
> useful than they'd like, mainly because the shortlog groups commits by
> author, and gives no obvious chronological order.

Very true.

> To better reference relevant patches in the coverletter this patch
> introduces two new placeholders that can be used in the format spec:

"this patch introduces" -> "introduce" (imperative).

> %(count) and %(total). These are the chronological number of the patch
> in the series and the total amount of patches in the series. Note that
> the width of %(count) will always be the same witdh of %(total).

"total amount" -> "total number"?

"the width of %(count)..." -> "%(count) will zero-padded to the left
to match the number of digits in %(total)".

But the paragraph "To better reference ..." up to this point should
probably be moved a bit low?  The punchline "Give format-patch the
ability" to specify a custom format is the most important thing to
tell readers, and %(count)/%(total) is an implementation detail of
only one possible customized format.

> Give format-patch the ability to specify an alternative format spec
> through the "--cover-letter-format" option. This option either takes
> "shortlog", which is the current format, or a format spec prefixed with
> "log:".
>
> Example:
>     git format-patch --cover-letter \
>         --cover-letter-format="[%(count)/%(total)] %s (%an)" HEAD~3
>
>     [1/3] this is a commit summary (Mirko Faina)
>     [2/3] this is another commit summary (Mirko Faina)
>     ...

The example does not use a format spec 'prefixed with "log:"',
though?

> +--cover-letter-format=<format-spec>::
> +	Specify the format in which to generate the commit list of the patch
> +	series. This option is available if the user wants to use an
> +	alternative to the default `shortlog` format. The accepted values for

The second sentence reads funny.  The option is available whether
the user wants to use it or not.  I'd suggest dropping the sentence,
without which the paragraph reads just fine.

> +	format-spec are "shortlog" or a format string.
> +	e.g. `%s (%an)`

OK, so we are not requiring "log:"?  This robs extensibility from
future developers to introduce something other than "shortlog", no?
If the version of Git in 'next' supports "longlog" and user gives
"--cover-letter-format=longlog" to their version that does not yet
support it, it would be mistaken by the version of the code here as
a "log:longlog" without any placeholder that shows a fixed string
"longlog" for each commit in the series?  We'd rather want such an
input to cause failure, no?

> +	If defined, defaults to the `format.commitListFormat` configuration
> +	variable.

s/If defined, d\(efaults.*variable\)\./D\1, if defined./ would avoid
"if I define --cover-letter-format, why does it default to a
configuration?  do you mean 'if not given'?"

So, "If defined," -> "If not given" would be another possible
improvement.  I think I like it better, actually.

> +	This option is relevant only if a cover letter is generated.
> +

Hmph, an alternative that may make it easier to use is to make the
command line option _imply_ "--cover-letter", so that the user does
not have to give two similar looking command line options.

    git format-patch --cover-letter --cover-letter-format=...

Of course, the presence of the configuration variable should not
imply generation of a cover letter. I.e.

    git -c format.commitListInCoverLetterFormat=shortlog \
	    format-patch -1 HEAD

should not imply --cover-letter.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 21d6d0cd9e..631669c159 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -380,6 +380,64 @@ test_expect_success 'filename limit applies only to basename' '
>  	done
>  '
>  
> +test_expect_success 'cover letter with subject, author and count' '
> +	rm -rf patches &&
> +	test_when_finished "git reset --hard HEAD~1" &&
> +	test_when_finished "rm -rf patches result test_file" &&
> +	touch test_file &&
> +	git add test_file &&
> +	git commit -m "This is a subject" &&
> +	git format-patch --cover-letter \
> +	--cover-letter-format="[%(count)/%(total)] %s (%an)" -o patches HEAD~1 &&
> +	test_grep "^\[1/1\] This is a subject (A U Thor)$" patches/0000-cover-letter.patch
> +'
> +
> +cat > expected <<EOF
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +A U Thor (1):
> +  This is a subject
> +
> +
> +
> +
> +
> +
> +
> +
> +EOF

Many issues.

In modern tests (written within the past 10 years), we try not to
execute things outside text_expect_foo blocks.  The golden output
to compare with is customary called 'expect' (not 'expected').  A
redirection operator ">", "<<", etc. has a single SP before but no
SP after it, when there is no parameter interpolation happens in a
HERE document, quote the "EOF" marker to show the intention of the
author of the test that no parameter interpolation is expected.

i.e.,

    cat >expect <<\-EOF
	...
    EOF

and do so inside a set-up test_expect_success block.

Why do we have so many blank lines?  Are the number of blank lines
significant?  Such a hidden and hard to count dependency would hurt
maintainability of this test script.

> +test_expect_success 'cover letter shortlog' '
> +	test_when_finished "git reset --hard HEAD~1" &&
> +	test_when_finished "rm -rf patches expected test_file result" &&
> +	touch test_file &&
> +	git add test_file &&
> +	git commit -m "This is a subject" &&
> +	git format-patch --cover-letter --cover-letter-format=shortlog \
> +	-o patches HEAD~1 &&
> +	sed -n -e "/^A U Thor (1):$\|^  This is a subject$/!s/.*//; /.*/p" patches/0000-cover-letter.patch >result &&
> +	test_cmp expected result
> +'
> +
> +test_expect_success 'cover letter no format' '
> +	test_when_finished "git reset --hard HEAD~1" &&
> +	test_when_finished "rm -rf patches result test_file" &&
> +	touch test_file &&
> +	git add test_file &&
> +	git commit -m "This is a subject" &&
> +	git format-patch --no-cover-letter-format --cover-letter -o patches HEAD~1 &&
> +	sed -n -e "/^A U Thor/p;" patches/0000-cover-letter.patch >result &&
> +	test_line_count = 1 result
> +'
> +
>  test_expect_success 'reroll count' '
>  	rm -fr patches &&
>  	git format-patch -o patches --cover-letter --reroll-count 4 main..side >list &&
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 964e1f1569..4f760a7468 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2774,6 +2774,7 @@ test_expect_success PERL 'send-email' '
>  	test_completion "git send-email --cov" <<-\EOF &&
>  	--cover-from-description=Z
>  	--cover-letter Z
> +	--cover-letter-format=Z
>  	EOF
>  	test_completion "git send-email --val" <<-\EOF &&
>  	--validate Z