| Message ID | 1d9c6ce3df714211889453c245485d46b43edff6.1713324598.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | format-patch: fix an option coexistence bug and add new --resend option | expand |
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.
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.)
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.
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).
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?
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.
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.
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! :)
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); >
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.
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
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?
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 ```
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.
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.
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.
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.
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.
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.
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?
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.
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! :)
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.
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.
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.
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.
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.
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.
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);
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(-)