{"thread":{"id":"60157","subject":"[PATCH] builtin/log.c: prepend \"RFC\" on --rfc","startedAt":"2023-08-28T12:58:18Z","lastAt":"2023-08-28T18:13:11Z","messageCount":7,"participants":["Drew DeVault","Jeff King","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"481044","messageId":"20230828125132.25144-1-sir@cmpwn.com","threadId":"60157","inReplyTo":null,"subject":"[PATCH] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2023-08-28T12:50:34Z","receivedAt":"2023-08-28T12:58:18Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"Rather than replacing the configured subject prefix (either through the\ngit config or command line) entirely with \"RFC PATCH\", this change\nprepends RFC to whatever subject prefix was already in use.\n\nThis is useful, for example, when a user is working on a repository that\nhas a subject prefix considered to disambiguate patches:\n\n\tgit config format.subjectPrefix 'PATCH my-project'\n\nPrior to this change, formatting patches with --rfc would lose the\n'my-project' information.\n\nSigned-off-by: Drew DeVault <sir@cmpwn.com>\n---\nImplementation note: this introduces a small memory leak, but freeing it\nrequires a non-trivial amount of refactoring and some dubious choices\nthat I was not sure of for a small patch; and it seems like memory leaks\nin this context are tolerated anyway from a perusal of the existing\ncode.\n\n Documentation/git-format-patch.txt |  6 +++---\n builtin/log.c                      | 15 ++++++++++++++-\n t/t4014-format-patch.sh            |  9 +++++++++\n 3 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 373b46fc0d..fdc52cf826 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -229,9 +229,9 @@ populated with placeholder text.\n \tvariable, or 64 if unconfigured.\n \n --rfc::\n-\tAlias for `--subject-prefix=\"RFC PATCH\"`. RFC means \"Request For\n-\tComments\"; use this when sending an experimental patch for\n-\tdiscussion rather than application.\n+\tPrepends \"RFC\" to the subject prefix (producing \"RFC PATCH\" by\n+\tdefault). RFC means \"Request For Comments\"; use this when sending\n+\tan experimental patch for discussion rather than application.\n \n -v <n>::\n --reroll-count=<n>::\ndiff --git a/builtin/log.c b/builtin/log.c\nindex db3a88bfe9..d986faebed 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1476,9 +1476,22 @@ static int subject_prefix_callback(const struct option *opt, const char *arg,\n \n static int rfc_callback(const struct option *opt, const char *arg, int unset)\n {\n+\tint n;\n+\tchar *prefix;\n+\tconst char *prev;\n+\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n-\treturn subject_prefix_callback(opt, \"RFC PATCH\", unset);\n+\n+\tprev = ((struct rev_info *)opt->value)->subject_prefix;\n+\tassert(prev != NULL);\n+\tn = snprintf(NULL, 0, \"RFC %s\", prev);\n+\tassert(n > 0);\n+\tprefix = xmalloc(n + 1);\n+\tn = snprintf(prefix, n + 1, \"RFC %s\", prev);\n+\tassert(n > 0);\n+\n+\treturn subject_prefix_callback(opt, prefix, unset);\n }\n \n static int numbered_cmdline_opt = 0;\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 3cf2b7a7fb..a7fe839683 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1377,6 +1377,15 @@ test_expect_success '--rfc' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--rfc does not overwrite prefix' '\n+\tcat >expect <<-\\EOF &&\n+\tSubject: [RFC PATCH foobar 1/1] header with . in it\n+\tEOF\n+\tgit format-patch -n -1 --stdout --subject-prefix \"PATCH foobar\" --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-- \n2.42.0\n\n"},{"id":"481049","messageId":"20230828144215.GA2537587@coredump.intra.peff.net","threadId":"60157","inReplyTo":"20230828125132.25144-1-sir@cmpwn.com","subject":"Re: [PATCH] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-28T14:42:15Z","receivedAt":"2023-08-28T14:43:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 28, 2023 at 02:50:34PM +0200, Drew DeVault wrote:\n\n> Rather than replacing the configured subject prefix (either through the\n> git config or command line) entirely with \"RFC PATCH\", this change\n> prepends RFC to whatever subject prefix was already in use.\n> \n> This is useful, for example, when a user is working on a repository that\n> has a subject prefix considered to disambiguate patches:\n> \n> \tgit config format.subjectPrefix 'PATCH my-project'\n> \n> Prior to this change, formatting patches with --rfc would lose the\n> 'my-project' information.\n\nThis sounds like a good change to me. It would be backwards-incompatible\nfor anybody expecting:\n\n  git format-patch --subject=foo --rfc\n\nto override the --subject line, but that seems rather unlikely.\n\n> Implementation note: this introduces a small memory leak, but freeing it\n> requires a non-trivial amount of refactoring and some dubious choices\n> that I was not sure of for a small patch; and it seems like memory leaks\n> in this context are tolerated anyway from a perusal of the existing\n> code.\n\nWe do have a lot of small leaks like this, but we've been trying to\nclean them up slowly. There's some infrastructure in the test suite for\nmarking scripts as leak-free, but t4014 is not yet there, so this\nwon't cause CI to complain at this point.\n\nIt is tempting while we are here and thinking about it to put in an easy\nhack, like storing the allocated string in a static variable.\n\n>  static int rfc_callback(const struct option *opt, const char *arg, int unset)\n>  {\n> +\tint n;\n> +\tchar *prefix;\n> +\tconst char *prev;\n> +\n>  \tBUG_ON_OPT_NEG(unset);\n>  \tBUG_ON_OPT_ARG(arg);\n> -\treturn subject_prefix_callback(opt, \"RFC PATCH\", unset);\n> +\n> +\tprev = ((struct rev_info *)opt->value)->subject_prefix;\n> +\tassert(prev != NULL);\n> +\tn = snprintf(NULL, 0, \"RFC %s\", prev);\n> +\tassert(n > 0);\n> +\tprefix = xmalloc(n + 1);\n> +\tn = snprintf(prefix, n + 1, \"RFC %s\", prev);\n> +\tassert(n > 0);\n> +\n> +\treturn subject_prefix_callback(opt, prefix, unset);\n>  }\n\nWe try to avoid manually computing string sizes like this, since it's\nerror-prone and can be subject to integer overflow attacks (not in this\ncase, but every instance makes auditing harder). You can use xstrfmt()\ninstead.\n\nCoupled with the leak-hack from above, maybe just:\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex db3a88bfe9..579c3a2419 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1476,9 +1476,19 @@ static int subject_prefix_callback(const struct option *opt, const char *arg,\n \n static int rfc_callback(const struct option *opt, const char *arg, int unset)\n {\n+\t/*\n+\t * \"avoid\" leak by holding on to a reference to the memory, since we\n+\t * need the string for the lifetime of the process anyway\n+\t */\n+\tstatic char *prefix;\n+\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n-\treturn subject_prefix_callback(opt, \"RFC PATCH\", unset);\n+\n+\tfree(prefix);\n+\tprefix = xstrfmt(\"RFC %s\", ((struct rev_info *)opt->value)->subject_prefix);\n+\n+\treturn subject_prefix_callback(opt, prefix, unset);\n }\n \n static int numbered_cmdline_opt = 0;\n\nThe rest of the patch (docs and tests) looked good to me.\n"},{"id":"481051","messageId":"CV49EUBH6USG.TQMTR5HU2OGD@taiga","threadId":"60157","inReplyTo":"20230828144215.GA2537587@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2023-08-28T14:49:10Z","receivedAt":"2023-08-28T14:59:19Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"On Mon Aug 28, 2023 at 4:42 PM CEST, Jeff King wrote:\n>  static int rfc_callback(const struct option *opt, const char *arg, int unset)\n>  {\n> +\t/*\n> +\t * \"avoid\" leak by holding on to a reference to the memory, since we\n> +\t * need the string for the lifetime of the process anyway\n> +\t */\n> +\tstatic char *prefix;\n> +\n>  \tBUG_ON_OPT_NEG(unset);\n>  \tBUG_ON_OPT_ARG(arg);\n> -\treturn subject_prefix_callback(opt, \"RFC PATCH\", unset);\n> +\n> +\tfree(prefix);\n> +\tprefix = xstrfmt(\"RFC %s\", ((struct rev_info *)opt->value)->subject_prefix);\n> +\n> +\treturn subject_prefix_callback(opt, prefix, unset);\n>  }\n\nI like this better, thanks!\n"},{"id":"481053","messageId":"xmqqedjnji8t.fsf@gitster.g","threadId":"60157","inReplyTo":"20230828125132.25144-1-sir@cmpwn.com","subject":"Re: [PATCH] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-28T15:31:46Z","receivedAt":"2023-08-28T15:32:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Drew DeVault <sir@cmpwn.com> writes:\n\n> Rather than replacing the configured subject prefix (either through the\n> git config or command line) entirely with \"RFC PATCH\", this change\n> prepends RFC to whatever subject prefix was already in use.\n>\n> This is useful, for example, when a user is working on a repository that\n> has a subject prefix considered to disambiguate patches:\n>\n> \tgit config format.subjectPrefix 'PATCH my-project'\n>\n> Prior to this change, formatting patches with --rfc would lose the\n> 'my-project' information.\n\nOK.  \n\nMy initial reaction was that we should just deprecate \"--rfc\" and\ninstead use \"--subject-prefix\" for whatever multi-token string; that\nway, we do not need to worry about having to add \"--wip\" and other\n\"shorthand\" options ;-).  But the combination of the configuration\nvariable that specifies the tag that is used for everyday operation\nand a command line option that allows you to add (not replace) RFC\nwould be a justifiable behaviour.  It certainly is better than the\ncurrent (original) design of \"--rfc\".  This needs to be advertised\nas a backward incompatible change in the release notes, but I doubt\nthat the fallout would be major.\n\nThe implementation below looks like it is quite out of our style,\nbut I'll read v2 instead.\n\n"},{"id":"481056","messageId":"ae22b71b-73ea-4634-bd2a-4b64082be955@gmail.com","threadId":"60157","inReplyTo":"20230828144215.GA2537587@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-28T16:30:36Z","receivedAt":"2023-08-28T16:31:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 28/08/2023 15:42, Jeff King wrote:\n> On Mon, Aug 28, 2023 at 02:50:34PM +0200, Drew DeVault wrote:\n> \n>> Rather than replacing the configured subject prefix (either through the\n>> git config or command line) entirely with \"RFC PATCH\", this change\n>> prepends RFC to whatever subject prefix was already in use.\n>>\n>> This is useful, for example, when a user is working on a repository that\n>> has a subject prefix considered to disambiguate patches:\n>>\n>> \tgit config format.subjectPrefix 'PATCH my-project'\n>>\n>> Prior to this change, formatting patches with --rfc would lose the\n>> 'my-project' information.\n> \n> This sounds like a good change to me. \n\nI agree it sounds like a good change but if we're going to change it \nthan I think we should ensure\n\n     git format-patch --subject-prefix=foo --rfc\n\nand\n\n     git format-patch --rfc --subject-prefix=foo\n\ngive the same result. That would mean dropping rfc_callback() and using \nOPT_BOOL() instead of OPT_CALLBACK_F(). We could add the \"RFC \" prefix \njust before we add the re-roll suffix.\n\nBest Wishes\n\nPhillip\n\nIt would be backwards-incompatible\n> for anybody expecting:\n> \n>    git format-patch --subject=foo --rfc\n> \n> to override the --subject line, but that seems rather unlikely.\n\n>> Implementation note: this introduces a small memory leak, but freeing it\n>> requires a non-trivial amount of refactoring and some dubious choices\n>> that I was not sure of for a small patch; and it seems like memory leaks\n>> in this context are tolerated anyway from a perusal of the existing\n>> code.\n> \n> We do have a lot of small leaks like this, but we've been trying to\n> clean them up slowly. There's some infrastructure in the test suite for\n> marking scripts as leak-free, but t4014 is not yet there, so this\n> won't cause CI to complain at this point.\n> \n> It is tempting while we are here and thinking about it to put in an easy\n> hack, like storing the allocated string in a static variable.\n> \n>>   static int rfc_callback(const struct option *opt, const char *arg, int unset)\n>>   {\n>> +\tint n;\n>> +\tchar *prefix;\n>> +\tconst char *prev;\n>> +\n>>   \tBUG_ON_OPT_NEG(unset);\n>>   \tBUG_ON_OPT_ARG(arg);\n>> -\treturn subject_prefix_callback(opt, \"RFC PATCH\", unset);\n>> +\n>> +\tprev = ((struct rev_info *)opt->value)->subject_prefix;\n>> +\tassert(prev != NULL);\n>> +\tn = snprintf(NULL, 0, \"RFC %s\", prev);\n>> +\tassert(n > 0);\n>> +\tprefix = xmalloc(n + 1);\n>> +\tn = snprintf(prefix, n + 1, \"RFC %s\", prev);\n>> +\tassert(n > 0);\n>> +\n>> +\treturn subject_prefix_callback(opt, prefix, unset);\n>>   }\n> \n> We try to avoid manually computing string sizes like this, since it's\n> error-prone and can be subject to integer overflow attacks (not in this\n> case, but every instance makes auditing harder). You can use xstrfmt()\n> instead.\n> \n> Coupled with the leak-hack from above, maybe just:\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index db3a88bfe9..579c3a2419 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1476,9 +1476,19 @@ static int subject_prefix_callback(const struct option *opt, const char *arg,\n>   \n>   static int rfc_callback(const struct option *opt, const char *arg, int unset)\n>   {\n> +\t/*\n> +\t * \"avoid\" leak by holding on to a reference to the memory, since we\n> +\t * need the string for the lifetime of the process anyway\n> +\t */\n> +\tstatic char *prefix;\n> +\n>   \tBUG_ON_OPT_NEG(unset);\n>   \tBUG_ON_OPT_ARG(arg);\n> -\treturn subject_prefix_callback(opt, \"RFC PATCH\", unset);\n> +\n> +\tfree(prefix);\n> +\tprefix = xstrfmt(\"RFC %s\", ((struct rev_info *)opt->value)->subject_prefix);\n> +\n> +\treturn subject_prefix_callback(opt, prefix, unset);\n>   }\n>   \n>   static int numbered_cmdline_opt = 0;\n> \n> The rest of the patch (docs and tests) looked good to me.\n"},{"id":"481057","messageId":"20230828174259.GA3007263@coredump.intra.peff.net","threadId":"60157","inReplyTo":"ae22b71b-73ea-4634-bd2a-4b64082be955@gmail.com","subject":"Re: [PATCH] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-28T17:42:59Z","receivedAt":"2023-08-28T17:43:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 28, 2023 at 05:30:36PM +0100, Phillip Wood wrote:\n\n> I agree it sounds like a good change but if we're going to change it than I\n> think we should ensure\n> \n>     git format-patch --subject-prefix=foo --rfc\n> \n> and\n> \n>     git format-patch --rfc --subject-prefix=foo\n> \n> give the same result. That would mean dropping rfc_callback() and using\n> OPT_BOOL() instead of OPT_CALLBACK_F(). We could add the \"RFC \" prefix just\n> before we add the re-roll suffix.\n\nGood catch. That should also make the leak issue easier to solve, too,\nas we'd hold the string (and free it) in the main cmd_format_patch()\nfunction. This is exactly how the \"reroll_count\" feature works\ncurrently.\n\n-Peff\n"},{"id":"481058","messageId":"xmqq1qfnhw93.fsf@gitster.g","threadId":"60157","inReplyTo":"ae22b71b-73ea-4634-bd2a-4b64082be955@gmail.com","subject":"Re: [PATCH] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-28T18:12:08Z","receivedAt":"2023-08-28T18:13:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> I agree it sounds like a good change but if we're going to change it\n> than I think we should ensure\n>\n>     git format-patch --subject-prefix=foo --rfc\n>\n> and\n>\n>     git format-patch --rfc --subject-prefix=foo\n>\n> give the same result.\n\nGood catch.  The implementation with this patch feel philosophically\ndirty, in that the new \"--rfc\" is no longer \"we use a different\nsubject-prefix\" but \"this new option is independent from the\nsubject-prefix; whatever string that other option receives goes\nbefore the title, and our string goes even before that\".  And to\nreflect that independent nature better, it should just grab the\nstring into a separate local variable and combine the two into a\nsingle prefix string after parse_options() returns.\n\nThanks.\n\n\n"}]}