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

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

[Josh Triplett]

This provides a shorter and more convenient alias for
--subject-prefix='RFC PATCH'.

Add a test covering --rfc.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---

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'.

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

diff --git a/builtin/log.c b/builtin/log.c
index 92dc34d..48d6a38 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1112,6 +1112,13 @@ static int subject_prefix_callback(const struct option *opt, const char *arg,
 	return 0;
 }
 
+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;
+}
+
 static int numbered_cmdline_opt = 0;
 
 static int numbered_callback(const struct option *opt, const char *arg,
@@ -1419,6 +1426,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    N_("start numbering patches at <n> instead of 1")),
 		OPT_INTEGER('v', "reroll-count", &reroll_count,
 			    N_("mark the series as Nth re-roll")),
+		{ OPTION_CALLBACK, 0, "rfc", &rev, NULL,
+			    N_("Use [RFC PATCH] instead of [PATCH]"),
+			    PARSE_OPT_NOARG | PARSE_OPT_NONEG, rfc_callback },
 		{ OPTION_CALLBACK, 0, "subject-prefix", &rev, N_("prefix"),
 			    N_("Use [<prefix>] instead of [PATCH]"),
 			    PARSE_OPT_NONEG, subject_prefix_callback },
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index b0579dd..81b0498 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1073,6 +1073,15 @@ test_expect_success 'empty subject prefix does not have extra space' '
 	test_cmp expect actual
 '
 
+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
+'
+
 test_expect_success '--from=ident notices bogus ident' '
 	test_must_fail git format-patch -1 --stdout --from=foo >patch
 '

base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
-- 
git-series 0.8.10

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

[Jacob Keller]

On Fri, Sep 16, 2016 at 10:27 AM, Josh Triplett <josh@joshtriplett.org> wrote:
> This provides a shorter and more convenient alias for
> --subject-prefix='RFC PATCH'.
>
> Add a test covering --rfc.
>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
>
> 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'.
>

I agree!

Thanks,
Jake

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

[Jeff King]

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

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

[Josh Triplett]

On Fri, Sep 16, 2016 at 05:49:22PM -0700, Jeff King wrote:
> 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.

I haven't seen a significant number of variations on subject prefixes; I
can't think of any other prefix I've seen often enough to suggest an
option.

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

I agree; the short option space seems more contentious.  And in any
case, I find --rfc more ergonomic than "-T RFC". :)

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

Oops, thanks.  I'll send v2 shortly.

> > +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. :)

Nice idea; will do.

> > +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).

I copied this from a test immediately above it. :)

I can change it easily enough, though.

- Josh Triplett