{"thread":{"id":"44098","subject":"[PATCH] format-patch: Add --rfc for the common case of [RFC PATCH]","startedAt":"2016-09-16T17:27:55Z","lastAt":"2016-09-17T04:20:26Z","messageCount":4,"participants":["Josh Triplett","Jacob Keller","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"302056","messageId":"28c5d2c59851279858df22e844c6ff7c09f33199.1474046573.git-series.josh@joshtriplett.org","threadId":"44098","inReplyTo":null,"subject":"[PATCH] format-patch: Add --rfc for the common case of [RFC PATCH]","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-09-16T17:27:45Z","receivedAt":"2016-09-16T17:27:55Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"This provides a shorter and more convenient alias for\n--subject-prefix='RFC PATCH'.\n\nAdd a test covering --rfc.\n\nSigned-off-by: Josh Triplett <josh@joshtriplett.org>\n---\n\nBy far, the most common subject-prefix I've seen other than \"PATCH\" is\n\"RFC PATCH\" (or occasionally \"PATCH RFC\").  Seems worth optimizing for\nthe common case, to avoid having to spell it out the long way as\n--subject-prefix='RFC PATCH'.\n\n builtin/log.c           | 10 ++++++++++\n t/t4014-format-patch.sh |  9 +++++++++\n 2 files changed, 19 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 92dc34d..48d6a38 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1112,6 +1112,13 @@ static int subject_prefix_callback(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n+static int rfc_callback(const struct option *opt, const char *arg, int unset)\n+{\n+\tsubject_prefix = 1;\n+\t((struct rev_info *)opt->value)->subject_prefix = xstrdup(\"RFC PATCH\");\n+\treturn 0;\n+}\n+\n static int numbered_cmdline_opt = 0;\n \n static int numbered_callback(const struct option *opt, const char *arg,\n@@ -1419,6 +1426,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t    N_(\"start numbering patches at <n> instead of 1\")),\n \t\tOPT_INTEGER('v', \"reroll-count\", &reroll_count,\n \t\t\t    N_(\"mark the series as Nth re-roll\")),\n+\t\t{ OPTION_CALLBACK, 0, \"rfc\", &rev, NULL,\n+\t\t\t    N_(\"Use [RFC PATCH] instead of [PATCH]\"),\n+\t\t\t    PARSE_OPT_NOARG | PARSE_OPT_NONEG, rfc_callback },\n \t\t{ OPTION_CALLBACK, 0, \"subject-prefix\", &rev, N_(\"prefix\"),\n \t\t\t    N_(\"Use [<prefix>] instead of [PATCH]\"),\n \t\t\t    PARSE_OPT_NONEG, subject_prefix_callback },\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex b0579dd..81b0498 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1073,6 +1073,15 @@ test_expect_success 'empty subject prefix does not have extra space' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <<'EOF'\n+Subject: [RFC PATCH 1/1] header with . in it\n+EOF\n+test_expect_success '--rfc' '\n+\tgit format-patch -n -1 --stdout --rfc >patch &&\n+\tgrep ^Subject: patch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--from=ident notices bogus ident' '\n \ttest_must_fail git format-patch -1 --stdout --from=foo >patch\n '\n\nbase-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b\n-- \ngit-series 0.8.10\n"},{"id":"302057","messageId":"CA+P7+xo5vijUhWvoM4q_sW5Ps4rii7=wFBREP-Q_bn+mBTbCiw@mail.gmail.com","threadId":"44098","inReplyTo":"28c5d2c59851279858df22e844c6ff7c09f33199.1474046573.git-series.josh@joshtriplett.org","subject":"Re: [PATCH] format-patch: Add --rfc for the common case of [RFC PATCH]","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-09-16T17:34:28Z","receivedAt":"2016-09-16T17:35:55Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Sep 16, 2016 at 10:27 AM, Josh Triplett <josh@joshtriplett.org> wrote:\n> This provides a shorter and more convenient alias for\n> --subject-prefix='RFC PATCH'.\n>\n> Add a test covering --rfc.\n>\n> Signed-off-by: Josh Triplett <josh@joshtriplett.org>\n> ---\n>\n> By far, the most common subject-prefix I've seen other than \"PATCH\" is\n> \"RFC PATCH\" (or occasionally \"PATCH RFC\").  Seems worth optimizing for\n> the common case, to avoid having to spell it out the long way as\n> --subject-prefix='RFC PATCH'.\n>\n\nI agree!\n\nThanks,\nJake\n"},{"id":"302093","messageId":"20160917004922.rd6g3bqajyt4iyjm@sigill.intra.peff.net","threadId":"44098","inReplyTo":"28c5d2c59851279858df22e844c6ff7c09f33199.1474046573.git-series.josh@joshtriplett.org","subject":"Re: [PATCH] format-patch: Add --rfc for the common case of [RFC PATCH]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-17T00:49:22Z","receivedAt":"2016-09-17T00:50:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 16, 2016 at 10:27:45AM -0700, Josh Triplett wrote:\n\n> By far, the most common subject-prefix I've seen other than \"PATCH\" is\n> \"RFC PATCH\" (or occasionally \"PATCH RFC\").  Seems worth optimizing for\n> the common case, to avoid having to spell it out the long way as\n> --subject-prefix='RFC PATCH'.\n\n\"RFC\" is the most common one for me, too. And if it ends here, I'm OK\nwith it. But I'm a little worried with ending up with a proliferation of\noptions.\n\nIf we had a short-option for --subject-prefix, then:\n\n  -P RFC\n\nis not so bad compared to \"--rfc\". But if you want to spell it as \"RFC\nPATCH\" that's getting a bit longer. We could have a short option for\n\"tag this in the subject prefix _in addition_ to writing PATCH\". And\nthen you could do:\n\n  -T RFC\n\nI dunno. One other thing to consider is that format-patch takes\narbitrary diff options, so we'd want to avoid stomping on them with any\nshort options (which is why I used \"-T\" instead of \"-t\", though I find\nit unlikely that many people use the latter with format-patch). That's a\npoint in favor of --rfc, I think.\n\n>  builtin/log.c           | 10 ++++++++++\n>  t/t4014-format-patch.sh |  9 +++++++++\n>  2 files changed, 19 insertions(+), 0 deletions(-)\n\nDocumentation?\n\n> +static int rfc_callback(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tsubject_prefix = 1;\n> +\t((struct rev_info *)opt->value)->subject_prefix = xstrdup(\"RFC PATCH\");\n> +\treturn 0;\n> +}\n\nI was going to complain that you don't free() the previous value, but\nactually the other callers do not xstrdup() in the first place (and we\ndo not need to do so here, either, as it's a string literal). We\nactually _do_ allocate a new copy when reading the value from config,\nbut it's probably not a big deal in practice to leak that.\n\nI also wonder if you could implement this as just:\n\n  return subject_prefix_callback(opt, \"RFC PATCH\", unset);\n\nAnd then if you write the documentation as:\n\n  --rfc::\n\tBehave as if --subject-prefix=\"RFC PATCH\" was specified.\n\nthen it will be trivially correct. :)\n\n> +cat >expect <<'EOF'\n> +Subject: [RFC PATCH 1/1] header with . in it\n> +EOF\n> +test_expect_success '--rfc' '\n> +\tgit format-patch -n -1 --stdout --rfc >patch &&\n> +\tgrep ^Subject: patch >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nOur usual style these days is to set up expectations inside the test\nblocks (and use \"<<-\" to get nice indentation; we also typically use\n\"\\EOF\" but that's purely style).\n\n-Peff\n"},{"id":"302097","messageId":"20160917042010.xm3amjk7cqocorxh@x","threadId":"44098","inReplyTo":"20160917004922.rd6g3bqajyt4iyjm@sigill.intra.peff.net","subject":"Re: [PATCH] format-patch: Add --rfc for the common case of [RFC PATCH]","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-09-17T04:20:11Z","receivedAt":"2016-09-17T04:20:26Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Fri, Sep 16, 2016 at 05:49:22PM -0700, Jeff King wrote:\n> On Fri, Sep 16, 2016 at 10:27:45AM -0700, Josh Triplett wrote:\n> \n> > By far, the most common subject-prefix I've seen other than \"PATCH\" is\n> > \"RFC PATCH\" (or occasionally \"PATCH RFC\").  Seems worth optimizing for\n> > the common case, to avoid having to spell it out the long way as\n> > --subject-prefix='RFC PATCH'.\n> \n> \"RFC\" is the most common one for me, too. And if it ends here, I'm OK\n> with it. But I'm a little worried with ending up with a proliferation of\n> options.\n\nI haven't seen a significant number of variations on subject prefixes; I\ncan't think of any other prefix I've seen often enough to suggest an\noption.\n\n> If we had a short-option for --subject-prefix, then:\n> \n>   -P RFC\n> \n> is not so bad compared to \"--rfc\". But if you want to spell it as \"RFC\n> PATCH\" that's getting a bit longer. We could have a short option for\n> \"tag this in the subject prefix _in addition_ to writing PATCH\". And\n> then you could do:\n> \n>   -T RFC\n> \n> I dunno. One other thing to consider is that format-patch takes\n> arbitrary diff options, so we'd want to avoid stomping on them with any\n> short options (which is why I used \"-T\" instead of \"-t\", though I find\n> it unlikely that many people use the latter with format-patch). That's a\n> point in favor of --rfc, I think.\n\nI agree; the short option space seems more contentious.  And in any\ncase, I find --rfc more ergonomic than \"-T RFC\". :)\n\n> >  builtin/log.c           | 10 ++++++++++\n> >  t/t4014-format-patch.sh |  9 +++++++++\n> >  2 files changed, 19 insertions(+), 0 deletions(-)\n> \n> Documentation?\n\nOops, thanks.  I'll send v2 shortly.\n\n> > +static int rfc_callback(const struct option *opt, const char *arg, int unset)\n> > +{\n> > +\tsubject_prefix = 1;\n> > +\t((struct rev_info *)opt->value)->subject_prefix = xstrdup(\"RFC PATCH\");\n> > +\treturn 0;\n> > +}\n> \n> I was going to complain that you don't free() the previous value, but\n> actually the other callers do not xstrdup() in the first place (and we\n> do not need to do so here, either, as it's a string literal). We\n> actually _do_ allocate a new copy when reading the value from config,\n> but it's probably not a big deal in practice to leak that.\n> \n> I also wonder if you could implement this as just:\n> \n>   return subject_prefix_callback(opt, \"RFC PATCH\", unset);\n> \n> And then if you write the documentation as:\n> \n>   --rfc::\n> \tBehave as if --subject-prefix=\"RFC PATCH\" was specified.\n> \n> then it will be trivially correct. :)\n\nNice idea; will do.\n\n> > +cat >expect <<'EOF'\n> > +Subject: [RFC PATCH 1/1] header with . in it\n> > +EOF\n> > +test_expect_success '--rfc' '\n> > +\tgit format-patch -n -1 --stdout --rfc >patch &&\n> > +\tgrep ^Subject: patch >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> Our usual style these days is to set up expectations inside the test\n> blocks (and use \"<<-\" to get nice indentation; we also typically use\n> \"\\EOF\" but that's purely style).\n\nI copied this from a test immediately above it. :)\n\nI can change it easily enough, though.\n\n- Josh Triplett\n"}]}