{"thread":{"id":"60167","subject":"[PATCH git v3] builtin/log.c: prepend \"RFC\" on --rfc","startedAt":"2023-08-29T15:35:54Z","lastAt":"2023-08-29T17:26:00Z","messageCount":2,"participants":["Drew DeVault","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"481114","messageId":"20230829153509.27164-1-sir@cmpwn.com","threadId":"60167","inReplyTo":null,"subject":"[PATCH git v3] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2023-08-29T15:34:42Z","receivedAt":"2023-08-29T15:35:54Z","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---\nv3 rewrites the --rfc handler to use OPT_BOOL and track the RFC status\nseparately from the subject prefix as a whole, and updates the tests and\ndocumentation per Junio's feedback.\n\n Documentation/git-format-patch.txt | 18 ++++++++++++------\n builtin/log.c                      | 24 +++++++++++-------------\n t/t4014-format-patch.sh            | 12 +++++++++++-\n 3 files changed, 34 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 373b46fc0d..698c197213 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -217,9 +217,15 @@ populated with placeholder text.\n \n --subject-prefix=<subject prefix>::\n \tInstead of the standard '[PATCH]' prefix in the subject\n-\tline, instead use '[<subject prefix>]'. This\n-\tallows for useful naming of a patch series, and can be\n-\tcombined with the `--numbered` option.\n+\tline, instead use '[<subject prefix>]'. This can be used\n+\tto name a patch series, and can be combined with the\n+\t`--numbered` option.\n++\n+The config option format.subjectPrefix may also be used to to configure\n+a subject prefix to apply to a given repository for all patches. This\n+is often useful on mailing lists which receive patches for several\n+repositories and can be used to disambiguate the patches (with a value\n+of e.g. \"PATCH my-project\").\n \n --filename-max-length=<n>::\n \tInstead of the standard 64 bytes, chomp the generated output\n@@ -229,9 +235,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..16f4343852 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1474,13 +1474,6 @@ 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-\tBUG_ON_OPT_NEG(unset);\n-\tBUG_ON_OPT_ARG(arg);\n-\treturn subject_prefix_callback(opt, \"RFC PATCH\", unset);\n-}\n-\n static int numbered_cmdline_opt = 0;\n \n static int numbered_callback(const struct option *opt, const char *arg,\n@@ -1907,6 +1900,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tstruct strbuf rdiff_title = STRBUF_INIT;\n \tstruct strbuf sprefix = STRBUF_INIT;\n \tint creation_factor = -1;\n+\tint rfc = 0;\n \n \tconst struct option builtin_format_patch_options[] = {\n \t\tOPT_CALLBACK_F('n', \"numbered\", &numbered, NULL,\n@@ -1930,9 +1924,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t    N_(\"mark the series as Nth re-roll\")),\n \t\tOPT_INTEGER(0, \"filename-max-length\", &fmt_patch_name_max,\n \t\t\t    N_(\"max length of output filename\")),\n-\t\tOPT_CALLBACK_F(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\tOPT_BOOL(0, \"rfc\", &rfc, N_(\"use [RFC PATCH] instead of [PATCH]\")),\n \t\tOPT_STRING(0, \"cover-from-description\", &cover_from_description_arg,\n \t\t\t    N_(\"cover-from-description-mode\"),\n \t\t\t    N_(\"generate parts of a cover letter based on a branch's description\")),\n@@ -2048,13 +2040,19 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (cover_from_description_arg)\n \t\tcover_from_description_mode = parse_cover_from_description(cover_from_description_arg);\n \n+\tif (rfc) {\n+\t\tstrbuf_addf(&sprefix, \"RFC %s\", rev.subject_prefix);\n+\t} else {\n+\t\tstrbuf_addstr(&sprefix, rev.subject_prefix);\n+\t}\n+\n \tif (reroll_count) {\n-\t\tstrbuf_addf(&sprefix, \"%s v%s\",\n-\t\t\t    rev.subject_prefix, reroll_count);\n+\t\tstrbuf_addf(&sprefix, \" v%s\", reroll_count);\n \t\trev.reroll_count = reroll_count;\n-\t\trev.subject_prefix = sprefix.buf;\n \t}\n \n+\trev.subject_prefix = sprefix.buf;\n+\n \tfor (i = 0; i < extra_hdr.nr; i++) {\n \t\tstrbuf_addstr(&buf, extra_hdr.items[i].string);\n \t\tstrbuf_addch(&buf, '\\n');\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 3cf2b7a7fb..5d5bc21fd1 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1373,7 +1373,17 @@ test_expect_success '--rfc' '\n \tSubject: [RFC PATCH 1/1] header with . in it\n \tEOF\n \tgit format-patch -n -1 --stdout --rfc >patch &&\n-\tgrep ^Subject: patch >actual &&\n+\tgrep \"^Subject:\" patch >actual &&\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 -c format.subjectPrefix=\"PATCH foobar\" \\\n+\t\tformat-patch -n -1 --stdout --rfc >patch &&\n+\tgrep \"^Subject:\" patch >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.42.0\n\n"},{"id":"481124","messageId":"xmqqbkepep9k.fsf@gitster.g","threadId":"60167","inReplyTo":"20230829153509.27164-1-sir@cmpwn.com","subject":"Re: [PATCH git v3] builtin/log.c: prepend \"RFC\" on --rfc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-29T17:23:35Z","receivedAt":"2023-08-29T17:26:00Z","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> Re: [PATCH git v3] builtin/log.c: prepend \"RFC\" on --rfc\n\nThis is only about format-patch and not other subcommands\nimplemented in that file, so let's retitle it.  E.g.\n\n    Subject: [PATCH v3] format-patch: --rfc honors what --subject-prefix sets\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> v3 rewrites the --rfc handler to use OPT_BOOL and track the RFC status\n> separately from the subject prefix as a whole, and updates the tests and\n> documentation per Junio's feedback.\n\nOPT_BOOL would be a very reasonable simplification at this point.  I\nhadn't considered it back when I commented on the previous rounds,\nas I thought people would want to do something like --rfc=WIP in the\nfuture and using a string that can be given by the end-user was\ninevitable, but we do not need to do that now.\n\n> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n> index 373b46fc0d..698c197213 100644\n> --- a/Documentation/git-format-patch.txt\n> +++ b/Documentation/git-format-patch.txt\n> @@ -217,9 +217,15 @@ populated with placeholder text.\n>  \n>  --subject-prefix=<subject prefix>::\n>  \tInstead of the standard '[PATCH]' prefix in the subject\n> -\tline, instead use '[<subject prefix>]'. This\n> -\tallows for useful naming of a patch series, and can be\n> -\tcombined with the `--numbered` option.\n> +\tline, instead use '[<subject prefix>]'. This can be used\n> +\tto name a patch series, and can be combined with the\n> +\t`--numbered` option.\n> ++\n> +The config option format.subjectPrefix may also be used to to configure\n> +a subject prefix to apply to a given repository for all patches. This\n> +is often useful on mailing lists which receive patches for several\n> +repositories and can be used to disambiguate the patches (with a value\n> +of e.g. \"PATCH my-project\").\n\nThanks for addressing this, too.  One minor thing is that we almost\nnever truncate and say \"config option\" in this document, and say\n\"configuration variable\" instead more often.\n\nI'd update the above to\n\n\tThe configuration variable `format.subjectPrefix` may also...\n\nlocally while queuing, unless there is a strong reason not to (which\nI do not expect).\n\n> @@ -229,9 +235,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\nOK.\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index db3a88bfe9..16f4343852 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1474,13 +1474,6 @@ 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> -\tBUG_ON_OPT_NEG(unset);\n> -\tBUG_ON_OPT_ARG(arg);\n> -\treturn subject_prefix_callback(opt, \"RFC PATCH\", unset);\n> -}\n\nNice to see this go.\n\n> @@ -1907,6 +1900,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>  \tstruct strbuf rdiff_title = STRBUF_INIT;\n>  \tstruct strbuf sprefix = STRBUF_INIT;\n>  \tint creation_factor = -1;\n> +\tint rfc = 0;\n> ...\n> -\t\tOPT_CALLBACK_F(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\tOPT_BOOL(0, \"rfc\", &rfc, N_(\"use [RFC PATCH] instead of [PATCH]\")),\n\nOK, the help text is now a bit of white lie but I think favoring\nbrevity over preciseness is a good choice here.\n\n> @@ -2048,13 +2040,19 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>  \tif (cover_from_description_arg)\n>  \t\tcover_from_description_mode = parse_cover_from_description(cover_from_description_arg);\n>  \n> +\tif (rfc) {\n> +\t\tstrbuf_addf(&sprefix, \"RFC %s\", rev.subject_prefix);\n> +\t} else {\n> +\t\tstrbuf_addstr(&sprefix, rev.subject_prefix);\n> +\t}\n\nExcess braces around single-statement blocks.  I would write this as\n\n\tstrbuf_addf(&sprefix, \"%s%s\", rfc ? \"RFC \" : \"\", rev.subject_prefix);\n\nif I were writing this code.\n\n>  \tif (reroll_count) {\n> -\t\tstrbuf_addf(&sprefix, \"%s v%s\",\n> -\t\t\t    rev.subject_prefix, reroll_count);\n> +\t\tstrbuf_addf(&sprefix, \" v%s\", reroll_count);\n>  \t\trev.reroll_count = reroll_count;\n> -\t\trev.subject_prefix = sprefix.buf;\n>  \t}\n>  \n> +\trev.subject_prefix = sprefix.buf;\n\nOK.  The postimage somehow reads a lot more logical, which is funny.\n\nThe design philosophy of the preimage was \"rev.subject_prefix is the\nking, and anybody who wants to futz with the value would use a\ntemporary to update it\".  In contrast, the new world order is \"we\nbuild the string in sprefix, and at the very end give the result to\nrev.subject.prefix\".  And when viewed from that angle, the above\n\"if\" block that does something extra on sprefix only when\nreroll_count is set makes perfect sense.\n\nBut then from that point of view, I wonder if we should use sprefix\nfrom the very beginning?  That will make your new feature just like\nhow reroll_count futzes with sprefix.\n\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index 3cf2b7a7fb..5d5bc21fd1 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\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 -c format.subjectPrefix=\"PATCH foobar\" \\\n> +\t\tformat-patch -n -1 --stdout --rfc >patch &&\n> +\tgrep \"^Subject:\" patch >actual &&\n>  \ttest_cmp expect actual\n>  '\n\nLooking good.  We would also pass a test that uses the two options\nin the \"wrong\" order, i.e.\n\n\tgit format-patch --rfc --subject-prefix ...\n\nwith the new implementation, which is great.\n\nJust an illustration of the idea about \"sprefix\", which is not even\ncompile tested, looks like the following.  What do you think?\n\nThanks.\n\n builtin/log.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git c/builtin/log.c w/builtin/log.c\nindex 16f4343852..7f22af7ac3 100644\n--- c/builtin/log.c\n+++ w/builtin/log.c\n@@ -1468,9 +1468,13 @@ static int subject_prefix = 0;\n static int subject_prefix_callback(const struct option *opt, const char *arg,\n \t\t\t    int unset)\n {\n+\tstruct strbuf *sprefix;\n+\n \tBUG_ON_OPT_NEG(unset);\n \tsubject_prefix = 1;\n-\t((struct rev_info *)opt->value)->subject_prefix = arg;\n+\tsprefix = (struct strbuf *)opt->value;\n+\tstrbuf_reset(sprefix);\n+\tstrbuf_addstr(sprefix, arg);\n \treturn 0;\n }\n \n@@ -1928,7 +1932,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"cover-from-description\", &cover_from_description_arg,\n \t\t\t    N_(\"cover-from-description-mode\"),\n \t\t\t    N_(\"generate parts of a cover letter based on a branch's description\")),\n-\t\tOPT_CALLBACK_F(0, \"subject-prefix\", &rev, N_(\"prefix\"),\n+\t\tOPT_CALLBACK_F(0, \"subject-prefix\", &sprefix, N_(\"prefix\"),\n \t\t\t    N_(\"use [<prefix>] instead of [PATCH]\"),\n \t\t\t    PARSE_OPT_NONEG, subject_prefix_callback),\n \t\tOPT_CALLBACK_F('o', \"output-directory\", &output_directory,\n@@ -2008,11 +2012,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \trev.max_parents = 1;\n \trev.diffopt.flags.recursive = 1;\n \trev.diffopt.no_free = 1;\n-\trev.subject_prefix = fmt_patch_subject_prefix;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.def = \"HEAD\";\n \ts_r_opt.revarg_opt = REVARG_COMMITTISH;\n \n+\tstrbuf_addstr(&sprefix, fmt_patch_subject_prefix);\n \tif (format_no_prefix)\n \t\tdiff_set_noprefix(&rev.diffopt);\n \n@@ -2040,11 +2044,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (cover_from_description_arg)\n \t\tcover_from_description_mode = parse_cover_from_description(cover_from_description_arg);\n \n-\tif (rfc) {\n-\t\tstrbuf_addf(&sprefix, \"RFC %s\", rev.subject_prefix);\n-\t} else {\n-\t\tstrbuf_addstr(&sprefix, rev.subject_prefix);\n-\t}\n+\tif (rfc)\n+\t\tstrbuf_insertstr(&sprefix, 0, \"RFC \");\n \n \tif (reroll_count) {\n \t\tstrbuf_addf(&sprefix, \" v%s\", reroll_count);\n"}]}