Re: [PATCH] format-patch: Add --rfc for the common case of [RFC PATCH]

[Jeff King <peff@peff.net>]

On Fri, Sep 16, 2016 at 10:27:45AM -0700, Josh Triplett wrote:

> By far, the most common subject-prefix I've seen other than "PATCH" is
> "RFC PATCH" (or occasionally "PATCH RFC").  Seems worth optimizing for
> the common case, to avoid having to spell it out the long way as
> --subject-prefix='RFC PATCH'.

"RFC" is the most common one for me, too. And if it ends here, I'm OK
with it. But I'm a little worried with ending up with a proliferation of
options.

If we had a short-option for --subject-prefix, then:

  -P RFC

is not so bad compared to "--rfc". But if you want to spell it as "RFC
PATCH" that's getting a bit longer. We could have a short option for
"tag this in the subject prefix _in addition_ to writing PATCH". And
then you could do:

  -T RFC

I dunno. One other thing to consider is that format-patch takes
arbitrary diff options, so we'd want to avoid stomping on them with any
short options (which is why I used "-T" instead of "-t", though I find
it unlikely that many people use the latter with format-patch). That's a
point in favor of --rfc, I think.

>  builtin/log.c           | 10 ++++++++++
>  t/t4014-format-patch.sh |  9 +++++++++
>  2 files changed, 19 insertions(+), 0 deletions(-)

Documentation?

> +static int rfc_callback(const struct option *opt, const char *arg, int unset)
> +{
> +	subject_prefix = 1;
> +	((struct rev_info *)opt->value)->subject_prefix = xstrdup("RFC PATCH");
> +	return 0;
> +}

I was going to complain that you don't free() the previous value, but
actually the other callers do not xstrdup() in the first place (and we
do not need to do so here, either, as it's a string literal). We
actually _do_ allocate a new copy when reading the value from config,
but it's probably not a big deal in practice to leak that.

I also wonder if you could implement this as just:

  return subject_prefix_callback(opt, "RFC PATCH", unset);

And then if you write the documentation as:

  --rfc::
	Behave as if --subject-prefix="RFC PATCH" was specified.

then it will be trivially correct. :)

> +cat >expect <<'EOF'
> +Subject: [RFC PATCH 1/1] header with . in it
> +EOF
> +test_expect_success '--rfc' '
> +	git format-patch -n -1 --stdout --rfc >patch &&
> +	grep ^Subject: patch >actual &&
> +	test_cmp expect actual
> +'

Our usual style these days is to set up expectations inside the test
blocks (and use "<<-" to get nice indentation; we also typically use
"\EOF" but that's purely style).

-Peff