[PATCH 0/4] format-patch: fix an option coexistence bug and add new --resend option

[PATCH 0/4] format-patch: fix an option coexistence bug and add new --resend option

[Dragan Simic]

This series fixes a bug that allows --rfc and -k options to be specified
together when running "git format-patch".  This bug was introduced about
eight months ago, but it has remained undetected, presumably because of
lacking test coverage.  While fixing this bug, also add a test that covers
this mutual exclusion, for future coverage.

This series also adds --resend as the new option for "git format-patch"
that adds "RESEND" as a (sub)suffix to the patch subject prefix, which
eventually produces "[PATCH RESEND]" as the default patch subject prefix.
This subject prefix is commonly used on mailing lists to denote patches
resent after they had attracted no attention for a while.

Dragan Simic (4):
  format-patch docs: avoid use of parentheses to improve readability
  format-patch: fix a bug in option exclusivity and add a test to t4014
  format-patch: new --resend option for adding "RESEND" to patch
    subjects
  t4014: add tests to cover --resend option and its exclusivity

 Documentation/git-format-patch.txt |  9 +++++--
 builtin/log.c                      | 16 +++++++++---
 t/t4014-format-patch.sh            | 41 ++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 5 deletions(-)

[PATCH 1/4] format-patch docs: avoid use of parentheses to improve readability

[Dragan Simic]

In general, using the parentheses disrupts the flow and reduces readability,
so they should be avoided whenever possible.  The improved sentence is a clear
example, in which the adjustment is obvious and simple.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 Documentation/git-format-patch.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 728bb3821c17..a5019ab46926 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -239,8 +239,8 @@ the patches (with a value of e.g. "PATCH my-project").
 	variable, or 64 if unconfigured.
 
 --rfc::
-	Prepends "RFC" to the subject prefix (producing "RFC PATCH" by
-	default). RFC means "Request For Comments"; use this when sending
+	Prepends "RFC" to the subject prefix, producing "RFC PATCH" by
+	default.  RFC means "Request For Comments"; use this when sending
 	an experimental patch for discussion rather than application.
 
 -v <n>::

[PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Dragan Simic]

Fix a bug that allows --rfc and -k options to be specified together when
executing "git format-patch".  This bug was introduced back in the commit
e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix sets"),
about eight months ago, but it has remained undetected so far, presumably
because of no associated test coverage.

Add a new test to the t4014 that covers the mutual exclusivity of the --rfc
and -k command-line options for "git format-patch", for future coverage.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 builtin/log.c           | 5 ++++-
 t/t4014-format-patch.sh | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index c0a8bb95e983..e5a238f1cf2c 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_from_description_arg)
 		cover_from_description_mode = parse_cover_from_description(cover_from_description_arg);
 
-	if (rfc)
+	/* Also mark the subject prefix as modified, for later checks */
+	if (rfc) {
 		strbuf_insertstr(&sprefix, 0, "RFC ");
+		subject_prefix = 1;
+	}
 
 	if (reroll_count) {
 		strbuf_addf(&sprefix, " v%s", reroll_count);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index e37a1411ee24..e22c4ac34e6e 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1397,6 +1397,10 @@ test_expect_success '--rfc is argument order independent' '
 	test_cmp expect actual
 '
 
+test_expect_success '--rfc and -k cannot be used together' '
+	test_must_fail git format-patch -1 --stdout --rfc -k >patch
+'
+
 test_expect_success '--from=ident notices bogus ident' '
 	test_must_fail git format-patch -1 --stdout --from=foo >patch
 '

[PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

Add --resend as the new command-line option for "git format-patch" that adds
"RESEND" as a (sub)suffix to the patch subject prefix, eventually producing
"[PATCH RESEND]" as the default patch subject prefix.

"[PATCH RESEND]" is a patch subject prefix commonly used on mailing lists
for patches resent to a mailing list after they had attracted no attention
for some time, usually for a couple of weeks.  As such, this subject prefix
deserves adding --resend as a new shorthand option to "git format-patch".

Of course, add the description of the new --resend command-line option to
the documentation for "git format-patch".

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 Documentation/git-format-patch.txt |  5 +++++
 builtin/log.c                      | 11 +++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index a5019ab46926..8e63b62620ed 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -243,6 +243,11 @@ the patches (with a value of e.g. "PATCH my-project").
 	default.  RFC means "Request For Comments"; use this when sending
 	an experimental patch for discussion rather than application.
 
+--resend::
+	Appends "RESEND" to the subject prefix, producing "PATCH RESEND"
+	by default.  Use this when sending again a patch that had resulted
+	in attracting no discussion for a while.
+
 -v <n>::
 --reroll-count=<n>::
 	Mark the series as the <n>-th iteration of the topic. The
diff --git a/builtin/log.c b/builtin/log.c
index e5a238f1cf2c..28f31659bcde 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1908,7 +1908,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct strbuf rdiff_title = STRBUF_INIT;
 	struct strbuf sprefix = STRBUF_INIT;
 	int creation_factor = -1;
-	int rfc = 0;
+	int rfc = 0, resend = 0;
 
 	const struct option builtin_format_patch_options[] = {
 		OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
@@ -1933,6 +1933,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
 			    N_("max length of output filename")),
 		OPT_BOOL(0, "rfc", &rfc, N_("use [RFC PATCH] instead of [PATCH]")),
+		OPT_BOOL(0, "resend", &resend, N_("use [PATCH RESEND] instead of [PATCH]")),
 		OPT_STRING(0, "cover-from-description", &cover_from_description_arg,
 			    N_("cover-from-description-mode"),
 			    N_("generate parts of a cover letter based on a branch's description")),
@@ -2055,6 +2056,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		strbuf_insertstr(&sprefix, 0, "RFC ");
 		subject_prefix = 1;
 	}
+	if (resend) {
+		strbuf_addstr(&sprefix, " RESEND");
+		subject_prefix = 1;
+	}
 
 	if (reroll_count) {
 		strbuf_addf(&sprefix, " v%s", reroll_count);
@@ -2111,7 +2116,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (numbered && keep_subject)
 		die(_("options '%s' and '%s' cannot be used together"), "-n", "-k");
 	if (keep_subject && subject_prefix)
-		die(_("options '%s' and '%s' cannot be used together"), "--subject-prefix/--rfc", "-k");
+		die(_("options '%s' and '%s' cannot be used together"), "--subject-prefix/--rfc/--resend", "-k");
+	if (rfc && resend)
+		die(_("options '%s' and '%s' cannot be used together"), "--rfc", "--resend");
 	rev.preserve_subject = keep_subject;
 
 	argc = setup_revisions(argc, argv, &rev, &s_r_opt);

[PATCH 4/4] t4014: add tests to cover --resend option and its exclusivity

[Dragan Simic]

Add a few new tests to the t4014 that cover the --resend command-line option
for "git format-patch", which include the tests for its exclusivity with the
already existing -k and --rfc command-line options.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 t/t4014-format-patch.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index e22c4ac34e6e..bcf7b633e78f 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1401,6 +1401,43 @@ test_expect_success '--rfc and -k cannot be used together' '
 	test_must_fail git format-patch -1 --stdout --rfc -k >patch
 '
 
+test_expect_success '--resend' '
+	cat >expect <<-\EOF &&
+	Subject: [PATCH RESEND 1/1] header with . in it
+	EOF
+	git format-patch -n -1 --stdout --resend >patch &&
+	grep "^Subject:" patch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--resend does not overwrite prefix' '
+	cat >expect <<-\EOF &&
+	Subject: [PATCH RFC RESEND 1/1] header with . in it
+	EOF
+	git -c format.subjectPrefix="PATCH RFC" \
+		format-patch -n -1 --stdout --resend >patch &&
+	grep "^Subject:" patch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--resend is argument order independent' '
+	cat >expect <<-\EOF &&
+	Subject: [PATCH RFC RESEND 1/1] header with . in it
+	EOF
+	git format-patch -n -1 --stdout --resend \
+		--subject-prefix="PATCH RFC" >patch &&
+	grep "^Subject:" patch >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--resend and -k cannot be used together' '
+	test_must_fail git format-patch -1 --stdout --resend -k >patch
+'
+
+test_expect_success '--rfc and --resend cannot be used together' '
+	test_must_fail git format-patch -1 --stdout --rfc --resend >patch
+'
+
 test_expect_success '--from=ident notices bogus ident' '
 	test_must_fail git format-patch -1 --stdout --from=foo >patch
 '

Re: [PATCH 0/4] format-patch: fix an option coexistence bug and add new --resend option

[Eric Sunshine]

On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org> wrote:
> This series fixes a bug that allows --rfc and -k options to be specified
> together when running "git format-patch".  This bug was introduced about
> eight months ago, but it has remained undetected, presumably because of
> lacking test coverage.  While fixing this bug, also add a test that covers
> this mutual exclusion, for future coverage.
>
> This series also adds --resend as the new option for "git format-patch"
> that adds "RESEND" as a (sub)suffix to the patch subject prefix, which
> eventually produces "[PATCH RESEND]" as the default patch subject prefix.
> This subject prefix is commonly used on mailing lists to denote patches
> resent after they had attracted no attention for a while.

I'd recommend splitting this into two series, one which fixes the bug,
and one which introduces the new feature. Otherwise, the bug fix is
likely to be held hostage as reviewers bikeshed over the new feature
and opine about whether such a feature is even desirable[*]. As a
result, the bug fix may take much longer to get applied than if
submitted as a standalone series.

[*] For instance, my knee-jerk reaction is that we don't want to keep
piling on these special-case flags each time someone wants their new
favorite word as a lead-in to "PATCH". In addition to --rfc, and
--resend, the next person might want --rfd or --tbd, etc. More
palatable would be a general-purpose option which lets you specify the
prefix which appears in front of "PATCH", but even that can be argued
as unnecessary since we already have --subject-prefix.

Re: [PATCH 0/4] format-patch: fix an option coexistence bug and add new --resend option

[Dragan Simic]

Hello Eric,

On 2024-04-17 08:02, Eric Sunshine wrote:
> On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> This series fixes a bug that allows --rfc and -k options to be 
>> specified
>> together when running "git format-patch".  This bug was introduced 
>> about
>> eight months ago, but it has remained undetected, presumably because 
>> of
>> lacking test coverage.  While fixing this bug, also add a test that 
>> covers
>> this mutual exclusion, for future coverage.
>> 
>> This series also adds --resend as the new option for "git 
>> format-patch"
>> that adds "RESEND" as a (sub)suffix to the patch subject prefix, which
>> eventually produces "[PATCH RESEND]" as the default patch subject 
>> prefix.
>> This subject prefix is commonly used on mailing lists to denote 
>> patches
>> resent after they had attracted no attention for a while.
> 
> I'd recommend splitting this into two series, one which fixes the bug,
> and one which introduces the new feature. Otherwise, the bug fix is
> likely to be held hostage as reviewers bikeshed over the new feature
> and opine about whether such a feature is even desirable[*]. As a
> result, the bug fix may take much longer to get applied than if
> submitted as a standalone series.

Thanks for the suggestion!  I'll wait a couple of days for more
feedback, and I'll then split the series.

> [*] For instance, my knee-jerk reaction is that we don't want to keep
> piling on these special-case flags each time someone wants their new
> favorite word as a lead-in to "PATCH". In addition to --rfc, and
> --resend, the next person might want --rfd or --tbd, etc. More
> palatable would be a general-purpose option which lets you specify the
> prefix which appears in front of "PATCH", but even that can be argued
> as unnecessary since we already have --subject-prefix.

Makes sense, but in that case accepting the --rfc option, back at the
time, was actually some kind of a mistake, if you agree.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Kristoffer Haugsbakk]

On Wed, Apr 17, 2024, at 05:32, Dragan Simic wrote:
> Add --resend as the new command-line option for "git format-patch" that adds
> "RESEND" as a (sub)suffix to the patch subject prefix, eventually producing
> "[PATCH RESEND]" as the default patch subject prefix.

I think this paragraph is a bit *long*. How about

  “ --resend adds "RESEND" to the subject prefix (producing "PATCH
    RESEND" by default).

(I took this from description of `--rfc`.)

Probably modified to fit in with the other paragraphs.

> Of course, add the description of the new --resend command-line option to
> the documentation for "git format-patch".

This paragraph can be dropped. ;) Adding documentation along with a new
feature doesn’t need to be called out.

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Eric Sunshine]

On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org> wrote:
> format-patch: fix a bug in option exclusivity and add a test to t4014

Reviewers assume that a conscientious patch author will add tests when
appropriate, so stating that you did so is unnecessary. Thus it's safe
to omit "and add a test to t4014" without negatively impacting
comprehension of the subject.

    format-patch: ensure --rfc and -k are mutually exclusive

> Fix a bug that allows --rfc and -k options to be specified together when
> executing "git format-patch".  This bug was introduced back in the commit
> e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix sets"),
> about eight months ago, but it has remained undetected so far, presumably
> because of no associated test coverage.

Everything starting at "...about eight months" through the end of the
paragraph could be easily dropped. Reviewers understand implicitly
that the bug went undiscovered due to lack of test coverage.

> Add a new test to the t4014 that covers the mutual exclusivity of the --rfc
> and -k command-line options for "git format-patch", for future coverage.

Similarly, no need for this paragraph. As a conscientious patch
author, reviewers assume that you added the test, so this paragraph
adds no information. Also, the body of the patch provides this
information clearly without it having to be stated here.

> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
> diff --git a/builtin/log.c b/builtin/log.c
> @@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> -       if (rfc)
> +       /* Also mark the subject prefix as modified, for later checks */
> +       if (rfc) {
>                 strbuf_insertstr(&sprefix, 0, "RFC ");
> +               subject_prefix = 1;
> +       }

I'm not sure that this new comment (/* Also mark... */) adds any value
beyond what the code itself already says. It may actually be confusing
with its current placement. Had you placed it immediately above the
`stubject_prefix = 1` line, it would have been more understandable,
but still probably unnecessary since anyone studying this code is
going to have to understand the purpose of `subject_prefix` anyhow.

At any rate, I doubt that any of these review comments on their own is
worth a reroll.

Re: [PATCH 0/4] format-patch: fix an option coexistence bug and add new --resend option

[Eric Sunshine]

On Wed, Apr 17, 2024 at 2:07 AM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2024-04-17 08:02, Eric Sunshine wrote:
> > [*] For instance, my knee-jerk reaction is that we don't want to keep
> > piling on these special-case flags each time someone wants their new
> > favorite word as a lead-in to "PATCH". In addition to --rfc, and
> > --resend, the next person might want --rfd or --tbd, etc. More
> > palatable would be a general-purpose option which lets you specify the
> > prefix which appears in front of "PATCH", but even that can be argued
> > as unnecessary since we already have --subject-prefix.
>
> Makes sense, but in that case accepting the --rfc option, back at the
> time, was actually some kind of a mistake, if you agree.

Possibly. It does happen that, in retrospect, some changes come to be
viewed as mistakes. On the other hand, if --rfc existed before
--subject-prefix was introduced, then --rfc would just be historic
accretion rather than a mistake. (I didn't check which option came
first.)

At any rate, we probably want to be careful about piling on more
special-cases without considering general-purpose solutions.

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On Wed, Apr 17, 2024 at 05:32:42AM +0200, Dragan Simic wrote:
> Fix a bug that allows --rfc and -k options to be specified together when
> executing "git format-patch".  This bug was introduced back in the commit
> e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix sets"),
> about eight months ago, but it has remained undetected so far, presumably
> because of no associated test coverage.
> 
> Add a new test to the t4014 that covers the mutual exclusivity of the --rfc
> and -k command-line options for "git format-patch", for future coverage.
> 
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  builtin/log.c           | 5 ++++-
>  t/t4014-format-patch.sh | 4 ++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index c0a8bb95e983..e5a238f1cf2c 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (cover_from_description_arg)
>  		cover_from_description_mode = parse_cover_from_description(cover_from_description_arg);
>  
> -	if (rfc)
> +	/* Also mark the subject prefix as modified, for later checks */
> +	if (rfc) {
>  		strbuf_insertstr(&sprefix, 0, "RFC ");
> +		subject_prefix = 1;
> +	}

As an alternative fix, can we drop `subject_prefix` and replace it with
`sprefix.len` instead? It seems to merely be a proxy value for that
anyway, and if we didn't have that variable then the bug would not have
been possible to begin with.

Patrick

>  	if (reroll_count) {
>  		strbuf_addf(&sprefix, " v%s", reroll_count);
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index e37a1411ee24..e22c4ac34e6e 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1397,6 +1397,10 @@ test_expect_success '--rfc is argument order independent' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success '--rfc and -k cannot be used together' '
> +	test_must_fail git format-patch -1 --stdout --rfc -k >patch
> +'
> +
>  test_expect_success '--from=ident notices bogus ident' '
>  	test_must_fail git format-patch -1 --stdout --from=foo >patch
>  '
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Dragan Simic]

On 2024-04-17 08:15, Eric Sunshine wrote:
> On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> format-patch: fix a bug in option exclusivity and add a test to t4014
> 
> Reviewers assume that a conscientious patch author will add tests when
> appropriate, so stating that you did so is unnecessary. Thus it's safe
> to omit "and add a test to t4014" without negatively impacting
> comprehension of the subject.
> 
>     format-patch: ensure --rfc and -k are mutually exclusive

Makes sense, but the previous authors obviously weren't diligent
enough to include such a test, which presumably made the fixed bug
remain undetected for so long, so I wanted to put some emphasis on
the addition of a test.

>> Fix a bug that allows --rfc and -k options to be specified together 
>> when
>> executing "git format-patch".  This bug was introduced back in the 
>> commit
>> e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix 
>> sets"),
>> about eight months ago, but it has remained undetected so far, 
>> presumably
>> because of no associated test coverage.
> 
> Everything starting at "...about eight months" through the end of the
> paragraph could be easily dropped. Reviewers understand implicitly
> that the bug went undiscovered due to lack of test coverage.

I have no problems with dropping that part, but IMHO that's nitpicking.
Also, dropping it would delete some of the context that people might
find useful later.

>> Add a new test to the t4014 that covers the mutual exclusivity of the 
>> --rfc
>> and -k command-line options for "git format-patch", for future 
>> coverage.
> 
> Similarly, no need for this paragraph. As a conscientious patch
> author, reviewers assume that you added the test, so this paragraph
> adds no information. Also, the body of the patch provides this
> information clearly without it having to be stated here.

With all the respect, I think that having that paragraph is actually
good, because explaining it clearly provides good context for the
repository history and people reading it later.

>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> diff --git a/builtin/log.c b/builtin/log.c
>> @@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char 
>> **argv, const char *prefix)
>> -       if (rfc)
>> +       /* Also mark the subject prefix as modified, for later checks 
>> */
>> +       if (rfc) {
>>                 strbuf_insertstr(&sprefix, 0, "RFC ");
>> +               subject_prefix = 1;
>> +       }
> 
> I'm not sure that this new comment (/* Also mark... */) adds any value
> beyond what the code itself already says. It may actually be confusing
> with its current placement. Had you placed it immediately above the
> `stubject_prefix = 1` line, it would have been more understandable,
> but still probably unnecessary since anyone studying this code is
> going to have to understand the purpose of `subject_prefix` anyhow.

Setting such flags should actually be performed in a callback,
but in this case creating a callback isn't warranted, IMHO.  Thus,
that comment tries to explain why a flag is set out of place.
I have no objections against removing this comment, if you find
it doing more harm than good.

I didn't place it immediately above the relevant line because it
also applies to the adjacent block for the --resend option, and I
wanted to reduce the code churn that would result from placing it
immediately before the relevant line, and moving it a couple of
lines above just a couple of patches later.

> At any rate, I doubt that any of these review comments on their own is
> worth a reroll.

Well, I need to split the series anyway, so the v2 is pretty much
inevitable.

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Kristoffer Haugsbakk]

It could be useful to Cc the author of that commit since it’s so
recent. Like an FYI.

On Wed, Apr 17, 2024, at 05:32, Dragan Simic wrote:
> Fix a bug that allows --rfc and -k options to be specified together when
> executing "git format-patch".  This bug was introduced back in the commit
> e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix sets"),
> about eight months ago, but it has remained undetected so far, presumably
> because of no associated test coverage.

I don’t think speculating on why the bug is still there improves the
commit message.

This paragraph could perhaps be rewritten to

  “ Fix a bug from e0d7db7423a (format-patch: --rfc honors what
    --subject-prefix sets, 2023-08-30) that allows --rfc and -k options
    to be specified together when executing "git format-patch".

The extra sentence in the original doesn’t really explain anything more
about the commit. Except the “eight months ago”, but here I’ve used the
“reference” style (not the Linux-style) which contains the date.

> Add a new test to the t4014 that covers the mutual exclusivity of the --rfc
> and -k command-line options for "git format-patch", for future coverage.

I.e. add a regression test. Pretty standard.

>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  builtin/log.c           | 5 ++++-
>  t/t4014-format-patch.sh | 4 ++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index c0a8bb95e983..e5a238f1cf2c 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char
> **argv, const char *prefix)
>  	if (cover_from_description_arg)
>  		cover_from_description_mode =
> parse_cover_from_description(cover_from_description_arg);
>
> -	if (rfc)
> +	/* Also mark the subject prefix as modified, for later checks */

I think the code speaks for itself in this case.

> +	if (rfc) {
>  		strbuf_insertstr(&sprefix, 0, "RFC ");
> +		subject_prefix = 1;
> +	}
>
>  	if (reroll_count) {
>  		strbuf_addf(&sprefix, " v%s", reroll_count);
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index e37a1411ee24..e22c4ac34e6e 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1397,6 +1397,10 @@ test_expect_success '--rfc is argument order
> independent' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success '--rfc and -k cannot be used together' '
> +	test_must_fail git format-patch -1 --stdout --rfc -k >patch

I don’t understand why you redirect to `patch` if you only check the
exit code. (I don’t expect any stdout since it will fail.)

Although it would be nice with a text comparison or grep on the stderr
output to make sure that the command died for the expected reason. But I
haven’t read the associated code.

> +'
> +
>  test_expect_success '--from=ident notices bogus ident' '
>  	test_must_fail git format-patch -1 --stdout --from=foo >patch
>  '

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Eric Sunshine]

On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org> wrote:
> Add --resend as the new command-line option for "git format-patch" that adds
> "RESEND" as a (sub)suffix to the patch subject prefix, eventually producing
> "[PATCH RESEND]" as the default patch subject prefix.
>
> "[PATCH RESEND]" is a patch subject prefix commonly used on mailing lists
> for patches resent to a mailing list after they had attracted no attention
> for some time, usually for a couple of weeks.  As such, this subject prefix
> deserves adding --resend as a new shorthand option to "git format-patch".
>
> Of course, add the description of the new --resend command-line option to
> the documentation for "git format-patch".
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
> diff --git a/builtin/log.c b/builtin/log.c
> @@ -2111,7 +2116,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>         if (keep_subject && subject_prefix)
> -               die(_("options '%s' and '%s' cannot be used together"), "--subject-prefix/--rfc", "-k");
> +               die(_("options '%s' and '%s' cannot be used together"), "--subject-prefix/--rfc/--resend", "-k");

You probably want to be using die_for_incompatible_opt4() from
parse-options.h here.

(And you may want a preparatory patch which fixes the preimage to use
die_for_incompatible_opt3() for --subject-prefix, --rfc, and -k
exclusivity, though that may be overkill.)

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

Hello Kristoffer,

On 2024-04-17 08:14, Kristoffer Haugsbakk wrote:
> On Wed, Apr 17, 2024, at 05:32, Dragan Simic wrote:
>> Add --resend as the new command-line option for "git format-patch" 
>> that adds
>> "RESEND" as a (sub)suffix to the patch subject prefix, eventually 
>> producing
>> "[PATCH RESEND]" as the default patch subject prefix.
> 
> I think this paragraph is a bit *long*. How about
> 
>   “ --resend adds "RESEND" to the subject prefix (producing "PATCH
>     RESEND" by default).
> 
> (I took this from description of `--rfc`.)
> 
> Probably modified to fit in with the other paragraphs.

Thanks for your feedback!

I also wasn't super happy with that paragraph, because I tried to be
as technically accurate as possible, but that unfortunately made the
wording a bit awkward.  Though, I'm not really sure that your proposed
description is actually better, because the parenthesis should in
general be avoided, because they disrupt the flow.  It also doesn't
use imperative mood.

Of course, I'll see to improve that paragraph.

>> Of course, add the description of the new --resend command-line option 
>> to
>> the documentation for "git format-patch".
> 
> This paragraph can be dropped. ;) Adding documentation along with a new
> feature doesn’t need to be called out.

Makes sense.

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Eric Sunshine]

On Wed, Apr 17, 2024 at 2:34 AM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
> On Wed, Apr 17, 2024, at 05:32, Dragan Simic wrote:
> > Fix a bug that allows --rfc and -k options to be specified together when
> > executing "git format-patch".  This bug was introduced back in the commit
> > e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix sets"),
> > about eight months ago, but it has remained undetected so far, presumably
> > because of no associated test coverage.
>
> I don’t think speculating on why the bug is still there improves the
> commit message.
>
> This paragraph could perhaps be rewritten to
>
>   “ Fix a bug from e0d7db7423a (format-patch: --rfc honors what
>     --subject-prefix sets, 2023-08-30) that allows --rfc and -k options
>     to be specified together when executing "git format-patch".
>
> The extra sentence in the original doesn’t really explain anything more
> about the commit. Except the “eight months ago”, but here I’ve used the
> “reference” style (not the Linux-style) which contains the date.
> > @@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char
> > -     if (rfc)
> > +     /* Also mark the subject prefix as modified, for later checks */
>
> I think the code speaks for itself in this case.

Apparently we're thinking along the same lines since we both said
essentially the same things in our reviews.

> > +test_expect_success '--rfc and -k cannot be used together' '
> > +     test_must_fail git format-patch -1 --stdout --rfc -k >patch
>
> I don’t understand why you redirect to `patch` if you only check the
> exit code. (I don’t expect any stdout since it will fail.)

I had the same question but left it unwritten since I noticed that
this new test is modelled after the test immediately following it in
the script, and the existing test also redirects to "patch"
unnecessarily. So, if it's done this way for consistency with existing
tests, I don't mind letting it slide.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Kristoffer Haugsbakk]

On Wed, Apr 17, 2024, at 08:36, Dragan Simic wrote:
> It also doesn't use imperative mood.

The sentence describes what the option does (usage). It doesn’t explain
what the commit message does. In context:

    Teach format-patch about --resend

    --resend adds "RESEND" to the subject prefix (producing "PATCH
    RESEND" by default).

-- 
Kristoffer Haugsbakk

Re: [PATCH 0/4] format-patch: fix an option coexistence bug and add new --resend option

[Dragan Simic]

On 2024-04-17 08:23, Eric Sunshine wrote:
> On Wed, Apr 17, 2024 at 2:07 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-04-17 08:02, Eric Sunshine wrote:
>> > [*] For instance, my knee-jerk reaction is that we don't want to keep
>> > piling on these special-case flags each time someone wants their new
>> > favorite word as a lead-in to "PATCH". In addition to --rfc, and
>> > --resend, the next person might want --rfd or --tbd, etc. More
>> > palatable would be a general-purpose option which lets you specify the
>> > prefix which appears in front of "PATCH", but even that can be argued
>> > as unnecessary since we already have --subject-prefix.
>> 
>> Makes sense, but in that case accepting the --rfc option, back at the
>> time, was actually some kind of a mistake, if you agree.
> 
> Possibly. It does happen that, in retrospect, some changes come to be
> viewed as mistakes. On the other hand, if --rfc existed before
> --subject-prefix was introduced, then --rfc would just be historic
> accretion rather than a mistake. (I didn't check which option came
> first.)
> 
> At any rate, we probably want to be careful about piling on more
> special-cases without considering general-purpose solutions.

Yes, but the usability should also be taken into consideration.
IOW, perhaps typing just --rfc or --resend is rather quick and
usable, instead of having to use a general-purpose solution and
type much more, or instead of having to create an alias.

Re: [PATCH 4/4] t4014: add tests to cover --resend option and its exclusivity

[Eric Sunshine]

On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org> wrote:
> Add a few new tests to the t4014 that cover the --resend command-line option
> for "git format-patch", which include the tests for its exclusivity with the
> already existing -k and --rfc command-line options.

I'd recommend squashing this patch into [3/4] which introduces the
--resend option since it's easier to review the tests when the code
which is being tested is still fresh in one's mind. (For the same
reason, reviewers like to see documentation added in the same patch
which changes the code since it's easier to verify that the
documentation matches the implementation while it's fresh in the
mind.)

> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> @@ -1401,6 +1401,43 @@ test_expect_success '--rfc and -k cannot be used together' '
> +test_expect_success '--resend' '
> +       cat >expect <<-\EOF &&
> +       Subject: [PATCH RESEND 1/1] header with . in it
> +       EOF

In all of the new tests, since it's just a single line body, it could
just as easily be created with `echo`:

    echo "Subject: [PATCH RESEND 1/1] header with . in it" >expect &&

On the other hand, if you're following precedent in this script, then
using a here-doc may be just fine.

At any rate it's somewhat subjective and not worth a reroll.

> +       git format-patch -n -1 --stdout --resend >patch &&
> +       grep "^Subject:" patch >actual &&
> +       test_cmp expect actual
> +'

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Dragan Simic]

On 2024-04-17 08:33, Kristoffer Haugsbakk wrote:
> It could be useful to Cc the author of that commit since it’s so
> recent. Like an FYI.

Good point.  Will do that in the v2.

> On Wed, Apr 17, 2024, at 05:32, Dragan Simic wrote:
>> Fix a bug that allows --rfc and -k options to be specified together 
>> when
>> executing "git format-patch".  This bug was introduced back in the 
>> commit
>> e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix 
>> sets"),
>> about eight months ago, but it has remained undetected so far, 
>> presumably
>> because of no associated test coverage.
> 
> I don’t think speculating on why the bug is still there improves the
> commit message.

Perhaps you're right, but perhaps I'm also right with that speculation. 
:)

> This paragraph could perhaps be rewritten to
> 
>   “ Fix a bug from e0d7db7423a (format-patch: --rfc honors what
>     --subject-prefix sets, 2023-08-30) that allows --rfc and -k options
>     to be specified together when executing "git format-patch".
> 
> The extra sentence in the original doesn’t really explain anything more
> about the commit. Except the “eight months ago”, but here I’ve used the
> “reference” style (not the Linux-style) which contains the date.

I'm fine with that.  Though, I just tried to explain it all in prose,
which may actually be helpful to the people going through the repository
history later.

>> Add a new test to the t4014 that covers the mutual exclusivity of the 
>> --rfc
>> and -k command-line options for "git format-patch", for future 
>> coverage.
> 
> I.e. add a regression test. Pretty standard.

Yes, pretty standard, but again, it obviously wasn't that standard
to the other authors, who missed to include such a test.

>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>>  builtin/log.c           | 5 ++++-
>>  t/t4014-format-patch.sh | 4 ++++
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>> 
>> diff --git a/builtin/log.c b/builtin/log.c
>> index c0a8bb95e983..e5a238f1cf2c 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char
>> **argv, const char *prefix)
>>  	if (cover_from_description_arg)
>>  		cover_from_description_mode =
>> parse_cover_from_description(cover_from_description_arg);
>> 
>> -	if (rfc)
>> +	/* Also mark the subject prefix as modified, for later checks */
> 
> I think the code speaks for itself in this case.

Alright, two votes so far, so this comments gets deleted in the v2. :)
I'm perfectly fine with that.

>> +	if (rfc) {
>>  		strbuf_insertstr(&sprefix, 0, "RFC ");
>> +		subject_prefix = 1;
>> +	}
>> 
>>  	if (reroll_count) {
>>  		strbuf_addf(&sprefix, " v%s", reroll_count);
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> index e37a1411ee24..e22c4ac34e6e 100755
>> --- a/t/t4014-format-patch.sh
>> +++ b/t/t4014-format-patch.sh
>> @@ -1397,6 +1397,10 @@ test_expect_success '--rfc is argument order
>> independent' '
>>  	test_cmp expect actual
>>  '
>> 
>> +test_expect_success '--rfc and -k cannot be used together' '
>> +	test_must_fail git format-patch -1 --stdout --rfc -k >patch
> 
> I don’t understand why you redirect to `patch` if you only check the
> exit code. (I don’t expect any stdout since it will fail.)

You're right, but who knows what might actually happen in the
future, i.e. while someone in the future makes some changes to
the code and runs this test?  It's better to stay on the safe
side and prevent some output from appearing somewhere.

> Although it would be nice with a text comparison or grep on the stderr
> output to make sure that the command died for the expected reason. But 
> I
> haven’t read the associated code.

Yes, it would be nice, and the same thoughts actually already
crossed my mind while working on this patch, but there are already
more similar tests that don't validate such stderr outputs.  Thus,
perhaps it would be better to improve such tests, including this one,
in a separate follow-up series.

>> +'
>> +
>>  test_expect_success '--from=ident notices bogus ident' '
>>  	test_must_fail git format-patch -1 --stdout --from=foo >patch
>>  '

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Dragan Simic]

Hello Patrick,

On 2024-04-17 08:27, Patrick Steinhardt wrote:
> On Wed, Apr 17, 2024 at 05:32:42AM +0200, Dragan Simic wrote:
>> Fix a bug that allows --rfc and -k options to be specified together 
>> when
>> executing "git format-patch".  This bug was introduced back in the 
>> commit
>> e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix 
>> sets"),
>> about eight months ago, but it has remained undetected so far, 
>> presumably
>> because of no associated test coverage.
>> 
>> Add a new test to the t4014 that covers the mutual exclusivity of the 
>> --rfc
>> and -k command-line options for "git format-patch", for future 
>> coverage.
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>>  builtin/log.c           | 5 ++++-
>>  t/t4014-format-patch.sh | 4 ++++
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>> 
>> diff --git a/builtin/log.c b/builtin/log.c
>> index c0a8bb95e983..e5a238f1cf2c 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char 
>> **argv, const char *prefix)
>>  	if (cover_from_description_arg)
>>  		cover_from_description_mode = 
>> parse_cover_from_description(cover_from_description_arg);
>> 
>> -	if (rfc)
>> +	/* Also mark the subject prefix as modified, for later checks */
>> +	if (rfc) {
>>  		strbuf_insertstr(&sprefix, 0, "RFC ");
>> +		subject_prefix = 1;
>> +	}
> 
> As an alternative fix, can we drop `subject_prefix` and replace it with
> `sprefix.len` instead? It seems to merely be a proxy value for that
> anyway, and if we didn't have that variable then the bug would not have
> been possible to begin with.

Thanks for your feedback!

I'll think about it, and I'll come back a bit later with an update.

>>  	if (reroll_count) {
>>  		strbuf_addf(&sprefix, " v%s", reroll_count);
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> index e37a1411ee24..e22c4ac34e6e 100755
>> --- a/t/t4014-format-patch.sh
>> +++ b/t/t4014-format-patch.sh
>> @@ -1397,6 +1397,10 @@ test_expect_success '--rfc is argument order 
>> independent' '
>>  	test_cmp expect actual
>>  '
>> 
>> +test_expect_success '--rfc and -k cannot be used together' '
>> +	test_must_fail git format-patch -1 --stdout --rfc -k >patch
>> +'
>> +
>>  test_expect_success '--from=ident notices bogus ident' '
>>  	test_must_fail git format-patch -1 --stdout --from=foo >patch
>>  '
>> 

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

On 2024-04-17 08:35, Eric Sunshine wrote:
> On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> Add --resend as the new command-line option for "git format-patch" 
>> that adds
>> "RESEND" as a (sub)suffix to the patch subject prefix, eventually 
>> producing
>> "[PATCH RESEND]" as the default patch subject prefix.
>> 
>> "[PATCH RESEND]" is a patch subject prefix commonly used on mailing 
>> lists
>> for patches resent to a mailing list after they had attracted no 
>> attention
>> for some time, usually for a couple of weeks.  As such, this subject 
>> prefix
>> deserves adding --resend as a new shorthand option to "git 
>> format-patch".
>> 
>> Of course, add the description of the new --resend command-line option 
>> to
>> the documentation for "git format-patch".
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> diff --git a/builtin/log.c b/builtin/log.c
>> @@ -2111,7 +2116,9 @@ int cmd_format_patch(int argc, const char 
>> **argv, const char *prefix)
>>         if (keep_subject && subject_prefix)
>> -               die(_("options '%s' and '%s' cannot be used 
>> together"), "--subject-prefix/--rfc", "-k");
>> +               die(_("options '%s' and '%s' cannot be used 
>> together"), "--subject-prefix/--rfc/--resend", "-k");
> 
> You probably want to be using die_for_incompatible_opt4() from
> parse-options.h here.

Thanks for the suggestion.  Frankly, I haven't researched the
available options, assuming that the current code uses the right
option.  Of course, I'll have a detailed look into it.

> (And you may want a preparatory patch which fixes the preimage to use
> die_for_incompatible_opt3() for --subject-prefix, --rfc, and -k
> exclusivity, though that may be overkill.)

I'm not really sure what to do.  Maybe the other reviewers would
prefer an orthogonal approach instead?  Maybe that would be better
for bisecting later, if need arises for that?

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Dragan Simic]

On 2024-04-17 08:40, Eric Sunshine wrote:
> On Wed, Apr 17, 2024 at 2:34 AM Kristoffer Haugsbakk
> <code@khaugsbakk.name> wrote:
>> On Wed, Apr 17, 2024, at 05:32, Dragan Simic wrote:
>> > Fix a bug that allows --rfc and -k options to be specified together when
>> > executing "git format-patch".  This bug was introduced back in the commit
>> > e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix sets"),
>> > about eight months ago, but it has remained undetected so far, presumably
>> > because of no associated test coverage.
>> 
>> I don’t think speculating on why the bug is still there improves the
>> commit message.
>> 
>> This paragraph could perhaps be rewritten to
>> 
>>   “ Fix a bug from e0d7db7423a (format-patch: --rfc honors what
>>     --subject-prefix sets, 2023-08-30) that allows --rfc and -k 
>> options
>>     to be specified together when executing "git format-patch".
>> 
>> The extra sentence in the original doesn’t really explain anything 
>> more
>> about the commit. Except the “eight months ago”, but here I’ve used 
>> the
>> “reference” style (not the Linux-style) which contains the date.
>> > @@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char
>> > -     if (rfc)
>> > +     /* Also mark the subject prefix as modified, for later checks */
>> 
>> I think the code speaks for itself in this case.
> 
> Apparently we're thinking along the same lines since we both said
> essentially the same things in our reviews.

Two votes, so the comments goes away. :)

>> > +test_expect_success '--rfc and -k cannot be used together' '
>> > +     test_must_fail git format-patch -1 --stdout --rfc -k >patch
>> 
>> I don’t understand why you redirect to `patch` if you only check the
>> exit code. (I don’t expect any stdout since it will fail.)
> 
> I had the same question but left it unwritten since I noticed that
> this new test is modelled after the test immediately following it in
> the script, and the existing test also redirects to "patch"
> unnecessarily. So, if it's done this way for consistency with existing
> tests, I don't mind letting it slide.

Yes, I also wasn't super happy with this new test, as I already noted
in one of my replies, but improving this and the other similar tests
is most probably something best left for a follow-up series.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

On 2024-04-17 08:43, Kristoffer Haugsbakk wrote:
> On Wed, Apr 17, 2024, at 08:36, Dragan Simic wrote:
>> It also doesn't use imperative mood.
> 
> The sentence describes what the option does (usage). It doesn’t explain
> what the commit message does. In context:
> 
>     Teach format-patch about --resend
> 
>     --resend adds "RESEND" to the subject prefix (producing "PATCH
>     RESEND" by default).

Frankly, I don't like the "teach abc xyz" wording very much.  It isn't
some intelligent being to be taught something new. :)  That's just my
personal preference, of course.

Furthermore, starting a sentence with "--resend" isn't very good.  
Please
note that you're still using parenthesis, for which I already explained
why they should be avoided.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Eric Sunshine]

On Wed, Apr 17, 2024 at 3:05 AM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2024-04-17 08:35, Eric Sunshine wrote:
> > On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org>
> > wrote:
> >> -               die(_("options '%s' and '%s' cannot be used
> >> together"), "--subject-prefix/--rfc", "-k");
> >> +               die(_("options '%s' and '%s' cannot be used
> >> together"), "--subject-prefix/--rfc/--resend", "-k");
> >
> > You probably want to be using die_for_incompatible_opt4() from
> > parse-options.h here.
>
> Thanks for the suggestion.  Frankly, I haven't researched the
> available options, assuming that the current code uses the right
> option.  Of course, I'll have a detailed look into it.
>
> > (And you may want a preparatory patch which fixes the preimage to use
> > die_for_incompatible_opt3() for --subject-prefix, --rfc, and -k
> > exclusivity, though that may be overkill.)
>
> I'm not really sure what to do.  Maybe the other reviewers would
> prefer an orthogonal approach instead?  Maybe that would be better
> for bisecting later, if need arises for that?

The comment about using die_for_incompatible_opt4() in this patch is
the meaningful one.

You are very welcome to ignore the parenthesized comment about a
preparatory patch. There is probably very little value in such a patch
to fix the preimage to use die_for_incompatible_opt3(), only to then
apply this patch which updates it to use die_for_incompatible_opt4().
That would just be busy-work for you and for reviewers. I mentioned it
only because I noticed that the preimage was doing it wrong (not using
die_for_incompatible_opt3()), which presumably misled you into
continuing that mistake.

Re: [PATCH 4/4] t4014: add tests to cover --resend option and its exclusivity

[Dragan Simic]

On 2024-04-17 08:48, Eric Sunshine wrote:
> On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> Add a few new tests to the t4014 that cover the --resend command-line 
>> option
>> for "git format-patch", which include the tests for its exclusivity 
>> with the
>> already existing -k and --rfc command-line options.
> 
> I'd recommend squashing this patch into [3/4] which introduces the
> --resend option since it's easier to review the tests when the code
> which is being tested is still fresh in one's mind. (For the same
> reason, reviewers like to see documentation added in the same patch
> which changes the code since it's easier to verify that the
> documentation matches the implementation while it's fresh in the
> mind.)

I'm fine with that.  Squashing these two patches together might also
be good for bisecting later, if need arises.

>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> @@ -1401,6 +1401,43 @@ test_expect_success '--rfc and -k cannot be 
>> used together' '
>> +test_expect_success '--resend' '
>> +       cat >expect <<-\EOF &&
>> +       Subject: [PATCH RESEND 1/1] header with . in it
>> +       EOF
> 
> In all of the new tests, since it's just a single line body, it could
> just as easily be created with `echo`:
> 
>     echo "Subject: [PATCH RESEND 1/1] header with . in it" >expect &&
> 
> On the other hand, if you're following precedent in this script, then
> using a here-doc may be just fine.

I agree that using "echo ..." would be nicer.  Though, I just wanted
to follow the already existing tests for consistency, which may actually
outweigh a nicer approach.

> At any rate it's somewhat subjective and not worth a reroll.
> 
>> +       git format-patch -n -1 --stdout --resend >patch &&
>> +       grep "^Subject:" patch >actual &&
>> +       test_cmp expect actual
>> +'

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

On 2024-04-17 09:17, Eric Sunshine wrote:
> On Wed, Apr 17, 2024 at 3:05 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-04-17 08:35, Eric Sunshine wrote:
>> > On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org>
>> > wrote:
>> >> -               die(_("options '%s' and '%s' cannot be used
>> >> together"), "--subject-prefix/--rfc", "-k");
>> >> +               die(_("options '%s' and '%s' cannot be used
>> >> together"), "--subject-prefix/--rfc/--resend", "-k");
>> >
>> > You probably want to be using die_for_incompatible_opt4() from
>> > parse-options.h here.
>> 
>> Thanks for the suggestion.  Frankly, I haven't researched the
>> available options, assuming that the current code uses the right
>> option.  Of course, I'll have a detailed look into it.
>> 
>> > (And you may want a preparatory patch which fixes the preimage to use
>> > die_for_incompatible_opt3() for --subject-prefix, --rfc, and -k
>> > exclusivity, though that may be overkill.)
>> 
>> I'm not really sure what to do.  Maybe the other reviewers would
>> prefer an orthogonal approach instead?  Maybe that would be better
>> for bisecting later, if need arises for that?
> 
> The comment about using die_for_incompatible_opt4() in this patch is
> the meaningful one.
> 
> You are very welcome to ignore the parenthesized comment about a
> preparatory patch. There is probably very little value in such a patch
> to fix the preimage to use die_for_incompatible_opt3(), only to then
> apply this patch which updates it to use die_for_incompatible_opt4().
> That would just be busy-work for you and for reviewers. I mentioned it
> only because I noticed that the preimage was doing it wrong (not using
> die_for_incompatible_opt3()), which presumably misled you into
> continuing that mistake.

Ah, makes sense, thanks for the clarification! :)

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Phillip Wood]

Hi Dragan

On 17/04/2024 04:32, Dragan Simic wrote:
> Add --resend as the new command-line option for "git format-patch" that adds
> "RESEND" as a (sub)suffix to the patch subject prefix, eventually producing
> "[PATCH RESEND]" as the default patch subject prefix.
> 
> "[PATCH RESEND]" is a patch subject prefix commonly used on mailing lists
> for patches resent to a mailing list after they had attracted no attention
> for some time, usually for a couple of weeks.  As such, this subject prefix
> deserves adding --resend as a new shorthand option to "git format-patch".

Playing devil's advocate for a minute, is this really common enough to 
justify a new option when the user can use "--subject-prefix='PATCH 
RESEND'" instead?

Best Wishes

Phillip

> Of course, add the description of the new --resend command-line option to
> the documentation for "git format-patch".
> 
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>   Documentation/git-format-patch.txt |  5 +++++
>   builtin/log.c                      | 11 +++++++++--
>   2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index a5019ab46926..8e63b62620ed 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -243,6 +243,11 @@ the patches (with a value of e.g. "PATCH my-project").
>   	default.  RFC means "Request For Comments"; use this when sending
>   	an experimental patch for discussion rather than application.
>   
> +--resend::
> +	Appends "RESEND" to the subject prefix, producing "PATCH RESEND"
> +	by default.  Use this when sending again a patch that had resulted
> +	in attracting no discussion for a while.
> +
>   -v <n>::
>   --reroll-count=<n>::
>   	Mark the series as the <n>-th iteration of the topic. The
> diff --git a/builtin/log.c b/builtin/log.c
> index e5a238f1cf2c..28f31659bcde 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1908,7 +1908,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>   	struct strbuf rdiff_title = STRBUF_INIT;
>   	struct strbuf sprefix = STRBUF_INIT;
>   	int creation_factor = -1;
> -	int rfc = 0;
> +	int rfc = 0, resend = 0;
>   
>   	const struct option builtin_format_patch_options[] = {
>   		OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
> @@ -1933,6 +1933,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>   		OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
>   			    N_("max length of output filename")),
>   		OPT_BOOL(0, "rfc", &rfc, N_("use [RFC PATCH] instead of [PATCH]")),
> +		OPT_BOOL(0, "resend", &resend, N_("use [PATCH RESEND] instead of [PATCH]")),
>   		OPT_STRING(0, "cover-from-description", &cover_from_description_arg,
>   			    N_("cover-from-description-mode"),
>   			    N_("generate parts of a cover letter based on a branch's description")),
> @@ -2055,6 +2056,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>   		strbuf_insertstr(&sprefix, 0, "RFC ");
>   		subject_prefix = 1;
>   	}
> +	if (resend) {
> +		strbuf_addstr(&sprefix, " RESEND");
> +		subject_prefix = 1;
> +	}
>   
>   	if (reroll_count) {
>   		strbuf_addf(&sprefix, " v%s", reroll_count);
> @@ -2111,7 +2116,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>   	if (numbered && keep_subject)
>   		die(_("options '%s' and '%s' cannot be used together"), "-n", "-k");
>   	if (keep_subject && subject_prefix)
> -		die(_("options '%s' and '%s' cannot be used together"), "--subject-prefix/--rfc", "-k");
> +		die(_("options '%s' and '%s' cannot be used together"), "--subject-prefix/--rfc/--resend", "-k");
> +	if (rfc && resend)
> +		die(_("options '%s' and '%s' cannot be used together"), "--rfc", "--resend");
>   	rev.preserve_subject = keep_subject;
>   
>   	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
> 

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

Hello Phillip,

On 2024-04-17 12:02, Phillip Wood wrote:
> On 17/04/2024 04:32, Dragan Simic wrote:
>> Add --resend as the new command-line option for "git format-patch" 
>> that adds
>> "RESEND" as a (sub)suffix to the patch subject prefix, eventually 
>> producing
>> "[PATCH RESEND]" as the default patch subject prefix.
>> 
>> "[PATCH RESEND]" is a patch subject prefix commonly used on mailing 
>> lists
>> for patches resent to a mailing list after they had attracted no 
>> attention
>> for some time, usually for a couple of weeks.  As such, this subject 
>> prefix
>> deserves adding --resend as a new shorthand option to "git 
>> format-patch".
> 
> Playing devil's advocate for a minute, is this really common enough to
> justify a new option when the user can use "--subject-prefix='PATCH
> RESEND'" instead?

Based on my experience, "[PATCH RESEND]" is roughly as commonly
used as "[PATCH RFC]".  In other words, it obviously isn't used
as much as the good, old plain "[PATCH]", but it is used.

We should also take the overall usability into account, if you
agree.  Just like with "--rfc", typing "--resend" is much easier
and quicker than typing "--subject-prefix='PATCH RESEND'", which
is a lot.  Defining an alias can help, of course, but that isn't
always a convenient solution.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Kristoffer Haugsbakk]

On Wed, Apr 17, 2024, at 12:52, Dragan Simic wrote:
> Hello Phillip,
>
> On 2024-04-17 12:02, Phillip Wood wrote:
>> On 17/04/2024 04:32, Dragan Simic wrote:
>>> Add --resend as the new command-line option for "git format-patch"
>>> that adds
>>> "RESEND" as a (sub)suffix to the patch subject prefix, eventually
>>> producing
>>> "[PATCH RESEND]" as the default patch subject prefix.
>>>
>>> "[PATCH RESEND]" is a patch subject prefix commonly used on mailing
>>> lists
>>> for patches resent to a mailing list after they had attracted no
>>> attention
>>> for some time, usually for a couple of weeks.  As such, this subject
>>> prefix
>>> deserves adding --resend as a new shorthand option to "git
>>> format-patch".
>>
>> Playing devil's advocate for a minute, is this really common enough to
>> justify a new option when the user can use "--subject-prefix='PATCH
>> RESEND'" instead?
>
> Based on my experience, "[PATCH RESEND]" is roughly as commonly
> used as "[PATCH RFC]".  In other words, it obviously isn't used
> as much as the good, old plain "[PATCH]", but it is used.

The format-patch generated string is `RFC PATCH`.

The number of emails with `PATCH RESEND` for this list:[1]

```
$ git log --oneline --fixed-strings --grep='[PATCH RESEND' | wc -l
28
```

For RFC:

```
$ git log --oneline --fixed-strings --grep='[RFC PATCH' | wc -l
1181
```

† 1: According to http://lore.kernel.org/git/1

-- 
Kristoffer Haugsbakk

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

On 2024-04-17 13:31, Kristoffer Haugsbakk wrote:
> On Wed, Apr 17, 2024, at 12:52, Dragan Simic wrote:
>> On 2024-04-17 12:02, Phillip Wood wrote:
>>> On 17/04/2024 04:32, Dragan Simic wrote:
>>>> Add --resend as the new command-line option for "git format-patch"
>>>> that adds
>>>> "RESEND" as a (sub)suffix to the patch subject prefix, eventually
>>>> producing
>>>> "[PATCH RESEND]" as the default patch subject prefix.
>>>> 
>>>> "[PATCH RESEND]" is a patch subject prefix commonly used on mailing
>>>> lists
>>>> for patches resent to a mailing list after they had attracted no
>>>> attention
>>>> for some time, usually for a couple of weeks.  As such, this subject
>>>> prefix
>>>> deserves adding --resend as a new shorthand option to "git
>>>> format-patch".
>>> 
>>> Playing devil's advocate for a minute, is this really common enough 
>>> to
>>> justify a new option when the user can use "--subject-prefix='PATCH
>>> RESEND'" instead?
>> 
>> Based on my experience, "[PATCH RESEND]" is roughly as commonly
>> used as "[PATCH RFC]".  In other words, it obviously isn't used
>> as much as the good, old plain "[PATCH]", but it is used.
> 
> The format-patch generated string is `RFC PATCH`.

True.  It's just that I more often see "PATCH RFC", for some reason.
Please note that I'm also taking other mailing lists into account.

> The number of emails with `PATCH RESEND` for this list:[1]
> 
> $ git log --oneline --fixed-strings --grep='[PATCH RESEND' | wc -l
> 28
> 
> For RFC:
> 
> $ git log --oneline --fixed-strings --grep='[RFC PATCH' | wc -l
> 1181
> 
> † 1: According to http://lore.kernel.org/git/1

I wonder what does it say for "RESEND" only?

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Kristoffer Haugsbakk]

On Wed, Apr 17, 2024, at 09:11, Dragan Simic wrote:
>> I had the same question but left it unwritten since I noticed that
>> this new test is modelled after the test immediately following it in
>> the script, and the existing test also redirects to "patch"
>> unnecessarily. So, if it's done this way for consistency with existing
>> tests, I don't mind letting it slide.
>
> Yes, I also wasn't super happy with this new test, as I already noted
> in one of my replies, but improving this and the other similar tests
> is most probably something best left for a follow-up series.

I don’t see the point in writing the test in mimic-neighbors way only to
improve it shortly after.

If the test can be written in a better way then the other tests can be
improved later. Or now. I think I’ve seen other discussions were a less
good pattern wasn’t accepted in new tests even though they were used in
existing ones. The reviewer then pointed out that the other tests should
be updated later.

That’s just my opinion and recollection.

-- 
Kristoffer Haugsbakk

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Kristoffer Haugsbakk]

On Wed, Apr 17, 2024, at 13:34, Dragan Simic wrote:
>> $ git log --oneline --fixed-strings --grep='[RFC PATCH' | wc -l
>> 1181
>>
>> † 1: According to http://lore.kernel.org/git/1
>
> I wonder what does it say for "RESEND" only?

```
$ git log --oneline --fixed-strings --grep='[RESEND' | wc -l
27
$ git log --oneline --fixed-strings --grep='RESEND' | wc -l
57
```

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

On 2024-04-17 13:34, Dragan Simic wrote:
> On 2024-04-17 13:31, Kristoffer Haugsbakk wrote:
>> On Wed, Apr 17, 2024, at 12:52, Dragan Simic wrote:
>>> On 2024-04-17 12:02, Phillip Wood wrote:
>>>> On 17/04/2024 04:32, Dragan Simic wrote:
>>>>> Add --resend as the new command-line option for "git format-patch"
>>>>> that adds
>>>>> "RESEND" as a (sub)suffix to the patch subject prefix, eventually
>>>>> producing
>>>>> "[PATCH RESEND]" as the default patch subject prefix.
>>>>> 
>>>>> "[PATCH RESEND]" is a patch subject prefix commonly used on mailing
>>>>> lists
>>>>> for patches resent to a mailing list after they had attracted no
>>>>> attention
>>>>> for some time, usually for a couple of weeks.  As such, this 
>>>>> subject
>>>>> prefix
>>>>> deserves adding --resend as a new shorthand option to "git
>>>>> format-patch".
>>>> 
>>>> Playing devil's advocate for a minute, is this really common enough 
>>>> to
>>>> justify a new option when the user can use "--subject-prefix='PATCH
>>>> RESEND'" instead?
>>> 
>>> Based on my experience, "[PATCH RESEND]" is roughly as commonly
>>> used as "[PATCH RFC]".  In other words, it obviously isn't used
>>> as much as the good, old plain "[PATCH]", but it is used.
>> 
>> The format-patch generated string is `RFC PATCH`.
> 
> True.  It's just that I more often see "PATCH RFC", for some reason.
> Please note that I'm also taking other mailing lists into account.
> 
>> The number of emails with `PATCH RESEND` for this list:[1]
>> 
>> $ git log --oneline --fixed-strings --grep='[PATCH RESEND' | wc -l
>> 28
>> 
>> For RFC:
>> 
>> $ git log --oneline --fixed-strings --grep='[RFC PATCH' | wc -l
>> 1181
>> 
>> † 1: According to http://lore.kernel.org/git/1
> 
> I wonder what does it say for "RESEND" only?

Here are some numbers pulled from https://lore.kernel.org/linux-kernel/:

- "RFC": ~400,000
- "PATCH RFC": ~50,000
- "RFC PATCH": ~200,000
- "RESEND": ~200,000
- "PATCH RESEND": ~30,000
- "RESEND PATCH": ~30,000

Though, I'm not sure how accurate those numbers are.  Even a cursory
look at the produced search results shows inaccuracy of the search
matches.  There's probably some "fuzzy logic" at play there.

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Dragan Simic]

On 2024-04-17 13:38, Kristoffer Haugsbakk wrote:
> On Wed, Apr 17, 2024, at 09:11, Dragan Simic wrote:
>>> I had the same question but left it unwritten since I noticed that
>>> this new test is modelled after the test immediately following it in
>>> the script, and the existing test also redirects to "patch"
>>> unnecessarily. So, if it's done this way for consistency with 
>>> existing
>>> tests, I don't mind letting it slide.
>> 
>> Yes, I also wasn't super happy with this new test, as I already noted
>> in one of my replies, but improving this and the other similar tests
>> is most probably something best left for a follow-up series.
> 
> I don’t see the point in writing the test in mimic-neighbors way only 
> to
> improve it shortly after.

Well, the logic is quite simple:  let me get this patch accepted,
and we'll deal with the improvements later.  Though, don't get me
wrong, I'd always prefer to see things done the right way, but the
time, just like the other resources, is limited.

> If the test can be written in a better way then the other tests can be
> improved later. Or now. I think I’ve seen other discussions were a less
> good pattern wasn’t accepted in new tests even though they were used in
> existing ones. The reviewer then pointed out that the other tests 
> should
> be updated later.
> 
> That’s just my opinion and recollection.

I see, but this makes me wonder how often the other tests actually
get improved later?

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Dragan Simic]

On 2024-04-17 08:33, Kristoffer Haugsbakk wrote:
> On Wed, Apr 17, 2024, at 05:32, Dragan Simic wrote:
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> index e37a1411ee24..e22c4ac34e6e 100755
>> --- a/t/t4014-format-patch.sh
>> +++ b/t/t4014-format-patch.sh
>> @@ -1397,6 +1397,10 @@ test_expect_success '--rfc is argument order
>> independent' '
>>  	test_cmp expect actual
>>  '
>> 
>> +test_expect_success '--rfc and -k cannot be used together' '
>> +	test_must_fail git format-patch -1 --stdout --rfc -k >patch
> 
> I don’t understand why you redirect to `patch` if you only check the
> exit code. (I don’t expect any stdout since it will fail.)
> 
> Although it would be nice with a text comparison or grep on the stderr
> output to make sure that the command died for the expected reason. But 
> I
> haven’t read the associated code.

Actually, if you agree, we should check both the stdout
and stderr -- the former for emptiness, and the latter for
the expected error message.

>> +'
>> +
>>  test_expect_success '--from=ident notices bogus ident' '
>>  	test_must_fail git format-patch -1 --stdout --from=foo >patch
>>  '

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Playing devil's advocate for a minute, is this really common enough to
> justify a new option when the user can use "--subject-prefix='PATCH
> RESEND'" instead?

The same applies to "--rfc", but the justification goes like this.

 * When you are working on a single subsystem in a larger project,
   your patches would want to carry the subsystem name.  You'd use
   "--subject-prefix='PATCH frotz'" (and more likely it comes from
   format.subjectPrefix in a working repository dedicated to work on
   the frotz subsystem) for that.

 * In the context of working on that subsystem, sometimes you would
   need to mark your patch as a RFC patch, i.e., "[RFC PATCH frotz]",
   and that is done per-invocation basis (i.e., you are not always
   constantly sending an RFC) with "--rfc".

Having orthogonal two mechanisms whose results are concatenated
together is handy than having to specify the whole thing.

I somehow thought that during the review of the "--rfc" option a few
ideas were brought up to deal with adornments other than but similar
to RFC.  I still think the approach to make "--rfc" take an optional
value, e.g., "--rfc=WIP" from the repository working in "frotz"
subsystem would produce "[WIP PATCH frotz v2 2/4]" a reasonable one.

cf.  https://lore.kernel.org/git/xmqqbkepep9k.fsf@gitster.g/

Thanks.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

Hello Junio,

On 2024-04-17 17:27, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> Playing devil's advocate for a minute, is this really common enough to
>> justify a new option when the user can use "--subject-prefix='PATCH
>> RESEND'" instead?
> 
> The same applies to "--rfc", but the justification goes like this.
> 
>  * When you are working on a single subsystem in a larger project,
>    your patches would want to carry the subsystem name.  You'd use
>    "--subject-prefix='PATCH frotz'" (and more likely it comes from
>    format.subjectPrefix in a working repository dedicated to work on
>    the frotz subsystem) for that.
> 
>  * In the context of working on that subsystem, sometimes you would
>    need to mark your patch as a RFC patch, i.e., "[RFC PATCH frotz]",
>    and that is done per-invocation basis (i.e., you are not always
>    constantly sending an RFC) with "--rfc".
> 
> Having orthogonal two mechanisms whose results are concatenated
> together is handy than having to specify the whole thing.
> 
> I somehow thought that during the review of the "--rfc" option a few
> ideas were brought up to deal with adornments other than but similar
> to RFC.  I still think the approach to make "--rfc" take an optional
> value, e.g., "--rfc=WIP" from the repository working in "frotz"
> subsystem would produce "[WIP PATCH frotz v2 2/4]" a reasonable one.

With all due respect, "--rfc=WIP" looks like a kludge, simply
because "--rfc" should, IIUC, be some kind of a fixed shorthand.
Perhaps a new option should be added for that purpose, but I'm
not really sure how it could be called.

> cf.  https://lore.kernel.org/git/xmqqbkepep9k.fsf@gitster.g/
> 
> Thanks.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> With all due respect, "--rfc=WIP" looks like a kludge, simply
> because "--rfc" should, IIUC, be some kind of a fixed shorthand.

I wouldn't use "should" there.  In any case, we are not going to add
unbounded number of --wip, --resend, etc., on top of what we have
already.  Introducing --something={WIP,RESEND,RFC,HACK,...} and
deprecating --rfc is not something I would object to, though.

Thanks.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

On 2024-04-17 23:03, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> With all due respect, "--rfc=WIP" looks like a kludge, simply
>> because "--rfc" should, IIUC, be some kind of a fixed shorthand.
> 
> I wouldn't use "should" there.  In any case, we are not going to add
> unbounded number of --wip, --resend, etc., on top of what we have
> already.  Introducing --something={WIP,RESEND,RFC,HACK,...} and
> deprecating --rfc is not something I would object to, though.

Good to know, thanks.  I'll drop the patches that add "--resend"
as a new command-line option, and I'll think a bit about the solution
you described as acceptable.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

On 2024-04-17 23:09, Dragan Simic wrote:
> On 2024-04-17 23:03, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> 
>>> With all due respect, "--rfc=WIP" looks like a kludge, simply
>>> because "--rfc" should, IIUC, be some kind of a fixed shorthand.
>> 
>> I wouldn't use "should" there.  In any case, we are not going to add
>> unbounded number of --wip, --resend, etc., on top of what we have
>> already.  Introducing --something={WIP,RESEND,RFC,HACK,...} and
>> deprecating --rfc is not something I would object to, though.
> 
> Good to know, thanks.  I'll drop the patches that add "--resend"
> as a new command-line option, and I'll think a bit about the solution
> you described as acceptable.

How about introducing "--label=<string>" as the new option, where
"<string>" could also contain '$' as the last character, which would
get stripped while indicating that the label is to treated as a
(sub)suffix, instead of as a (sub)prefix.

For example, "--label=RFC" would be equal to the current "--rfc"
(which would also become deprecated), producing "[RFC PATCH]",
"--label=WIP" would produce "[WIP PATCH]", and "--label=RESEND$"
would produce "[PATCH RESEND]".

Specifying '$' before a space character in a command line doesn't
trigger parameter substitution or variable expansion by the shell,
which means that using '$' as a "suffix anchor", as proposed above,
would require no escaping or use of single quotation marks, making
it more convenient to use.

Please, let me know your thoughts.

Re: [PATCH 2/4] format-patch: fix a bug in option exclusivity and add a test to t4014

[Dragan Simic]

Hello Patrick,

On 2024-04-17 08:56, Dragan Simic wrote:
> On 2024-04-17 08:27, Patrick Steinhardt wrote:
>> On Wed, Apr 17, 2024 at 05:32:42AM +0200, Dragan Simic wrote:
>>> Fix a bug that allows --rfc and -k options to be specified together 
>>> when
>>> executing "git format-patch".  This bug was introduced back in the 
>>> commit
>>> e0d7db7423a9 ("format-patch: --rfc honors what --subject-prefix 
>>> sets"),
>>> about eight months ago, but it has remained undetected so far, 
>>> presumably
>>> because of no associated test coverage.
>>> 
>>> Add a new test to the t4014 that covers the mutual exclusivity of the 
>>> --rfc
>>> and -k command-line options for "git format-patch", for future 
>>> coverage.
>>> 
>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>>> ---
>>>  builtin/log.c           | 5 ++++-
>>>  t/t4014-format-patch.sh | 4 ++++
>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/builtin/log.c b/builtin/log.c
>>> index c0a8bb95e983..e5a238f1cf2c 100644
>>> --- a/builtin/log.c
>>> +++ b/builtin/log.c
>>> @@ -2050,8 +2050,11 @@ int cmd_format_patch(int argc, const char 
>>> **argv, const char *prefix)
>>>  	if (cover_from_description_arg)
>>>  		cover_from_description_mode = 
>>> parse_cover_from_description(cover_from_description_arg);
>>> 
>>> -	if (rfc)
>>> +	/* Also mark the subject prefix as modified, for later checks */
>>> +	if (rfc) {
>>>  		strbuf_insertstr(&sprefix, 0, "RFC ");
>>> +		subject_prefix = 1;
>>> +	}
>> 
>> As an alternative fix, can we drop `subject_prefix` and replace it 
>> with
>> `sprefix.len` instead? It seems to merely be a proxy value for that
>> anyway, and if we didn't have that variable then the bug would not 
>> have
>> been possible to begin with.
> 
> Thanks for your feedback!
> 
> I'll think about it, and I'll come back a bit later with an update.

Unfortunately, we can't use sprefix.len instead, because it can
still be zero even if the --subject-prefix option was present, more
specifically if we receive --subject-prefix="" on the command line.

The checks that use subject_prefix need to check if --subject-prefix
was specified at all as an option, instead of checking if the actual
subject prefix is of non-zero length.

As you already noted, if sprefix.len was used instead of the separate
subject_prefix variable, the '--rfc -k' bug wouldn't be possible, but
the new '--subject-prefix="" -k' bug would be possible instead.

>>>  	if (reroll_count) {
>>>  		strbuf_addf(&sprefix, " v%s", reroll_count);
>>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>>> index e37a1411ee24..e22c4ac34e6e 100755
>>> --- a/t/t4014-format-patch.sh
>>> +++ b/t/t4014-format-patch.sh
>>> @@ -1397,6 +1397,10 @@ test_expect_success '--rfc is argument order 
>>> independent' '
>>>  	test_cmp expect actual
>>>  '
>>> 
>>> +test_expect_success '--rfc and -k cannot be used together' '
>>> +	test_must_fail git format-patch -1 --stdout --rfc -k >patch
>>> +'
>>> +
>>>  test_expect_success '--from=ident notices bogus ident' '
>>>  	test_must_fail git format-patch -1 --stdout --from=foo >patch
>>>  '

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

Hello Eric,

On 2024-04-17 09:05, Dragan Simic wrote:
> On 2024-04-17 08:35, Eric Sunshine wrote:
>> On Tue, Apr 16, 2024 at 11:33 PM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>>> diff --git a/builtin/log.c b/builtin/log.c
>>> @@ -2111,7 +2116,9 @@ int cmd_format_patch(int argc, const char 
>>> **argv, const char *prefix)
>>>         if (keep_subject && subject_prefix)
>>> -               die(_("options '%s' and '%s' cannot be used 
>>> together"), "--subject-prefix/--rfc", "-k");
>>> +               die(_("options '%s' and '%s' cannot be used 
>>> together"), "--subject-prefix/--rfc/--resend", "-k");
>> 
>> You probably want to be using die_for_incompatible_opt4() from
>> parse-options.h here.
> 
> Thanks for the suggestion.  Frankly, I haven't researched the
> available options, assuming that the current code uses the right
> option.  Of course, I'll have a detailed look into it.

Unfortunately, die_for_incompatible_opt3() cannot be used because
it also prevents the --subject-prefix and --rfc options from being
used together, which is expected to be possible.

>> (And you may want a preparatory patch which fixes the preimage to use
>> die_for_incompatible_opt3() for --subject-prefix, --rfc, and -k
>> exclusivity, though that may be overkill.)
> 
> I'm not really sure what to do.  Maybe the other reviewers would
> prefer an orthogonal approach instead?  Maybe that would be better
> for bisecting later, if need arises for that?

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>>>> With all due respect, "--rfc=WIP" looks like a kludge, simply
>>>> because "--rfc" should, IIUC, be some kind of a fixed shorthand.
>>> I wouldn't use "should" there.

> How about introducing "--label=<string>" as the new option,...

I still think --rfc=WIP is a lot more natural and easier to
understand, and it is just the matter of how you introduce it.
I'll show you how in a separate patch later.

The problem I see with an overly generic word like "label" is that
it would mislead readers to say "--label=important" and expect it to
appear on an extra e-mail header, not as a part of "Subject:".

But we can do this to get the ball rolling, without bikeshedding
what option name to use.  Until we find a good name, users can
use --rfc=WIP and when we do find a good name, it can be added
as a synonym, possibly deprecating --rfc, and if we never agree
on a good name, that is fine as well.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

Hello Junio,

On 2024-04-19 00:34, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>>>>> With all due respect, "--rfc=WIP" looks like a kludge, simply
>>>>> because "--rfc" should, IIUC, be some kind of a fixed shorthand.
>>>> I wouldn't use "should" there.
> 
>> How about introducing "--label=<string>" as the new option,...
> 
> I still think --rfc=WIP is a lot more natural and easier to
> understand, and it is just the matter of how you introduce it.
> I'll show you how in a separate patch later.
> 
> The problem I see with an overly generic word like "label" is that
> it would mislead readers to say "--label=important" and expect it to
> appear on an extra e-mail header, not as a part of "Subject:".

"Label" is a little generic, I'll give you that.

> But we can do this to get the ball rolling, without bikeshedding
> what option name to use.  Until we find a good name, users can
> use --rfc=WIP and when we do find a good name, it can be added
> as a synonym, possibly deprecating --rfc, and if we never agree
> on a good name, that is fine as well.

If you insist, let's do it that way! :)

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Eric Sunshine]

On Thu, Apr 18, 2024 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> > How about introducing "--label=<string>" as the new option,...
>
> I still think --rfc=WIP is a lot more natural and easier to
> understand, and it is just the matter of how you introduce it.
> I'll show you how in a separate patch later.
>
> The problem I see with an overly generic word like "label" is that
> it would mislead readers to say "--label=important" and expect it to
> appear on an extra e-mail header, not as a part of "Subject:".
>
> But we can do this to get the ball rolling, without bikeshedding
> what option name to use.  Until we find a good name, users can
> use --rfc=WIP and when we do find a good name, it can be added
> as a synonym, possibly deprecating --rfc, and if we never agree
> on a good name, that is fine as well.

I remain skeptical that adding such an option is necessary, even
though I made a similar suggestion earlier in this discussion as an
alternative to `--resend`. I'm especially skeptical since the existing
`--subject-prefix` covers this use-case already (i.e.
`--subject-prefix="RESEND PATCH"`). It's dead simple to use and
doesn't require any magical incantations with corresponding complex
implementation such as the proposed `--label=RESEND$` which renders as
"[PATCH RESEND]" instead of "[RESEND PATCH]"; `--subject-prefix`
already handles this without any need for magic.

I do understand and am sympathetic to the desire to reduce the typing
load (hence, the original `--resend` proposal), but I have difficulty
believing that `git format-patch` is so commonly used throughout the
day that the time saved by typing `--resend` over
`--subject-prefix="RESEND PATCH"` warrants the extra implementation,
documentation, and testing baggage. Likewise, I don't see the value in
`--label=WIP` (or `--rfc=WIP` or whatever) over the existing more
general `--subject-prefix`.

If reducing the typing load is the primary concern, then a very simple
middle-ground would be to give `--subject-prefix` a short alias (i.e.
`-S`). It's true that `-S "RESEND PATCH"` doesn't reduce the typing
load as much as `--resend` does over `--subject-prefix="RESEND
PATCH"`, but it seems a reasonable alternative which doesn't
significantly increase implementation, documentation, and testing
costs.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Dragan Simic]

Hello Eric,

On 2024-04-19 02:15, Eric Sunshine wrote:
> On Thu, Apr 18, 2024 at 6:34 PM Junio C Hamano <gitster@pobox.com> 
> wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> > How about introducing "--label=<string>" as the new option,...
>> 
>> I still think --rfc=WIP is a lot more natural and easier to
>> understand, and it is just the matter of how you introduce it.
>> I'll show you how in a separate patch later.
>> 
>> The problem I see with an overly generic word like "label" is that
>> it would mislead readers to say "--label=important" and expect it to
>> appear on an extra e-mail header, not as a part of "Subject:".
>> 
>> But we can do this to get the ball rolling, without bikeshedding
>> what option name to use.  Until we find a good name, users can
>> use --rfc=WIP and when we do find a good name, it can be added
>> as a synonym, possibly deprecating --rfc, and if we never agree
>> on a good name, that is fine as well.
> 
> I remain skeptical that adding such an option is necessary, even
> though I made a similar suggestion earlier in this discussion as an
> alternative to `--resend`. I'm especially skeptical since the existing
> `--subject-prefix` covers this use-case already (i.e.
> `--subject-prefix="RESEND PATCH"`). It's dead simple to use and
> doesn't require any magical incantations with corresponding complex
> implementation such as the proposed `--label=RESEND$` which renders as
> "[PATCH RESEND]" instead of "[RESEND PATCH]"; `--subject-prefix`
> already handles this without any need for magic.
> 
> I do understand and am sympathetic to the desire to reduce the typing
> load (hence, the original `--resend` proposal), but I have difficulty
> believing that `git format-patch` is so commonly used throughout the
> day that the time saved by typing `--resend` over
> `--subject-prefix="RESEND PATCH"` warrants the extra implementation,
> documentation, and testing baggage. Likewise, I don't see the value in
> `--label=WIP` (or `--rfc=WIP` or whatever) over the existing more
> general `--subject-prefix`.

An additional reason, IMHO, for having "--rfc", "--rfc=<string>"
or "--resend" is to reuse what's already configured through the
"format.subjectPrefix" configuration option.  In the sense of not
redefining what's already configured in ~/.gitconfig (in this case,
"PATCH" or "PATCH lib", for example), by specifying an additional
command-line option.

If some user configures different values for "format.subjectPrefix"
in different local repositories, such as when working on different
subsystems, it becomes rather easy to get lost in all those prefixes,
if the user needs to remember and type them entirely while using
"--subject-prefix=<string>" to add more "labels" to a prefix.

I hope it makes sense the way I wrote it above.

> If reducing the typing load is the primary concern, then a very simple
> middle-ground would be to give `--subject-prefix` a short alias (i.e.
> `-S`). It's true that `-S "RESEND PATCH"` doesn't reduce the typing
> load as much as `--resend` does over `--subject-prefix="RESEND
> PATCH"`, but it seems a reasonable alternative which doesn't
> significantly increase implementation, documentation, and testing
> costs.

I'd support the addition of a short alias for the already existing
"--subject-prefix" option.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> I do understand and am sympathetic to the desire to reduce the typing
> load (hence, the original `--resend` proposal), but I have difficulty
> believing that `git format-patch` is so commonly used throughout the
> day that the time saved by typing `--resend` over
> `--subject-prefix="RESEND PATCH"` warrants the extra implementation,
> documentation, and testing baggage. Likewise, I don't see the value in
> `--label=WIP` (or `--rfc=WIP` or whatever) over the existing more
> general `--subject-prefix`.

I am not interested in adding unbounded number of --wip and the like
at all, but the value you seem to be missing of the separate "--rfc"
is that there are folks who configure something other than "PATCH"
to "format.subjectPrefix".  They do not want to keep typing
--subject-prefix="PATCH net-next" on the command line, so they use
the configuration variable, which is "set it once and forget".  The
stress is on the fact that they can forget about it.

If they are told to say --subject-prefix="RFC PATCH net-next" when
they want to send an RFC patch as one-shot basis, they would not be
happy.  That is where the value of a command line "--rfc" for a
particular invocation is---they don't have to remember or care that
their normal subject prefix is "PATCH net-next", which is required
if you forced them to use --subject-prefix.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Eric Sunshine]

On Thu, Apr 18, 2024 at 8:45 PM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2024-04-19 02:15, Eric Sunshine wrote:
> > I do understand and am sympathetic to the desire to reduce the typing
> > load (hence, the original `--resend` proposal), but I have difficulty
> > believing that `git format-patch` is so commonly used throughout the
> > day that the time saved by typing `--resend` over
> > `--subject-prefix="RESEND PATCH"` warrants the extra implementation,
> > documentation, and testing baggage. Likewise, I don't see the value in
> > `--label=WIP` (or `--rfc=WIP` or whatever) over the existing more
> > general `--subject-prefix`.
>
> An additional reason, IMHO, for having "--rfc", "--rfc=<string>"
> or "--resend" is to reuse what's already configured through the
> "format.subjectPrefix" configuration option.  In the sense of not
> redefining what's already configured in ~/.gitconfig (in this case,
> "PATCH" or "PATCH lib", for example), by specifying an additional
> command-line option.
>
> If some user configures different values for "format.subjectPrefix"
> in different local repositories, such as when working on different
> subsystems, it becomes rather easy to get lost in all those prefixes,
> if the user needs to remember and type them entirely while using
> "--subject-prefix=<string>" to add more "labels" to a prefix.
>
> I hope it makes sense the way I wrote it above.

Yes, that makes sense. I wasn't aware of that behavior, as I have
never had a need to set that configuration.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Eric Sunshine]

On Thu, Apr 18, 2024 at 10:13 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > I do understand and am sympathetic to the desire to reduce the typing
> > load (hence, the original `--resend` proposal), but I have difficulty
> > believing that `git format-patch` is so commonly used throughout the
> > day that the time saved by typing `--resend` over
> > `--subject-prefix="RESEND PATCH"` warrants the extra implementation,
> > documentation, and testing baggage. Likewise, I don't see the value in
> > `--label=WIP` (or `--rfc=WIP` or whatever) over the existing more
> > general `--subject-prefix`.
>
> I am not interested in adding unbounded number of --wip and the like
> at all, but the value you seem to be missing of the separate "--rfc"
> is that there are folks who configure something other than "PATCH"
> to "format.subjectPrefix".  They do not want to keep typing
> --subject-prefix="PATCH net-next" on the command line, so they use
> the configuration variable, which is "set it once and forget".  The
> stress is on the fact that they can forget about it.

Indeed. I was unaware of that behavior.

Re: [PATCH 3/4] format-patch: new --resend option for adding "RESEND" to patch subjects

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> ..., but the value you seem to be missing of the separate "--rfc"
>> is that there are folks who configure something other than "PATCH"
>> to "format.subjectPrefix".  They do not want to keep typing
>> --subject-prefix="PATCH net-next" on the command line, so they use
>> the configuration variable, which is "set it once and forget".  The
>> stress is on the fact that they can forget about it.
>
> Indeed. I was unaware of that behavior.

The very original --rfc did overwrote --subject-prefix and did not
have a good reason to exist.  But with the relatively recent update,
it gained its usefulness.

Thanks.