{"thread":{"id":"60158","subject":"[PATCH v2] builtin/log.c: prepend \"RFC\" on --rfc","startedAt":"2023-08-28T14:50:33Z","lastAt":"2023-08-28T23:54:47Z","messageCount":4,"participants":["Drew DeVault","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"481050","messageId":"20230828144940.18245-1-sir@cmpwn.com","threadId":"60158","inReplyTo":null,"subject":"[PATCH v2] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2023-08-28T14:48:49Z","receivedAt":"2023-08-28T14:50:33Z","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---\nv2 incorporates feedback from Jeff King regarding the lifetime of the\nheap-allocated \"RFC %s\" formatted string.\n\n Documentation/git-format-patch.txt |  6 +++---\n builtin/log.c                      | 12 +++++++++++-\n t/t4014-format-patch.sh            |  9 +++++++++\n 3 files changed, 23 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..854216ee9c 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 * The subject_prefix in rev_info is not heap-allocated except in this\n+\t * specific case, so there is no obvious place to free it. Since this\n+\t * value is retained for the lifetime of the process, we just\n+\t * statically allocate storage for it here.\n+\t */\n+\tstatic char *prefix;\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+\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":"481054","messageId":"xmqqa5ubi1nh.fsf@gitster.g","threadId":"60158","inReplyTo":"20230828144940.18245-1-sir@cmpwn.com","subject":"Re: [PATCH v2] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-28T16:15:30Z","receivedAt":"2023-08-28T16:16:32Z","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>\n> Signed-off-by: Drew DeVault <sir@cmpwn.com>\n> ---\n> v2 incorporates feedback from Jeff King regarding the lifetime of the\n> heap-allocated \"RFC %s\" formatted string.\n\nFor future reference, you can signal that by adding\n\n\tHelped-by: Jeff King <peff@peff.net>\n\nbefore [*] your Sign-off.  I'll do that on the receiving end so no\nneed to resend this patch only to correct for this.\n\n    Side note: in other words, in the chronological order of what\n    happened to this particular version of the patch that you expect\n    to hit my tree (you wrote this version with the help by peff,\n    and after proofreading the end product certify that you are\n    contributing to this project under DCO 1.1).\n\n>  Documentation/git-format-patch.txt |  6 +++---\n>  builtin/log.c                      | 12 +++++++++++-\n>  t/t4014-format-patch.sh            |  9 +++++++++\n>  3 files changed, 23 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n> index 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\nLooks good.  \n\nNowhere in the manual page we hint how the separation between\n\"fairly permanent\" customization with format.subjectPrefix and \"per\ninvocation\" customization with this option, which has now become\npossible, is expected to be useful.  Do we expect it is too obvious\nfor our readers and they will figure it out themselves?\n\nIt might be sufficient to add mention of format.subjectPrefix to the\ndescription of \"--subject-prefix=<subject prefix>\" option on the\nsame page, with possible rephrasing of \"useful naming of a patch\nseries\", which leans toward \"per invocation\" customization (i.e.\neven for the same project, different names may be used depending on\na patch series).\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index db3a88bfe9..854216ee9c 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 * The subject_prefix in rev_info is not heap-allocated except in this\n> +\t * specific case, so there is no obvious place to free it. Since this\n> +\t * value is retained for the lifetime of the process, we just\n> +\t * statically allocate storage for it here.\n> +\t */\n> +\tstatic char *prefix;\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> +\treturn subject_prefix_callback(opt, prefix, unset);\n>  }\n\nIn the current code before this patch, the rev.subject_prefix member\nholds one of:\n\n * hardcoded \"PATCH\" in BSS (i.e. fmt_patch_subject_prefix)\n * hardcoded \"RFC PATCH\" in BSS when \"--rfc\" is given\n * value given to command line arg \"--subject-prefix=<prefix>\"\n * value given to format.subjectprefix\n\nand the last one should be freed.  We are removing the second one\nand replacing it with whatever we will do when adding this feature\nso we should be able to make it freeable.  And I do not think it is\nhard to make the third one freeable.\n\nI wonder how far we can go to plug this leak by simply \n\n - making subject_prefix_callback() xstrdup() its arg and free the\n   current value, unless it is the same pointer as\n   fmt_patch_subject_prefix, before assigning a new value, and \n\n - making \"format.subjectprefix\" take the value in a temporary\n   variable from git_config_string(), call\n   subject_prefix_callback(), and free that temporary.\n\nThen unless you use --rfc, --subject-prefix, or the\nformat.subjectprefix variable, we will have no allocation, and if\nyou use any of these, you'll allocate and do the right thing, no?\n\nOf course, as this is \"we compute the value once and keep using it\nwithout reallocating or updating\", coming up with overly complex\nsolution is not worth it, so the \"we only have one allocated string\npointed at by a static, but repeated use of options will not leak\"\nwe see above is probably good enough.\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\nThis is OK but it does not really test the expected use case, does\nit?  Nobody sane would give both --subject-prefix and --rfc from the\ncommand line---they will do --subject-prefix=\"RFC PATCH foobar\"\ninstead, of course.\n\n\tgit -c format.subjectPrefix=\"PATCH foobar\" \\\n\t\t-n -1 --stdout --rfc >patch\n\nmay be much closer to what we expect to see in the real life, I\nwould think.\n\n> +\tgrep ^Subject: patch >actual &&\n\nWhile it is not wrong per se, let's quote the pattern.  Visually,\nany token with a colon at the end attracts whatever comes after it\nand pulls the readers into an illusion that they are part of the\nsame single command line argument.  I.e.\n\n\tgrep -e \"^Subject: \" patch >actual &&\n\nThanks.\n\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"},{"id":"481098","messageId":"ZO0sFWJLX8YaJ2F/@nand.local","threadId":"60158","inReplyTo":"xmqqa5ubi1nh.fsf@gitster.g","subject":"Re: [PATCH v2] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T23:21:57Z","receivedAt":"2023-08-28T23:22:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 28, 2023 at 09:15:30AM -0700, Junio C Hamano wrote:\n> In the current code before this patch, the rev.subject_prefix member\n> holds one of:\n>\n>  * hardcoded \"PATCH\" in BSS (i.e. fmt_patch_subject_prefix)\n>  * hardcoded \"RFC PATCH\" in BSS when \"--rfc\" is given\n>  * value given to command line arg \"--subject-prefix=<prefix>\"\n>  * value given to format.subjectprefix\n>\n> and the last one should be freed.  We are removing the second one\n> and replacing it with whatever we will do when adding this feature\n> so we should be able to make it freeable.  And I do not think it is\n> hard to make the third one freeable.\n>\n> I wonder how far we can go to plug this leak by simply\n>\n>  - making subject_prefix_callback() xstrdup() its arg and free the\n>    current value, unless it is the same pointer as\n>    fmt_patch_subject_prefix, before assigning a new value, and\n>\n>  - making \"format.subjectprefix\" take the value in a temporary\n>    variable from git_config_string(), call\n>    subject_prefix_callback(), and free that temporary.\n\nI am not super familiar with this code, so could easily be missing\nsomething here, but I think that you can do this in a more direct way\nlike so:\n\n--- 8< ---\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 854216ee9c..f1c6c08f75 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -67,6 +67,7 @@ static const char *fmt_patch_subject_prefix = \"PATCH\";\n static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;\n static const char *fmt_pretty;\n static int format_no_prefix;\n+static char *subject_prefix;\n\n static const char * const builtin_log_usage[] = {\n \tN_(\"git log [<options>] [<revision-range>] [[--] <path>...]\"),\n@@ -362,6 +363,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n static int cmd_log_deinit(int ret, struct rev_info *rev)\n {\n \trelease_revisions(rev);\n+\tfree(subject_prefix);\n \treturn ret;\n }\n\n@@ -1463,32 +1465,27 @@ static int keep_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n\n-static int subject_prefix = 0;\n-\n static int subject_prefix_callback(const struct option *opt, const char *arg,\n \t\t\t    int unset)\n {\n+\tstruct rev_info *revs = opt->value;\n \tBUG_ON_OPT_NEG(unset);\n-\tsubject_prefix = 1;\n-\t((struct rev_info *)opt->value)->subject_prefix = arg;\n+\n+\trevs->subject_prefix = arg;\n+\n \treturn 0;\n }\n\n static int rfc_callback(const struct option *opt, const char *arg, int unset)\n {\n-\t/*\n-\t * The subject_prefix in rev_info is not heap-allocated except in this\n-\t * specific case, so there is no obvious place to free it. Since this\n-\t * value is retained for the lifetime of the process, we just\n-\t * statically allocate storage for it here.\n-\t */\n-\tstatic char *prefix;\n+\tstruct rev_info *revs = opt->value;\n+\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n\n-\tfree(prefix);\n-\tprefix = xstrfmt(\"RFC %s\", ((struct rev_info *)opt->value)->subject_prefix);\n-\treturn subject_prefix_callback(opt, prefix, unset);\n+\tfree(subject_prefix);\n+\tsubject_prefix = xstrfmt(\"RFC %s\", revs->subject_prefix);\n+\treturn subject_prefix_callback(opt, subject_prefix, unset);\n }\n\n static int numbered_cmdline_opt = 0;\n--- >8 ---\n\nsince we already have cmd_log_deinit(), which currently just calls\n`release_revisions()` and then propagates a return code.\n\nWe can't foist freeing the subject_prefix into release_revisions, since\n(as noted above), sometimes the value will point into the program's BSS.\n\nBut the only time we care about free()-ing subject_prefix is when we\nxstrdup() a new value into it, and the only place we do that is in\nrfc_callback().\n\nThanks,\nTaylor\n"},{"id":"481102","messageId":"xmqqledug1vd.fsf@gitster.g","threadId":"60158","inReplyTo":"ZO0sFWJLX8YaJ2F/@nand.local","subject":"Re: [PATCH v2] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-28T23:53:42Z","receivedAt":"2023-08-28T23:54:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> I am not super familiar with this code, so could easily be missing\n> something here, but I think that you can do this in a more direct way\n> like so:\n> ...\n>  static int rfc_callback(const struct option *opt, const char *arg, int unset)\n>  {\n> +\tfree(subject_prefix);\n> +\tsubject_prefix = xstrfmt(\"RFC %s\", revs->subject_prefix);\n\nAs Phillip Wood pointed out, this approach no longer works once\n\"--rfc\" is \"no matter what subject-prefix says, we prepend RFC in\nfront\", as the order of command line flags is not forced.  At this\npoint, revs->subject_prefix may be one value (or worse, even not\ninitialized), and then --subject-prefix=<new-prefix> command line\nargument may yet to be parsed.\n\n> +\treturn subject_prefix_callback(opt, subject_prefix, unset);\n>  }\n\nSo, we'd most likely need to treat subject_prefix and rfc as two\nseparate strings while parse_options() is doing its work, and then\nafter that prepend the rfc string, if set, to the final version of\nthe subject_prefix string.\n"}]}