[GSOC] format-patch: allow a non-integral version numbers

[adlternative]

A rollback was performed again under Eric's suggestion.

this want to fix #882
Thanks.

cc: Junio C Hamano gitster@pobox.com
cc: Denton Liu liu.denton@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com
cc: Đoàn Trần Công Danh congdanhqx@gmail.com

[adlternative]

/submit

[gitgitgadget]

Submitted as pull.885.git.1614269753194.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v1

To fetch this version to local tag pr-885/adlternative/format_patch_non_intergral-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-885/adlternative/format_patch_non_intergral-v1

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Thu, Feb 25, 2021 at 11:19 AM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Usually we can only use `format-patch -v<n>` to generate integral
> version numbers patches, but if we can provide `format-patch` with
> non-integer versions numbers of patches, this may help us to send patches
> such as "v1.1" versions sometimes.

On the Git project itself, fractional or non-numeric re-roll "numbers"
are not necessarily encouraged[1], so this feature may not be
particularly useful here, though perhaps some other project might
benefit from it(?). Usually, you would want to justify why the change
is desirable. Denton did give a bit of justification in his
proposal[2] for this feature, so perhaps update this commit message by
copying some of what he wrote as justification.

[1]: I think I've only seen Denton send fractional re-rolls; other
people sometimes send a periodic "fixup!" patch, but both approaches
place extra burden on the project maintainer than merely re-rolling
the entire series with a new integer re-roll count.

[2]: https://github.com/gitgitgadget/git/issues/882

> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> @@ -215,12 +215,12 @@ populated with placeholder text.
>  -v <n>::
>  --reroll-count=<n>::
> -       Mark the series as the <n>-th iteration of the topic. The
> +       Mark the series as the specified version of the topic. The
>         output filenames have `v<n>` prepended to them, and the
>         subject prefix ("PATCH" by default, but configurable via the
>         `--subject-prefix` option) has ` v<n>` appended to it.  E.g.
> -       `--reroll-count=4` may produce `v4-0001-add-makefile.patch`
> -       file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
> +       `--reroll-count 4.4` may produce `v4.4-0001-add-makefile.patch`
> +       file that has "Subject: [PATCH v4.4 1/20] Add makefile" in it.

I'm not sure we want to encourage the use of fractional re-roll counts
by using it in an example like this. It would probably be better to
leave the example as-is. If you really want people to know that
fractional re-roll counts are supported, perhaps add separate sentence
saying that they are.

> diff --git a/builtin/log.c b/builtin/log.c
> @@ -1662,13 +1662,13 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
> -static const char *diff_title(struct strbuf *sb, int reroll_count,
> +static const char *diff_title(struct strbuf *sb, const char *reroll_count,
>                        const char *generic, const char *rerolled)
>  {
> -       if (reroll_count <= 0)
> +       if (!reroll_count)
>                 strbuf_addstr(sb, generic);
>         else /* RFC may be v0, so allow -v1 to diff against v0 */
> -               strbuf_addf(sb, rerolled, reroll_count - 1);
> +               strbuf_addf(sb, rerolled, "last version");
>         return sb->buf;
>  }

There are a couple problems here (at least). First, the string "last
version" should be localizable, `_("last version")`. Second, in
Denton's proposal[2], he suggested using the string "last version"
_only_ if the re-roll count is not an integer. What you have here
applies "last version" unconditionally when -v is used so that the
outcome is _always_ "Range-diff since last version". If that's what
you intend to do, there's no reason to do any sort of interpolation
using the template "Range-diff since %". What Denton had in mind was
this (using pseudo-code):

    if re-roll count not specified:
        message = "Range-diff"
    else if re-roll count is integer:
        message = "Range-diff since v%d", re-roll
    else:
        message = "Range-diff since v%s", re-roll

However, there isn't a good reason to favor "Range-diff since last
version" over the simpler generic message "Range-diff". So, the above
should be collapsed to:

    if re-roll count is specified and integer:
        message = "Range-diff since v%d", re-roll
    else:
        message = "Range-diff"

> @@ -2080,7 +2080,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> -                                            _("Interdiff against v%d:"));
> +                                            _("Interdiff against %s:"));
> @@ -2099,7 +2099,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> -                                            _("Range-diff against v%d:"));
> +                                            _("Range-diff against %s:"));

If you follow my recommendation above using the simplified
conditional, then you don't need to drop the "v" since you won't be
saying "last version".

[gitgitgadget]

User Eric Sunshine <sunshine@sunshineco.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: ZheNing Hu <adlternative@gmail.com>
>
> Usually we can only use `format-patch -v<n>` to generate integral
> version numbers patches, but if we can provide `format-patch` with
> non-integer versions numbers of patches, this may help us to send patches
> such as "v1.1" versions sometimes.

A pro-tip.  Do not make your change sound like "because we can do
this"; a change sells a lot easier if it were "because we need
this---see how useful it is in such and such cases".

I am not in principle opposed to support a fractional version number
that comes after an integral one, which allows users to signal that
it is a minor correction that does not deserve a full version bump.

But not in this form.

 - This implementation regresses the output of the diff_version()
   helper function, and does so deliberately, even by butchering a
   test that protects it to hide the regression from the test suite.

   "This is the change from the last version" is expecting too much
   from the reviewers.  Those who may recall seeing your v3 may have
   been otherwise occupied for a few weeks and ununware of your v4.

 - It also closes the door to a feature that is occasionally
   requested to update the make_cover_letter() to carry title and
   description over from the previous iteration.

If we were to do this, I would probably suggest a preliminary patch
that refactors the hardcoded "reroll_count - 1" out of diff_title()
so that the helper takes two "reroll count strings", i.e. reroll
count for this round, and the previous round, as two separate
parameters.  Teach the caller to pass "reroll_count - 1" for the new
parameter in this preliminary step.

Then in the main patch, soon after we finish parsing the command
line options to learn we got "-v$N" argument:

 (0) It might be acceptable to teach the command a new option to
     allow end-users to explicitly give the previous reroll count
     string from the command line.  If we were to do so, skip
     everything below when such an option is used and use the
     user-supplied one.

 (1) If $N is an integer, use $N and $N-1 as the reroll count
     strings for this and previous round.

 (2) Otherwise, scan for "v(.*)-0*-cover-letter.$suffix" in the
     output directory to find all previous rerolls, and pick the one
     that sorts the last and before $N (use versioncmp).  If there
     are existing v1-, v2-, etc., and if we are given "-v2.1" from
     the command line, we'd want to grab v2 as the previous reroll
     count.

 (3) After doing the above steps, if we still do not have the
     previous reroll count string, error out.

     You could argue that we may optionally work in degraded mode,
     using "last version" as the fallback value for the previous
     round count string, and it may work for some things (e.g. the
     "change against the last round" label) and not for other things
     (e.g. reuse description in the previous cover letter).  I would
     not recommend it.

And then, we have two reroll count strings that we can feed
diff_title(); we also can later use the reroll count string for the
previous round to teach make_cover_letter() read and recover the
description from the cover letter of the previous round.

Thanks.

>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>     [GSOC] format-patch: allow a non-integral version numbers
>     
>      * format-patch previously only integer version number -v<n>, now trying
>        to provide a non-integer version.
>     
>     this want to fix #882 Thanks.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-885%2Fadlternative%2Fformat_patch_non_intergral-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/885
>
>  Documentation/git-format-patch.txt |  6 +++---
>  builtin/log.c                      | 20 ++++++++++----------
>  log-tree.c                         |  4 ++--
>  revision.h                         |  2 +-
>  t/t4014-format-patch.sh            |  8 ++++----
>  5 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 3e49bf221087..0cc39dbf573c 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -215,12 +215,12 @@ populated with placeholder text.
>  
>  -v <n>::
>  --reroll-count=<n>::
> -	Mark the series as the <n>-th iteration of the topic. The
> +	Mark the series as the specified version of the topic. The
>  	output filenames have `v<n>` prepended to them, and the
>  	subject prefix ("PATCH" by default, but configurable via the
>  	`--subject-prefix` option) has ` v<n>` appended to it.  E.g.
> -	`--reroll-count=4` may produce `v4-0001-add-makefile.patch`
> -	file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
> +	`--reroll-count 4.4` may produce `v4.4-0001-add-makefile.patch`
> +	file that has "Subject: [PATCH v4.4 1/20] Add makefile" in it.
>  
>  --to=<email>::
>  	Add a `To:` header to the email headers. This is in addition
> diff --git a/builtin/log.c b/builtin/log.c
> index f67b67d80ed1..72af140b812e 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1662,13 +1662,13 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
>  	oidclr(&bases->base_commit);
>  }
>  
> -static const char *diff_title(struct strbuf *sb, int reroll_count,
> +static const char *diff_title(struct strbuf *sb, const char *reroll_count,
>  		       const char *generic, const char *rerolled)
>  {
> -	if (reroll_count <= 0)
> +	if (!reroll_count)
>  		strbuf_addstr(sb, generic);
>  	else /* RFC may be v0, so allow -v1 to diff against v0 */
> -		strbuf_addf(sb, rerolled, reroll_count - 1);
> +		strbuf_addf(sb, rerolled, "last version");
>  	return sb->buf;
>  }
>  
> @@ -1717,7 +1717,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	struct strbuf buf = STRBUF_INIT;
>  	int use_patch_format = 0;
>  	int quiet = 0;
> -	int reroll_count = -1;
> +	const char *reroll_count = NULL;
>  	char *cover_from_description_arg = NULL;
>  	char *branch_name = NULL;
>  	char *base_commit = NULL;
> @@ -1751,8 +1751,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			    N_("use <sfx> instead of '.patch'")),
>  		OPT_INTEGER(0, "start-number", &start_number,
>  			    N_("start numbering patches at <n> instead of 1")),
> -		OPT_INTEGER('v', "reroll-count", &reroll_count,
> -			    N_("mark the series as Nth re-roll")),
> +		OPT_STRING('v', "reroll-count", &reroll_count, N_("reroll-count"),
> +			    N_("mark the series as specified version re-roll")),
>  		OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
>  			    N_("max length of output filename")),
>  		OPT_CALLBACK_F(0, "rfc", &rev, NULL,
> @@ -1862,9 +1862,9 @@ 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 (0 < reroll_count) {
> +	if (reroll_count) {
>  		struct strbuf sprefix = STRBUF_INIT;
> -		strbuf_addf(&sprefix, "%s v%d",
> +		strbuf_addf(&sprefix, "%s v%s",
>  			    rev.subject_prefix, reroll_count);
>  		rev.reroll_count = reroll_count;
>  		rev.subject_prefix = strbuf_detach(&sprefix, NULL);
> @@ -2080,7 +2080,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		rev.idiff_oid2 = get_commit_tree_oid(list[0]);
>  		rev.idiff_title = diff_title(&idiff_title, reroll_count,
>  					     _("Interdiff:"),
> -					     _("Interdiff against v%d:"));
> +					     _("Interdiff against %s:"));
>  	}
>  
>  	if (creation_factor < 0)
> @@ -2099,7 +2099,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		rev.creation_factor = creation_factor;
>  		rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
>  					     _("Range-diff:"),
> -					     _("Range-diff against v%d:"));
> +					     _("Range-diff against %s:"));
>  	}
>  
>  	if (!signature) {
> diff --git a/log-tree.c b/log-tree.c
> index 4531cebfab38..5f2e08ebcaab 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename,
>  	int start_len = filename->len;
>  	int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);
>  
> -	if (0 < info->reroll_count)
> -		strbuf_addf(filename, "v%d-", info->reroll_count);
> +	if (info->reroll_count)
> +		strbuf_addf(filename, "v%s-", info->reroll_count);
>  	strbuf_addf(filename, "%04d-%s", nr, subject);
>  
>  	if (max_len < filename->len)
> diff --git a/revision.h b/revision.h
> index e6be3c845e66..097d08354c61 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -235,7 +235,7 @@ struct rev_info {
>  	const char	*mime_boundary;
>  	const char	*patch_suffix;
>  	int		numbered_files;
> -	int		reroll_count;
> +	const char	*reroll_count;
>  	char		*message_id;
>  	struct ident_split from_ident;
>  	struct string_list *ref_message_ids;
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 66630c8413d5..b911e08f0810 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -372,10 +372,10 @@ test_expect_success 'filename limit applies only to basename' '
>  
>  test_expect_success 'reroll count' '
>  	rm -fr patches &&
> -	git format-patch -o patches --cover-letter --reroll-count 4 main..side >list &&
> -	! grep -v "^patches/v4-000[0-3]-" list &&
> +	git format-patch -o patches --cover-letter --reroll-count 4.4 main..side >list &&
> +	! grep -v "^patches/v4.4-000[0-3]-" list &&
>  	sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> -	! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
> +	! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
>  '
>  
>  test_expect_success 'reroll count (-v)' '
> @@ -2252,7 +2252,7 @@ test_expect_success 'interdiff: cover-letter' '
>  
>  test_expect_success 'interdiff: reroll-count' '
>  	git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop &&
> -	test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
> +	test_i18ngrep "^Interdiff ..* last version:$" v2-0000-cover-letter.patch
>  '
>  
>  test_expect_success 'interdiff: solo-patch' '
>
> base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07

[gitgitgadget]

On the Git mailing list, ZheNing Hu wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> 于2021年2月26日周五 上午1:57写道：
>
> On Thu, Feb 25, 2021 at 11:19 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Usually we can only use `format-patch -v<n>` to generate integral
> > version numbers patches, but if we can provide `format-patch` with
> > non-integer versions numbers of patches, this may help us to send patches
> > such as "v1.1" versions sometimes.
>
> On the Git project itself, fractional or non-numeric re-roll "numbers"
> are not necessarily encouraged[1], so this feature may not be
> particularly useful here, though perhaps some other project might
> benefit from it(?). Usually, you would want to justify why the change
> is desirable. Denton did give a bit of justification in his
> proposal[2] for this feature, so perhaps update this commit message by
> copying some of what he wrote as justification.
>
OK, I will remember it.
> [1]: I think I've only seen Denton send fractional re-rolls; other
> people sometimes send a periodic "fixup!" patch, but both approaches
> place extra burden on the project maintainer than merely re-rolling
> the entire series with a new integer re-roll count.
>
> [2]: https://github.com/gitgitgadget/git/issues/882
>
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> > @@ -215,12 +215,12 @@ populated with placeholder text.
> >  -v <n>::
> >  --reroll-count=<n>::
> > -       Mark the series as the <n>-th iteration of the topic. The
> > +       Mark the series as the specified version of the topic. The
> >         output filenames have `v<n>` prepended to them, and the
> >         subject prefix ("PATCH" by default, but configurable via the
> >         `--subject-prefix` option) has ` v<n>` appended to it.  E.g.
> > -       `--reroll-count=4` may produce `v4-0001-add-makefile.patch`
> > -       file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
> > +       `--reroll-count 4.4` may produce `v4.4-0001-add-makefile.patch`
> > +       file that has "Subject: [PATCH v4.4 1/20] Add makefile" in it.
>
> I'm not sure we want to encourage the use of fractional re-roll counts
> by using it in an example like this. It would probably be better to
> leave the example as-is. If you really want people to know that
> fractional re-roll counts are supported, perhaps add separate sentence
> saying that they are.
Yes, but the original description `<n>-th iteration` may imply that the version
number is an integer. Is there any good way to solve it?
>
> > diff --git a/builtin/log.c b/builtin/log.c
> > @@ -1662,13 +1662,13 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
> > -static const char *diff_title(struct strbuf *sb, int reroll_count,
> > +static const char *diff_title(struct strbuf *sb, const char *reroll_count,
> >                        const char *generic, const char *rerolled)
> >  {
> > -       if (reroll_count <= 0)
> > +       if (!reroll_count)
> >                 strbuf_addstr(sb, generic);
> >         else /* RFC may be v0, so allow -v1 to diff against v0 */
> > -               strbuf_addf(sb, rerolled, reroll_count - 1);
> > +               strbuf_addf(sb, rerolled, "last version");
> >         return sb->buf;
> >  }
>
> There are a couple problems here (at least). First, the string "last
> version" should be localizable, `_("last version")`. Second, in
> Denton's proposal[2], he suggested using the string "last version"
> _only_ if the re-roll count is not an integer. What you have here
> applies "last version" unconditionally when -v is used so that the
> outcome is _always_ "Range-diff since last version". If that's what
> you intend to do, there's no reason to do any sort of interpolation
> using the template "Range-diff since %". What Denton had in mind was
> this (using pseudo-code):
>
>     if re-roll count not specified:
>         message = "Range-diff"
>     else if re-roll count is integer:
>         message = "Range-diff since v%d", re-roll
>     else:
>         message = "Range-diff since v%s", re-roll
>
> However, there isn't a good reason to favor "Range-diff since last
> version" over the simpler generic message "Range-diff". So, the above
> should be collapsed to:interpolation
>
>     if re-roll count is specified and integer:
>         message = "Range-diff since v%d", re-roll
>     else:
>         message = "Range-diff"
>
You mean using "Range-diff since %" may not be as
 good as" Range-diff" without sorting. I agree with you.
> > @@ -2080,7 +2080,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > -                                            _("Interdiff against v%d:"));
> > +                                            _("Interdiff against %s:"));
> > @@ -2099,7 +2099,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > -                                            _("Range-diff against v%d:"));
> > +                                            _("Range-diff against %s:"));
>
> If you follow my recommendation above using the simplified
> conditional, then you don't need to drop the "v" since you won't be
> saying "last version".

Your suggestion is very good and easy to implement, but I may have
made some changes in Junio suggestion later, that is, I used the
 `previous_count` method to provide it to `diff_title()`. I will explain
my thoughts and problems in my reply to Junio. You'll see it later.
Thank you for your help.

--
ZheNing Hu

[gitgitgadget]

On the Git mailing list, ZheNing Hu wrote (reply to this):

Junio C Hamano <gitster@pobox.com> 于2021年2月26日周五 上午2:13写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Usually we can only use `format-patch -v<n>` to generate integral
> > version numbers patches, but if we can provide `format-patch` with
> > non-integer versions numbers of patches, this may help us to send patches
> > such as "v1.1" versions sometimes.
>
> A pro-tip.  Do not make your change sound like "because we can do
> this"; a change sells a lot easier if it were "because we need
> this---see how useful it is in such and such cases".
OK. I will fully express the advantages of these patches in the future.
>
> I am not in principle opposed to support a fractional version number
> that comes after an integral one, which allows users to signal that
> it is a minor correction that does not deserve a full version bump.
>
> But not in this form.
>
>  - This implementation regresses the output of the diff_version()
>    helper function, and does so deliberately, even by butchering a
>    test that protects it to hide the regression from the test suite.
>
>    "This is the change from the last version" is expecting too much
>    from the reviewers.  Those who may recall seeing your v3 may have
>    been otherwise occupied for a few weeks and ununware of your v4.
>
>  - It also closes the door to a feature that is occasionally
>    requested to update the make_cover_letter() to carry title and
>    description over from the previous iteration.
>
Indeed.
> If we were to do this, I would probably suggest a preliminary patch
> that refactors the hardcoded "reroll_count - 1" out of diff_title()
> so that the helper takes two "reroll count strings", i.e. reroll
> count for this round, and the previous round, as two separate
> parameters.  Teach the caller to pass "reroll_count - 1" for the new
> parameter in this preliminary step.
>
I like the idea.
> Then in the main patch, soon after we finish parsing the command
> line options to learn we got "-v$N" argument:
>
>  (0) It might be acceptable to teach the command a new option to
>      allow end-users to explicitly give the previous reroll count
>      string from the command line.  If we were to do so, skip
>      everything below when such an option is used and use the
>      user-supplied one.
>

>  (1) If $N is an integer, use $N and $N-1 as the reroll count
>      strings for this and previous round.
>
My patch still in WIP state,but (1) work here.

>  (2) Otherwise, scan for "v(.*)-0*-cover-letter.$suffix" in the
>      output directory to find all previous rerolls, and pick the one
>      that sorts the last and before $N (use versioncmp).  If there
>      are existing v1-, v2-, etc., and if we are given "-v2.1" from
>      the command line, we'd want to grab v2 as the previous reroll
>      count.
>
I have some doubts:
(1) if the user generates multiple patches through the `git format patch -v<n>`,
they may be placed under different folders, we may not assert what the
version number of the previous version of the patch is by looking at all
patches in one directory.
(2) "0000-1234-cover-letter.patch" this form may also need to be considered in
the v(. *)-0*- cover-letter.$suffix".
for the time being, this approach will be more cumbersome.
>  (3) After doing the above steps, if we still do not have the
>      previous reroll count string, error out.
>
>      You could argue that we may optionally work in degraded mode,
>      using "last version" as the fallback value for the previous
>      round count string, and it may work for some things (e.g. the
>      "change against the last round" label) and not for other things
>      (e.g. reuse description in the previous cover letter).  I would
>      not recommend it.
>
> And then, we have two reroll count strings that we can feed
> diff_title(); we also can later use the reroll count string for the
> previous round to teach make_cover_letter() read and recover the
> description from the cover letter of the previous round.
>
> Thanks.
>
I think the user should be allowed to provide the previous version number
 in the console. If the specified `reroll_count=<n>` is integrated, then we will
use `n-1` in the `diff_title`, or if the user specifies `
previous_count `, we will
use `previous_count` in `diff title`, otherwise, `generic` in the parameter of
`diff_title` will be used.
I don’t know if my ideas like this are inadequate, hope you can point it out.
> >
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> >     [GSOC] format-patch: allow a non-integral version numbers
> >
> >      * format-patch previously only integer version number -v<n>, now trying
> >        to provide a non-integer version.
> >
> >     this want to fix #882 Thanks.
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-885%2Fadlternative%2Fformat_patch_non_intergral-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/885
> >
> >  Documentation/git-format-patch.txt |  6 +++---
> >  builtin/log.c                      | 20 ++++++++++----------
> >  log-tree.c                         |  4 ++--
> >  revision.h                         |  2 +-
> >  t/t4014-format-patch.sh            |  8 ++++----
> >  5 files changed, 20 insertions(+), 20 deletions(-)
> >
> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> > index 3e49bf221087..0cc39dbf573c 100644
> > --- a/Documentation/git-format-patch.txt
> > +++ b/Documentation/git-format-patch.txt
> > @@ -215,12 +215,12 @@ populated with placeholder text.
> >
> >  -v <n>::
> >  --reroll-count=<n>::
> > -     Mark the series as the <n>-th iteration of the topic. The
> > +     Mark the series as the specified version of the topic. The
> >       output filenames have `v<n>` prepended to them, and the
> >       subject prefix ("PATCH" by default, but configurable via the
> >       `--subject-prefix` option) has ` v<n>` appended to it.  E.g.
> > -     `--reroll-count=4` may produce `v4-0001-add-makefile.patch`
> > -     file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
> > +     `--reroll-count 4.4` may produce `v4.4-0001-add-makefile.patch`
> > +     file that has "Subject: [PATCH v4.4 1/20] Add makefile" in it.
> >
> >  --to=<email>::
> >       Add a `To:` header to the email headers. This is in addition
> > diff --git a/builtin/log.c b/builtin/log.c
> > index f67b67d80ed1..72af140b812e 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -1662,13 +1662,13 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
> >       oidclr(&bases->base_commit);
> >  }
> >
> > -static const char *diff_title(struct strbuf *sb, int reroll_count,
> > +static const char *diff_title(struct strbuf *sb, const char *reroll_count,
> >                      const char *generic, const char *rerolled)
> >  {
> > -     if (reroll_count <= 0)
> > +     if (!reroll_count)
> >               strbuf_addstr(sb, generic);
> >       else /* RFC may be v0, so allow -v1 to diff against v0 */
> > -             strbuf_addf(sb, rerolled, reroll_count - 1);
> > +             strbuf_addf(sb, rerolled, "last version");
> >       return sb->buf;
> >  }
> >
> > @@ -1717,7 +1717,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >       struct strbuf buf = STRBUF_INIT;
> >       int use_patch_format = 0;
> >       int quiet = 0;
> > -     int reroll_count = -1;
> > +     const char *reroll_count = NULL;
> >       char *cover_from_description_arg = NULL;
> >       char *branch_name = NULL;
> >       char *base_commit = NULL;
> > @@ -1751,8 +1751,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >                           N_("use <sfx> instead of '.patch'")),
> >               OPT_INTEGER(0, "start-number", &start_number,
> >                           N_("start numbering patches at <n> instead of 1")),
> > -             OPT_INTEGER('v', "reroll-count", &reroll_count,
> > -                         N_("mark the series as Nth re-roll")),
> > +             OPT_STRING('v', "reroll-count", &reroll_count, N_("reroll-count"),
> > +                         N_("mark the series as specified version re-roll")),
> >               OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
> >                           N_("max length of output filename")),
> >               OPT_CALLBACK_F(0, "rfc", &rev, NULL,
> > @@ -1862,9 +1862,9 @@ 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 (0 < reroll_count) {
> > +     if (reroll_count) {
> >               struct strbuf sprefix = STRBUF_INIT;
> > -             strbuf_addf(&sprefix, "%s v%d",
> > +             strbuf_addf(&sprefix, "%s v%s",
> >                           rev.subject_prefix, reroll_count);
> >               rev.reroll_count = reroll_count;
> >               rev.subject_prefix = strbuf_detach(&sprefix, NULL);
> > @@ -2080,7 +2080,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >               rev.idiff_oid2 = get_commit_tree_oid(list[0]);
> >               rev.idiff_title = diff_title(&idiff_title, reroll_count,
> >                                            _("Interdiff:"),
> > -                                          _("Interdiff against v%d:"));
> > +                                          _("Interdiff against %s:"));
> >       }
> >
> >       if (creation_factor < 0)
> > @@ -2099,7 +2099,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >               rev.creation_factor = creation_factor;
> >               rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
> >                                            _("Range-diff:"),
> > -                                          _("Range-diff against v%d:"));
> > +                                          _("Range-diff against %s:"));
> >       }
> >
> >       if (!signature) {
> > diff --git a/log-tree.c b/log-tree.c
> > index 4531cebfab38..5f2e08ebcaab 100644
> > --- a/log-tree.c
> > +++ b/log-tree.c
> > @@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename,
> >       int start_len = filename->len;
> >       int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);
> >
> > -     if (0 < info->reroll_count)
> > -             strbuf_addf(filename, "v%d-", info->reroll_count);
> > +     if (info->reroll_count)
> > +             strbuf_addf(filename, "v%s-", info->reroll_count);
> >       strbuf_addf(filename, "%04d-%s", nr, subject);
> >
> >       if (max_len < filename->len)
> > diff --git a/revision.h b/revision.h
> > index e6be3c845e66..097d08354c61 100644
> > --- a/revision.h
> > +++ b/revision.h
> > @@ -235,7 +235,7 @@ struct rev_info {
> >       const char      *mime_boundary;
> >       const char      *patch_suffix;
> >       int             numbered_files;
> > -     int             reroll_count;
> > +     const char      *reroll_count;
> >       char            *message_id;
> >       struct ident_split from_ident;
> >       struct string_list *ref_message_ids;
> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index 66630c8413d5..b911e08f0810 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -372,10 +372,10 @@ test_expect_success 'filename limit applies only to basename' '
> >
> >  test_expect_success 'reroll count' '
> >       rm -fr patches &&
> > -     git format-patch -o patches --cover-letter --reroll-count 4 main..side >list &&
> > -     ! grep -v "^patches/v4-000[0-3]-" list &&
> > +     git format-patch -o patches --cover-letter --reroll-count 4.4 main..side >list &&
> > +     ! grep -v "^patches/v4.4-000[0-3]-" list &&
> >       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> > -     ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
> > +     ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> >  '
> >
> >  test_expect_success 'reroll count (-v)' '
> > @@ -2252,7 +2252,7 @@ test_expect_success 'interdiff: cover-letter' '
> >
> >  test_expect_success 'interdiff: reroll-count' '
> >       git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop &&
> > -     test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
> > +     test_i18ngrep "^Interdiff ..* last version:$" v2-0000-cover-letter.patch
> >  '
> >
> >  test_expect_success 'interdiff: solo-patch' '
> >
> > base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07

[adlternative]

/submit

[gitgitgadget]

Submitted as pull.885.v2.git.1614588030233.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v2

To fetch this version to local tag pr-885/adlternative/format_patch_non_intergral-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-885/adlternative/format_patch_non_intergral-v2

[gitgitgadget]

This branch is now known as zh/format-patch-fractional-reroll-count.

[gitgitgadget]

This patch series was integrated into seen via git@5a9fd04.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/log.c b/builtin/log.c
> index f67b67d80ed1..95c95eab5393 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1662,13 +1662,15 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
>  	oidclr(&bases->base_commit);
>  }
>  
> -static const char *diff_title(struct strbuf *sb, int reroll_count,
> -		       const char *generic, const char *rerolled)
> +static const char *diff_title(struct strbuf *sb, const char *reroll_count, int reroll_count_is_integer,
> +			const char*previous_count, const char *generic, const char *rerolled)

Avoid overly long lines here, but quite honestly, I find that this
interface is way too ugly to live.

Can we do all the computation around previous count in the caller,
so that this function only takes reroll_count and previous_count
that are both "const char *", and then the body will just be:

	if (!reroll_count)
		strbuf_addstr(sb, generic);
	else if (previous_count)
		strbuf_addf(sb, rerolled, previous_count);
	return sb->buf;

That way, the callers do not have to prepare two different rerolled
template and switch between them based on "is_integer".

In other words, they need to care "is_integer" already, so making
them responsible for preparing "previous_count" always usable by
this function would be a reasonable way to partition the tasks
between this callee and the caller.

That way, this function do not even need to know about "is_integer"
bit.

> +	if (previous_count && !reroll_count)
> +		usage(_("previous-count can only used when reroll-count is used"));
> +	if (reroll_count) {
>  		struct strbuf sprefix = STRBUF_INIT;
> -		strbuf_addf(&sprefix, "%s v%d",
> +		char ch;
> +		size_t i = 0 , reroll_count_len = strlen(reroll_count);
> +
> +		for (; i != reroll_count_len; i++) {
> +			ch = reroll_count[i];
> +			if(!isdigit(ch))
> +				break;
> +		}
> +		reroll_count_is_integer = i == reroll_count_len ? 1 : 0;

Do not reinvent integer parsing.  In our codebase, it is far more
common (and it is less error prone) to do something like this:

	char *endp;

	count = strtoul(reroll_count_string, &endp, 10);
	if (*endp) {
		/* followed by non-digit: not an integer */
		is_integer = 0;
	} else {
        	is_integer = 1;
                if (0 < count)
			previous_count_string = xstrfmt("%d", count - 1);
	}

And then, you can move the "if previous is there and count is not
specified" check after this block, to make sure that a non-integer
reroll count is always accompanied by a previous count, for example.

> +		strbuf_addf(&sprefix, "%s v%s",
>  			    rev.subject_prefix, reroll_count);
>  		rev.reroll_count = reroll_count;
>  		rev.subject_prefix = strbuf_detach(&sprefix, NULL);
> @@ -2079,8 +2095,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		rev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];
>  		rev.idiff_oid2 = get_commit_tree_oid(list[0]);
>  		rev.idiff_title = diff_title(&idiff_title, reroll_count,
> -					     _("Interdiff:"),
> -					     _("Interdiff against v%d:"));
> +			reroll_count_is_integer, previous_count, _("Interdiff:"),
> +				reroll_count_is_integer ? _("Interdiff against v%d:") :
> +					_("Interdiff against v%s:"));

I've touched the calling convention into diff_title() function
earlier.

> @@ -2098,8 +2115,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		rev.rdiff2 = rdiff2.buf;
>  		rev.creation_factor = creation_factor;
>  		rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
> -					     _("Range-diff:"),
> -					     _("Range-diff against v%d:"));
> +			reroll_count_is_integer, previous_count, _("Range-diff:"),
> +				reroll_count_is_integer ? _("Range-diff against v%d:") :
> +					_("Range-diff against v%s:"));

Ditto.

[gitgitgadget]

This patch series was integrated into seen via git@540fc33.

[gitgitgadget]

On the Git mailing list, Denton Liu wrote (reply to this):

Hi ZheNing,

Thanks for picking up the issue. It's good to see that the GGG issue
tracker is helpful.

On Mon, Mar 01, 2021 at 08:40:29AM +0000, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>
> 
> Usually we can only use `format-patch -v<n>` to generate integral
> version numbers patches, but sometimes a same fixup should be
> laveled as a non-integral versions like `v1.1`,so teach format-patch

Some typos:

s/laveled/labeled/; s/1.1`,/& /; s/format-patch/& to/

> allow a non-integral versions may be helpful to send those patches.
> 
> Since the original `format-patch` logic, if we specify a version `-v<n>`
> and commbine with `--interdiff` or `--rangediff`, the patch will output
> "Interdiff again v<n-1>:" or "Range-diff again v<n-1>:`, but this does
> not meet the requirements of our fractional version numbers, so provide
> `format patch` a new option `--previous-count=<n>`, the patch can output
> user-specified previous version number. If the user use a integral version
> number `-v<n>`, ensure that the output in the patch is still `v<n-1>`.
> (let `--previous-count` become invalid.)

Hmm, others may disagree but I don't really like the idea of
`--previous-count`. It may be useful for populating "Range-diff vs <n>"
instead of just "Range-diff" but I don't think it's worth the cost of
maintaining this option.


> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---

[...]

> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 3e49bf221087..f10fa6527f5f 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -22,6 +22,7 @@ SYNOPSIS
>  		   [--cover-from-description=<mode>]
>  		   [--rfc] [--subject-prefix=<subject prefix>]
>  		   [(--reroll-count|-v) <n>]
> +		   [--previous-count=<n>]
>  		   [--to=<email>] [--cc=<email>]
>  		   [--[no-]cover-letter] [--quiet]
>  		   [--[no-]encode-email-headers]
> @@ -221,6 +222,15 @@ populated with placeholder text.
>  	`--subject-prefix` option) has ` v<n>` appended to it.  E.g.
>  	`--reroll-count=4` may produce `v4-0001-add-makefile.patch`
>  	file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
> +	now can support non-integrated version number like `-v1.1`.
> +
> +--previous-count=<n>::
> +	Under the premise that we have used `--reroll-count=<n>`,
> +	we can use `--previous-count=<n>` to specify the previous
> +	version number. E.g. When we use the `--range-diff` or
> +	`--interdiff` option and combine with `-v2.3 --previous-count=2.2`,
> +	"Interdiff against v2.2:" or "Range-diff against v2.2:"
> +	will be output in the patch.
>  
>  --to=<email>::
>  	Add a `To:` header to the email headers. This is in addition
> diff --git a/builtin/log.c b/builtin/log.c
> index f67b67d80ed1..95c95eab5393 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1662,13 +1662,15 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
>  	oidclr(&bases->base_commit);
>  }
>  
> -static const char *diff_title(struct strbuf *sb, int reroll_count,
> -		       const char *generic, const char *rerolled)
> +static const char *diff_title(struct strbuf *sb, const char *reroll_count, int reroll_count_is_integer,
> +			const char*previous_count, const char *generic, const char *rerolled)
>  {
> -	if (reroll_count <= 0)
> +	if (!reroll_count || (!reroll_count_is_integer && !previous_count))
>  		strbuf_addstr(sb, generic);
> -	else /* RFC may be v0, so allow -v1 to diff against v0 */
> -		strbuf_addf(sb, rerolled, reroll_count - 1);
> +	else if (reroll_count_is_integer)/* RFC may be v0, so allow -v1 to diff against v0 */
> +		strbuf_addf(sb, rerolled, atoi(reroll_count) - 1);
> +	else if (previous_count)
> +		strbuf_addf(sb, rerolled, previous_count);
>  	return sb->buf;
>  }

 I would just remove this hunk entirely and keep the existing logic...

> @@ -1717,7 +1719,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	struct strbuf buf = STRBUF_INIT;
>  	int use_patch_format = 0;
>  	int quiet = 0;
> -	int reroll_count = -1;
> +	int reroll_count_is_integer = 0;
> +	const char *reroll_count = NULL;
> +	const char *previous_count = NULL;

...then over here, we can do something like

	const char *reroll_count = NULL;
	int reroll_count_int = -1;

and then...

>  	char *cover_from_description_arg = NULL;
>  	char *branch_name = NULL;
>  	char *base_commit = NULL;
> @@ -1751,8 +1755,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			    N_("use <sfx> instead of '.patch'")),
>  		OPT_INTEGER(0, "start-number", &start_number,
>  			    N_("start numbering patches at <n> instead of 1")),
> -		OPT_INTEGER('v', "reroll-count", &reroll_count,
> -			    N_("mark the series as Nth re-roll")),
> +		OPT_STRING('v', "reroll-count", &reroll_count, N_("reroll-count"),
> +			    N_("mark the series as specified version re-roll")),
> +		OPT_STRING(0, "previous-count", &previous_count, N_("previous-count"),
> +			    N_("specified as the last version while we use --reroll-count")),
>  		OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
>  			    N_("max length of output filename")),
>  		OPT_CALLBACK_F(0, "rfc", &rev, NULL,
> @@ -1861,10 +1867,20 @@ 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 (0 < reroll_count) {
> +	if (previous_count && !reroll_count)
> +		usage(_("previous-count can only used when reroll-count is used"));
> +	if (reroll_count) {
>  		struct strbuf sprefix = STRBUF_INIT;
> -		strbuf_addf(&sprefix, "%s v%d",
> +		char ch;
> +		size_t i = 0 , reroll_count_len = strlen(reroll_count);
> +
> +		for (; i != reroll_count_len; i++) {
> +			ch = reroll_count[i];
> +			if(!isdigit(ch))
> +				break;
> +		}
> +		reroll_count_is_integer = i == reroll_count_len ? 1 : 0;
> +		strbuf_addf(&sprefix, "%s v%s",
>  			    rev.subject_prefix, reroll_count);
>  		rev.reroll_count = reroll_count;
>  		rev.subject_prefix = strbuf_detach(&sprefix, NULL);

...over here we can use Junio's integer parsing example and assign
reroll_count_int only if reroll_count can be parsed into an integer.

Thanks,
Denton

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Denton Liu <liu.denton@gmail.com> writes:

> Hmm, others may disagree but I don't really like the idea of
> `--previous-count`. It may be useful for populating "Range-diff vs <n>"
> instead of just "Range-diff" but I don't think it's worth the cost of
> maintaining this option.

It really depends on the target audience.  As a reviewer who may be
too busy to read every iteration of a series, I would probably find
it useless if it gives just "range-diff" or "range-diff with last"
without saying which exact round.  Obviously, if you are not doing
range-diff, it will not be an issue.  If the patch requires (I
didn't read the latest one) the previous-count to be given when
range-diff or interdiff is not requested, it should probably be
fixed.

I am also OK with any design decision, as long as it will not close
the door for the occasionally requested feature to carry over cover
letter material from the previous round to the current one.

Thanks.

[gitgitgadget]

This patch series was integrated into seen via git@c98d78b.

[gitgitgadget]

On the Git mailing list, ZheNing Hu wrote (reply to this):

Junio C Hamano <gitster@pobox.com> 于2021年3月4日周四 上午8:54写道：
>
> Denton Liu <liu.denton@gmail.com> writes:
>
> > Hmm, others may disagree but I don't really like the idea of
> > `--previous-count`. It may be useful for populating "Range-diff vs <n>"
> > instead of just "Range-diff" but I don't think it's worth the cost of
> > maintaining this option.
>
> It really depends on the target audience.  As a reviewer who may be
> too busy to read every iteration of a series, I would probably find
> it useless if it gives just "range-diff" or "range-diff with last"
> without saying which exact round.  Obviously, if you are not doing
> range-diff, it will not be an issue.  If the patch requires (I
> didn't read the latest one) the previous-count to be given when
> range-diff or interdiff is not requested, it should probably be
> fixed.
>
> I am also OK with any design decision, as long as it will not close
> the door for the occasionally requested feature to carry over cover
> letter material from the previous round to the current one.
>
> Thanks.

What we are arguing now is whether it is necessary to add
"aginst v<previous_count>" to the patch when the non-integer version
number + rangediff/interdiff is required. Denton's point of view may be
similar to that of Eric before.

Here are my personal thoughts:

  Personally, I may use GGG more. When I see a title like "Range-diff vs v1:",
 I can know that this is a change from the previous v1, and it may be better
 than "Range-diff again v1" To be more specific, but if it is a small patch such
 as "v1.2", we use previous_count to tell the reviewer that this is a
range-diff
change from "v1.1" or other versions.

Of course this `previous count` can be used in a very small range, but
I think it
 doesn't hurt to keep it, because even if you don't use it, `format
patch` will still
output "Range-diff", which will not break any known functions. It can
only be said
that `previous count` provides an option for submitters to know the
previous version
 for reviewers. In this regard, I agree with Junio's point of view.

Thanks.

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Wed, Mar 3, 2021 at 9:08 PM ZheNing Hu <adlternative@gmail.com> wrote:
> What we are arguing now is whether it is necessary to add
> "aginst v<previous_count>" to the patch when the non-integer version
> number + rangediff/interdiff is required. Denton's point of view may be
> similar to that of Eric before.

Yes, it sounds as if Denton and I share the same point of view.

> Here are my personal thoughts:
>
> Of course this `previous count` can be used in a very small range, but
> I think it
>  doesn't hurt to keep it, because even if you don't use it, `format
> patch` will still
> output "Range-diff", which will not break any known functions. It can
> only be said
> that `previous count` provides an option for submitters to know the
> previous version
>  for reviewers. In this regard, I agree with Junio's point of view.

I'm not outright opposed to supporting non-numeric, non-integer
reroll-counts, but I also don't see a big need for it. As mentioned
earlier, Denton is the only person I recall who sends fractional
re-rolls, so it's not obvious that there is a big advantage to adding
such support and complicating the code just for one person. Also, when
Denton does send fractional re-rolls, he typically does so for just a
single patch out of a longer series, and he doesn't (I think) provide
a range-diff or interdiff for the patch. So, for Denton's intended
use-case, this entire discussion about "Range-diff against v$V" and
"Interdiff against v$V" seems superfluous. That is, the simple logic:

    if reroll_count specified and is integer:
        s = "Range-diff against v${reroll_count -1}:"
    else
        s = "Range-diff:"

satisfies Denton's case without the complication of adding a
--previous-count switch. This probably explains why Denton doesn't see
a need for the extra complexity of --previous-count.

So, some ways forward are:

(1) drop this topic altogether since it so far seems of interest to
only a single person (Denton) -- nobody else has asked for it

(2) support non-integer reroll-count but just use the simple logic
shown above for constructing the "Range-diff/Interdiff against"
header; this leaves the door open for Junio's idea(s) of allowing the
user to specify --previous-count and automatically determining the
previous reroll-count by scanning a directory

(3) continue refining the changes made by this patch until reviewers
are happy with it (which might take a few more re-rolls)

I lean toward #1, but wouldn't be opposed to #2 or #3 either.

[gitgitgadget]

On the Git mailing list, Denton Liu wrote (reply to this):

Hi Eric,

On Wed, Mar 03, 2021 at 10:27:25PM -0500, Eric Sunshine wrote:
> I'm not outright opposed to supporting non-numeric, non-integer
> reroll-counts, but I also don't see a big need for it. As mentioned
> earlier, Denton is the only person I recall who sends fractional
> re-rolls, so it's not obvious that there is a big advantage to adding
> such support and complicating the code just for one person.

Although it is quite rare, I'm definitely not the only one who's done
it. A cursory search on the list reveals a few examples from people
other than me:

* https://lore.kernel.org/git/20180308224458.5730-1-szeder.dev@gmail.com/
* https://lore.kernel.org/git/20190706162114.21169-1-szeder.dev@gmail.com/
* https://lore.kernel.org/git/20180225134015.26964-1-szeder.dev@gmail.com/
* https://lore.kernel.org/git/1435350166-7122-1-git-send-email-Matthieu.Moy@imag.fr/
* https://lore.kernel.org/git/574DED66.6050008@web.de/

[adlternative]

/submit

[gitgitgadget]

Submitted as pull.885.v3.git.1614859926974.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v3

To fetch this version to local tag pr-885/adlternative/format_patch_non_intergral-v3:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-885/adlternative/format_patch_non_intergral-v3

[gitgitgadget]

On the Git mailing list, Denton Liu wrote (reply to this):

Hi ZheNing,

On Thu, Mar 04, 2021 at 12:12:06PM +0000, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>
> 
> Usually we can only use `format-patch -v<n>` to generate integral
> version numbers patches, but sometimes a same fixup should be
> labeled as a non-integral versions like `v1.1`, so teach `format-patch`

s/versions/version/

> to allow a non-integral versions may be helpful to send those patches.

s/may be/which &/

> 
> Since the original `format-patch` logic, if we specify a version `-v<n>`
> and commbine with `--interdiff` or `--rangediff`, the patch will output
> "Interdiff again v<n-1>:" or "Range-diff again v<n-1>:`, but this does
> not meet the requirements of our fractional version numbers, so if the
> user use a integral version number `-v<n>`, ensure that the output in
> the patch is still `v<n-1>`; otherwise, only output "Interdiff" or
> "Range-diff".
> 
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>     [GSOC] format-patch: allow a non-integral version numbers
>     
>     There is a small question: in the case of --reroll-count=<n>, "n" is an
>     integer, we output "n-1" in the patch instead of "m" specified by
>     --previous-count=<m>,Should we switch the priority of these two: let "m"
>     output?
>     
>     this want to fix #882 Thanks.
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-885%2Fadlternative%2Fformat_patch_non_intergral-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/885
> 
> Range-diff vs v2:
> 
>  1:  800094cbf53b ! 1:  d4f38b78c464 format-patch: allow a non-integral version numbers
>      @@ Commit message
>       
>           Usually we can only use `format-patch -v<n>` to generate integral
>           version numbers patches, but sometimes a same fixup should be
>      -    laveled as a non-integral versions like `v1.1`,so teach format-patch
>      -    allow a non-integral versions may be helpful to send those patches.
>      +    labeled as a non-integral versions like `v1.1`, so teach `format-patch`
>      +    to allow a non-integral versions may be helpful to send those patches.
>       
>           Since the original `format-patch` logic, if we specify a version `-v<n>`
>           and commbine with `--interdiff` or `--rangediff`, the patch will output
>           "Interdiff again v<n-1>:" or "Range-diff again v<n-1>:`, but this does
>      -    not meet the requirements of our fractional version numbers, so provide
>      -    `format patch` a new option `--previous-count=<n>`, the patch can output
>      -    user-specified previous version number. If the user use a integral version
>      -    number `-v<n>`, ensure that the output in the patch is still `v<n-1>`.
>      -    (let `--previous-count` become invalid.)
>      +    not meet the requirements of our fractional version numbers, so if the
>      +    user use a integral version number `-v<n>`, ensure that the output in
>      +    the patch is still `v<n-1>`; otherwise, only output "Interdiff" or
>      +    "Range-diff".
>       
>           Signed-off-by: ZheNing Hu <adlternative@gmail.com>
>       
>        ## Documentation/git-format-patch.txt ##
>      -@@ Documentation/git-format-patch.txt: SYNOPSIS
>      - 		   [--cover-from-description=<mode>]
>      - 		   [--rfc] [--subject-prefix=<subject prefix>]
>      - 		   [(--reroll-count|-v) <n>]
>      -+		   [--previous-count=<n>]
>      - 		   [--to=<email>] [--cc=<email>]
>      - 		   [--[no-]cover-letter] [--quiet]
>      - 		   [--[no-]encode-email-headers]
>       @@ Documentation/git-format-patch.txt: populated with placeholder text.
>        	`--subject-prefix` option) has ` v<n>` appended to it.  E.g.
>        	`--reroll-count=4` may produce `v4-0001-add-makefile.patch`
>        	file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
>       +	now can support non-integrated version number like `-v1.1`.
>      -+
>      -+--previous-count=<n>::
>      -+	Under the premise that we have used `--reroll-count=<n>`,
>      -+	we can use `--previous-count=<n>` to specify the previous
>      -+	version number. E.g. When we use the `--range-diff` or
>      -+	`--interdiff` option and combine with `-v2.3 --previous-count=2.2`,
>      -+	"Interdiff against v2.2:" or "Range-diff against v2.2:"
>      -+	will be output in the patch.
>        
>        --to=<email>::
>        	Add a `To:` header to the email headers. This is in addition
>      @@ builtin/log.c: static void print_bases(struct base_tree_info *bases, FILE *file)
>        
>       -static const char *diff_title(struct strbuf *sb, int reroll_count,
>       -		       const char *generic, const char *rerolled)
>      -+static const char *diff_title(struct strbuf *sb, const char *reroll_count, int reroll_count_is_integer,
>      -+			const char*previous_count, const char *generic, const char *rerolled)
>      ++static const char *diff_title(struct strbuf *sb,
>      ++			const char *reroll_count_string,
>      ++			const char*previous_count_string,
>      ++			const char *generic, const char *rerolled)
>        {
>       -	if (reroll_count <= 0)
>      -+	if (!reroll_count || (!reroll_count_is_integer && !previous_count))
>      ++	if (!reroll_count_string || !previous_count_string)
>        		strbuf_addstr(sb, generic);
>       -	else /* RFC may be v0, so allow -v1 to diff against v0 */
>       -		strbuf_addf(sb, rerolled, reroll_count - 1);
>      -+	else if (reroll_count_is_integer)/* RFC may be v0, so allow -v1 to diff against v0 */
>      -+		strbuf_addf(sb, rerolled, atoi(reroll_count) - 1);
>      -+	else if (previous_count)
>      -+		strbuf_addf(sb, rerolled, previous_count);
>      ++	else if (previous_count_string)
>      ++		strbuf_addf(sb, rerolled, previous_count_string);
>        	return sb->buf;
>        }
>        
>       @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
>      - 	struct strbuf buf = STRBUF_INIT;
>        	int use_patch_format = 0;
>        	int quiet = 0;
>      --	int reroll_count = -1;
>      -+	int reroll_count_is_integer = 0;
>      -+	const char *reroll_count = NULL;
>      -+	const char *previous_count = NULL;
>      + 	int reroll_count = -1;
>      ++	const char *reroll_count_string = NULL;
>      ++	const char *previous_count_string = NULL;
>        	char *cover_from_description_arg = NULL;
>        	char *branch_name = NULL;
>        	char *base_commit = NULL;
>      @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
>        			    N_("start numbering patches at <n> instead of 1")),
>       -		OPT_INTEGER('v', "reroll-count", &reroll_count,
>       -			    N_("mark the series as Nth re-roll")),
>      -+		OPT_STRING('v', "reroll-count", &reroll_count, N_("reroll-count"),
>      ++		OPT_STRING('v', "reroll-count", &reroll_count_string, N_("reroll-count"),
>       +			    N_("mark the series as specified version re-roll")),
>      -+		OPT_STRING(0, "previous-count", &previous_count, N_("previous-count"),
>      -+			    N_("specified as the last version while we use --reroll-count")),
>        		OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
>        			    N_("max length of output filename")),
>        		OPT_CALLBACK_F(0, "rfc", &rev, NULL,
>       @@ builtin/log.c: 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 (0 < reroll_count) {
>      -+	if (previous_count && !reroll_count)
>      -+		usage(_("previous-count can only used when reroll-count is used"));
>      -+	if (reroll_count) {
>      ++	if (reroll_count_string) {
>        		struct strbuf sprefix = STRBUF_INIT;
>       -		strbuf_addf(&sprefix, "%s v%d",
>      -+		char ch;
>      -+		size_t i = 0 , reroll_count_len = strlen(reroll_count);
>      +-			    rev.subject_prefix, reroll_count);
>      +-		rev.reroll_count = reroll_count;
>      ++		char *endp;
>       +
>      -+		for (; i != reroll_count_len; i++) {
>      -+			ch = reroll_count[i];
>      -+			if(!isdigit(ch))
>      -+				break;
>      ++		reroll_count = strtoul(reroll_count_string, &endp, 10);
>      ++		if (!*endp && 0 < reroll_count) {
>      ++			previous_count_string = xstrfmt("%d", reroll_count - 1);
>       +		}
>      -+		reroll_count_is_integer = i == reroll_count_len ? 1 : 0;
>       +		strbuf_addf(&sprefix, "%s v%s",
>      - 			    rev.subject_prefix, reroll_count);
>      - 		rev.reroll_count = reroll_count;
>      ++			    rev.subject_prefix, reroll_count_string);
>      ++		rev.reroll_count = reroll_count_string;
>        		rev.subject_prefix = strbuf_detach(&sprefix, NULL);
>      + 	}
>      + 
>       @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
>      + 			die(_("--interdiff requires --cover-letter or single patch"));
>        		rev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];
>        		rev.idiff_oid2 = get_commit_tree_oid(list[0]);
>      - 		rev.idiff_title = diff_title(&idiff_title, reroll_count,
>      +-		rev.idiff_title = diff_title(&idiff_title, reroll_count,
>       -					     _("Interdiff:"),
>       -					     _("Interdiff against v%d:"));
>      -+			reroll_count_is_integer, previous_count, _("Interdiff:"),
>      -+				reroll_count_is_integer ? _("Interdiff against v%d:") :
>      ++		rev.idiff_title = diff_title(&idiff_title, reroll_count_string,
>      ++					previous_count_string,
>      ++					_("Interdiff:"),
>       +					_("Interdiff against v%s:"));
>        	}
>        
>        	if (creation_factor < 0)
>       @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
>      + 		rev.rdiff1 = rdiff1.buf;
>        		rev.rdiff2 = rdiff2.buf;
>        		rev.creation_factor = creation_factor;
>      - 		rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
>      +-		rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
>       -					     _("Range-diff:"),
>       -					     _("Range-diff against v%d:"));
>      -+			reroll_count_is_integer, previous_count, _("Range-diff:"),
>      -+				reroll_count_is_integer ? _("Range-diff against v%d:") :
>      ++		rev.rdiff_title = diff_title(&rdiff_title, reroll_count_string,
>      ++					previous_count_string,
>      ++					_("Range-diff:"),
>       +					_("Range-diff against v%s:"));
>        	}
>        
>      @@ t/t3206-range-diff.sh: test_expect_success 'format-patch --range-diff as comment
>        	grep "> 1: .* new message" 0001-*
>        '
>        
>      -+test_expect_success 'format-patch --range-diff reroll-count with a non-integer and previous-count ' '
>      -+	git format-patch --range-diff=HEAD~1 -v2.9 --previous-count=2.8 HEAD~1 >actual &&
>      ++test_expect_success 'format-patch --range-diff reroll-count with a non-integer' '
>      ++	git format-patch --range-diff=HEAD~1 -v2.9 HEAD~1 >actual &&
>       +	test_when_finished "rm v2.9-0001-*" &&
>       +	test_line_count = 1 actual &&
>      -+	test_i18ngrep "^Range-diff ..* v2.8:$" v2.9-0001-* &&
>      ++	test_i18ngrep "^Range-diff:$" v2.9-0001-* &&
>       +	grep "> 1: .* new message" v2.9-0001-*
>       +'
>       +
>      -+test_expect_success 'format-patch --range-diff reroll-count with a integer previous-count' '
>      -+	git format-patch --range-diff=HEAD~1 -v2 --previous-count=1.8 HEAD~1 >actual &&
>      ++test_expect_success 'format-patch --range-diff reroll-count with a integer' '
>      ++	git format-patch --range-diff=HEAD~1 -v2 HEAD~1 >actual &&
>       +	test_when_finished "rm v2-0001-*" &&
>       +	test_line_count = 1 actual &&
>       +	test_i18ngrep "^Range-diff ..* v1:$" v2-0001-* &&
>      @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: reroll-count' '
>       +	test_i18ngrep "^Interdiff:$" v2.2-0000-cover-letter.patch
>       +'
>       +
>      -+test_expect_success 'interdiff: reroll-count with a non-integer and previous-count ' '
>      -+	git format-patch --cover-letter --interdiff=boop~2 -v2.2 --previous-count=2.1 -1 boop &&
>      -+	test_i18ngrep "^Interdiff ..* v2.1:$" v2.2-0000-cover-letter.patch
>      -+'
>      -+
>      -+test_expect_success 'interdiff: reroll-count with a integer and previous-count ' '
>      -+	git format-patch --cover-letter --interdiff=boop~2 -v2 --previous-count=1.5 -1 boop &&
>      ++test_expect_success 'interdiff: reroll-count with a integer' '
>      ++	git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop &&
>       +	test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
>       +'
>      -+test_expect_success 'interdiff: previous-count without reroll-count ' '
>      -+	test_must_fail git format-patch --cover-letter --interdiff=boop~2 --previous-count=1.5 -1 boop
>      -+'
>      ++
>        test_expect_success 'interdiff: solo-patch' '
>        	cat >expect <<-\EOF &&
>        	  +fleep
> 
> 
>  Documentation/git-format-patch.txt |  1 +
>  builtin/log.c                      | 46 +++++++++++++++++++-----------
>  log-tree.c                         |  4 +--
>  revision.h                         |  2 +-
>  t/t3206-range-diff.sh              | 16 +++++++++++
>  t/t4014-format-patch.sh            | 26 +++++++++++++++++
>  6 files changed, 75 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 3e49bf221087..8af0d2923118 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -221,6 +221,7 @@ populated with placeholder text.
>  	`--subject-prefix` option) has ` v<n>` appended to it.  E.g.
>  	`--reroll-count=4` may produce `v4-0001-add-makefile.patch`
>  	file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
> +	now can support non-integrated version number like `-v1.1`.

Perhaps something like:

	+
	`<n>` can be any string, such as `-v1.1`.  In the case where it
	is a non-integral value, the "Range-diff" and "Interdiff"
	headers will not include the previous version.

>  --to=<email>::
>  	Add a `To:` header to the email headers. This is in addition
> diff --git a/builtin/log.c b/builtin/log.c
> index f67b67d80ed1..d135c30620b6 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1662,13 +1662,15 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
>  	oidclr(&bases->base_commit);
>  }
>  
> -static const char *diff_title(struct strbuf *sb, int reroll_count,
> -		       const char *generic, const char *rerolled)
> +static const char *diff_title(struct strbuf *sb,
> +			const char *reroll_count_string,
> +			const char*previous_count_string,
> +			const char *generic, const char *rerolled)
>  {
> -	if (reroll_count <= 0)
> +	if (!reroll_count_string || !previous_count_string)
>  		strbuf_addstr(sb, generic);
> -	else /* RFC may be v0, so allow -v1 to diff against v0 */
> -		strbuf_addf(sb, rerolled, reroll_count - 1);
> +	else if (previous_count_string)
> +		strbuf_addf(sb, rerolled, previous_count_string);
>  	return sb->buf;
>  }

I don't think it's necessary to do this at all. We can just leave
`reroll_count < 0` here.

> @@ -1718,6 +1720,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	int use_patch_format = 0;
>  	int quiet = 0;
>  	int reroll_count = -1;
> +	const char *reroll_count_string = NULL;
> +	const char *previous_count_string = NULL;
>  	char *cover_from_description_arg = NULL;
>  	char *branch_name = NULL;
>  	char *base_commit = NULL;
> @@ -1751,8 +1755,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			    N_("use <sfx> instead of '.patch'")),
>  		OPT_INTEGER(0, "start-number", &start_number,
>  			    N_("start numbering patches at <n> instead of 1")),
> -		OPT_INTEGER('v', "reroll-count", &reroll_count,
> -			    N_("mark the series as Nth re-roll")),
> +		OPT_STRING('v', "reroll-count", &reroll_count_string, N_("reroll-count"),
> +			    N_("mark the series as specified version re-roll")),

Others may disagree but I'm okay with leaving this as "Nth re-roll".
It's just a synopsis. More information can be found in the docs.

>  		OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
>  			    N_("max length of output filename")),
>  		OPT_CALLBACK_F(0, "rfc", &rev, NULL,
> @@ -1862,11 +1866,17 @@ 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 (0 < reroll_count) {
> +	if (reroll_count_string) {
>  		struct strbuf sprefix = STRBUF_INIT;
> -		strbuf_addf(&sprefix, "%s v%d",
> -			    rev.subject_prefix, reroll_count);
> -		rev.reroll_count = reroll_count;
> +		char *endp;
> +
> +		reroll_count = strtoul(reroll_count_string, &endp, 10);
> +		if (!*endp && 0 < reroll_count) {

This 0 < reroll_count check is unnecessary; it was initialised to -1 and
it hasn't changed since here.

Also, we can take advantage of the strtol_i() function, which can
perform bounds checking and error checking for us. This allows us to
assign reroll_count only when a valid integer is found so we can
eliminate previous_count_string().

Something like:

	strtol_i(reroll_count_string, 10, &reroll_count);

> +			previous_count_string = xstrfmt("%d", reroll_count - 1);
> +		}
> +		strbuf_addf(&sprefix, "%s v%s",
> +			    rev.subject_prefix, reroll_count_string);
> +		rev.reroll_count = reroll_count_string;
>  		rev.subject_prefix = strbuf_detach(&sprefix, NULL);
>  	}
>  
> @@ -2078,9 +2088,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			die(_("--interdiff requires --cover-letter or single patch"));
>  		rev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];
>  		rev.idiff_oid2 = get_commit_tree_oid(list[0]);
> -		rev.idiff_title = diff_title(&idiff_title, reroll_count,
> -					     _("Interdiff:"),
> -					     _("Interdiff against v%d:"));
> +		rev.idiff_title = diff_title(&idiff_title, reroll_count_string,
> +					previous_count_string,
> +					_("Interdiff:"),
> +					_("Interdiff against v%s:"));
>  	}
>  
>  	if (creation_factor < 0)
> @@ -2097,9 +2108,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		rev.rdiff1 = rdiff1.buf;
>  		rev.rdiff2 = rdiff2.buf;
>  		rev.creation_factor = creation_factor;
> -		rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
> -					     _("Range-diff:"),
> -					     _("Range-diff against v%d:"));
> +		rev.rdiff_title = diff_title(&rdiff_title, reroll_count_string,
> +					previous_count_string,
> +					_("Range-diff:"),
> +					_("Range-diff against v%s:"));
>  	}
>  
>  	if (!signature) {
> diff --git a/log-tree.c b/log-tree.c
> index 4531cebfab38..5f2e08ebcaab 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename,
>  	int start_len = filename->len;
>  	int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);
>  
> -	if (0 < info->reroll_count)
> -		strbuf_addf(filename, "v%d-", info->reroll_count);
> +	if (info->reroll_count)
> +		strbuf_addf(filename, "v%s-", info->reroll_count);
>  	strbuf_addf(filename, "%04d-%s", nr, subject);
>  
>  	if (max_len < filename->len)
> diff --git a/revision.h b/revision.h
> index e6be3c845e66..097d08354c61 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -235,7 +235,7 @@ struct rev_info {
>  	const char	*mime_boundary;
>  	const char	*patch_suffix;
>  	int		numbered_files;
> -	int		reroll_count;
> +	const char	*reroll_count;
>  	char		*message_id;
>  	struct ident_split from_ident;
>  	struct string_list *ref_message_ids;
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 1b26c4c2ef91..dc419c087e07 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -521,6 +521,22 @@ test_expect_success 'format-patch --range-diff as commentary' '
>  	grep "> 1: .* new message" 0001-*
>  '
>  
> +test_expect_success 'format-patch --range-diff reroll-count with a non-integer' '
> +	git format-patch --range-diff=HEAD~1 -v2.9 HEAD~1 >actual &&
> +	test_when_finished "rm v2.9-0001-*" &&
> +	test_line_count = 1 actual &&
> +	test_i18ngrep "^Range-diff:$" v2.9-0001-* &&
> +	grep "> 1: .* new message" v2.9-0001-*
> +'
> +
> +test_expect_success 'format-patch --range-diff reroll-count with a integer' '
> +	git format-patch --range-diff=HEAD~1 -v2 HEAD~1 >actual &&
> +	test_when_finished "rm v2-0001-*" &&
> +	test_line_count = 1 actual &&
> +	test_i18ngrep "^Range-diff ..* v1:$" v2-0001-* &&
> +	grep "> 1: .* new message" v2-0001-*
> +'
> +
>  test_expect_success 'range-diff overrides diff.noprefix internally' '
>  	git -c diff.noprefix=true range-diff HEAD^...
>  '
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 66630c8413d5..59dff38065ab 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -378,6 +378,14 @@ test_expect_success 'reroll count' '
>  	! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
>  '
>  
> +test_expect_success 'reroll count with a non-integer' '
> +	rm -fr patches &&
> +	git format-patch -o patches --cover-letter --reroll-count 4.4 main..side >list &&
> +	! grep -v "^patches/v4.4-000[0-3]-" list &&
> +	sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> +	! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> +'
> +
>  test_expect_success 'reroll count (-v)' '
>  	rm -fr patches &&
>  	git format-patch -o patches --cover-letter -v4 main..side >list &&
> @@ -386,6 +394,14 @@ test_expect_success 'reroll count (-v)' '
>  	! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
>  '
>  
> +test_expect_success 'reroll count (-v) with a non-integer' '
> +	rm -fr patches &&
> +	git format-patch -o patches --cover-letter -v4.4 main..side >list &&
> +	! grep -v "^patches/v4.4-000[0-3]-" list &&
> +	sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> +	! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> +'
> +
>  check_threading () {
>  	expect="$1" &&
>  	shift &&
> @@ -2255,6 +2271,16 @@ test_expect_success 'interdiff: reroll-count' '
>  	test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
>  '
>  
> +test_expect_success 'interdiff: reroll-count with a non-integer' '
> +	git format-patch --cover-letter --interdiff=boop~2 -v2.2 -1 boop &&
> +	test_i18ngrep "^Interdiff:$" v2.2-0000-cover-letter.patch
> +'
> +
> +test_expect_success 'interdiff: reroll-count with a integer' '
> +	git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop &&
> +	test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
> +'
> +
>  test_expect_success 'interdiff: solo-patch' '
>  	cat >expect <<-\EOF &&
>  	  +fleep
> 
> base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07
> -- 
> gitgitgadget

[gitgitgadget]

This patch series was integrated into seen via git@a05753f.

[gitgitgadget]

On the Git mailing list, ZheNing Hu wrote (reply to this):

Denton Liu <liu.denton@gmail.com> 于2021年3月4日周四 下午8:49写道：
>
> Hi ZheNing,
>
> On Thu, Mar 04, 2021 at 12:12:06PM +0000, ZheNing Hu via GitGitGadget wrote:
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Usually we can only use `format-patch -v<n>` to generate integral
> > version numbers patches, but sometimes a same fixup should be
> > labeled as a non-integral versions like `v1.1`, so teach `format-patch`
>
> s/versions/version/
>
> > to allow a non-integral versions may be helpful to send those patches.
>
> s/may be/which &/
>
> >
> > Since the original `format-patch` logic, if we specify a version `-v<n>`
> > and commbine with `--interdiff` or `--rangediff`, the patch will output
> > "Interdiff again v<n-1>:" or "Range-diff again v<n-1>:`, but this does
> > not meet the requirements of our fractional version numbers, so if the
> > user use a integral version number `-v<n>`, ensure that the output in
> > the patch is still `v<n-1>`; otherwise, only output "Interdiff" or
> > "Range-diff".
> >
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> >     [GSOC] format-patch: allow a non-integral version numbers
> >
> >     There is a small question: in the case of --reroll-count=<n>, "n" is an
> >     integer, we output "n-1" in the patch instead of "m" specified by
> >     --previous-count=<m>,Should we switch the priority of these two: let "m"
> >     output?
> >
> >     this want to fix #882 Thanks.
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-885%2Fadlternative%2Fformat_patch_non_intergral-v3
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v3
> > Pull-Request: https://github.com/gitgitgadget/git/pull/885
> >
> > Range-diff vs v2:
> >
> >  1:  800094cbf53b ! 1:  d4f38b78c464 format-patch: allow a non-integral version numbers
> >      @@ Commit message
> >
> >           Usually we can only use `format-patch -v<n>` to generate integral
> >           version numbers patches, but sometimes a same fixup should be
> >      -    laveled as a non-integral versions like `v1.1`,so teach format-patch
> >      -    allow a non-integral versions may be helpful to send those patches.
> >      +    labeled as a non-integral versions like `v1.1`, so teach `format-patch`
> >      +    to allow a non-integral versions may be helpful to send those patches.
> >
> >           Since the original `format-patch` logic, if we specify a version `-v<n>`
> >           and commbine with `--interdiff` or `--rangediff`, the patch will output
> >           "Interdiff again v<n-1>:" or "Range-diff again v<n-1>:`, but this does
> >      -    not meet the requirements of our fractional version numbers, so provide
> >      -    `format patch` a new option `--previous-count=<n>`, the patch can output
> >      -    user-specified previous version number. If the user use a integral version
> >      -    number `-v<n>`, ensure that the output in the patch is still `v<n-1>`.
> >      -    (let `--previous-count` become invalid.)
> >      +    not meet the requirements of our fractional version numbers, so if the
> >      +    user use a integral version number `-v<n>`, ensure that the output in
> >      +    the patch is still `v<n-1>`; otherwise, only output "Interdiff" or
> >      +    "Range-diff".
> >
> >           Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> >
> >        ## Documentation/git-format-patch.txt ##
> >      -@@ Documentation/git-format-patch.txt: SYNOPSIS
> >      -                   [--cover-from-description=<mode>]
> >      -                   [--rfc] [--subject-prefix=<subject prefix>]
> >      -                   [(--reroll-count|-v) <n>]
> >      -+                  [--previous-count=<n>]
> >      -                   [--to=<email>] [--cc=<email>]
> >      -                   [--[no-]cover-letter] [--quiet]
> >      -                   [--[no-]encode-email-headers]
> >       @@ Documentation/git-format-patch.txt: populated with placeholder text.
> >               `--subject-prefix` option) has ` v<n>` appended to it.  E.g.
> >               `--reroll-count=4` may produce `v4-0001-add-makefile.patch`
> >               file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
> >       +       now can support non-integrated version number like `-v1.1`.
> >      -+
> >      -+--previous-count=<n>::
> >      -+       Under the premise that we have used `--reroll-count=<n>`,
> >      -+       we can use `--previous-count=<n>` to specify the previous
> >      -+       version number. E.g. When we use the `--range-diff` or
> >      -+       `--interdiff` option and combine with `-v2.3 --previous-count=2.2`,
> >      -+       "Interdiff against v2.2:" or "Range-diff against v2.2:"
> >      -+       will be output in the patch.
> >
> >        --to=<email>::
> >               Add a `To:` header to the email headers. This is in addition
> >      @@ builtin/log.c: static void print_bases(struct base_tree_info *bases, FILE *file)
> >
> >       -static const char *diff_title(struct strbuf *sb, int reroll_count,
> >       -                      const char *generic, const char *rerolled)
> >      -+static const char *diff_title(struct strbuf *sb, const char *reroll_count, int reroll_count_is_integer,
> >      -+                       const char*previous_count, const char *generic, const char *rerolled)
> >      ++static const char *diff_title(struct strbuf *sb,
> >      ++                       const char *reroll_count_string,
> >      ++                       const char*previous_count_string,
> >      ++                       const char *generic, const char *rerolled)
> >        {
> >       -       if (reroll_count <= 0)
> >      -+       if (!reroll_count || (!reroll_count_is_integer && !previous_count))
> >      ++       if (!reroll_count_string || !previous_count_string)
> >                       strbuf_addstr(sb, generic);
> >       -       else /* RFC may be v0, so allow -v1 to diff against v0 */
> >       -               strbuf_addf(sb, rerolled, reroll_count - 1);
> >      -+       else if (reroll_count_is_integer)/* RFC may be v0, so allow -v1 to diff against v0 */
> >      -+               strbuf_addf(sb, rerolled, atoi(reroll_count) - 1);
> >      -+       else if (previous_count)
> >      -+               strbuf_addf(sb, rerolled, previous_count);
> >      ++       else if (previous_count_string)
> >      ++               strbuf_addf(sb, rerolled, previous_count_string);
> >               return sb->buf;
> >        }
> >
> >       @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >      -        struct strbuf buf = STRBUF_INIT;
> >               int use_patch_format = 0;
> >               int quiet = 0;
> >      --       int reroll_count = -1;
> >      -+       int reroll_count_is_integer = 0;
> >      -+       const char *reroll_count = NULL;
> >      -+       const char *previous_count = NULL;
> >      +        int reroll_count = -1;
> >      ++       const char *reroll_count_string = NULL;
> >      ++       const char *previous_count_string = NULL;
> >               char *cover_from_description_arg = NULL;
> >               char *branch_name = NULL;
> >               char *base_commit = NULL;
> >      @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
> >                                   N_("start numbering patches at <n> instead of 1")),
> >       -               OPT_INTEGER('v', "reroll-count", &reroll_count,
> >       -                           N_("mark the series as Nth re-roll")),
> >      -+               OPT_STRING('v', "reroll-count", &reroll_count, N_("reroll-count"),
> >      ++               OPT_STRING('v', "reroll-count", &reroll_count_string, N_("reroll-count"),
> >       +                           N_("mark the series as specified version re-roll")),
> >      -+               OPT_STRING(0, "previous-count", &previous_count, N_("previous-count"),
> >      -+                           N_("specified as the last version while we use --reroll-count")),
> >                       OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
> >                                   N_("max length of output filename")),
> >                       OPT_CALLBACK_F(0, "rfc", &rev, NULL,
> >       @@ builtin/log.c: 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 (0 < reroll_count) {
> >      -+       if (previous_count && !reroll_count)
> >      -+               usage(_("previous-count can only used when reroll-count is used"));
> >      -+       if (reroll_count) {
> >      ++       if (reroll_count_string) {
> >                       struct strbuf sprefix = STRBUF_INIT;
> >       -               strbuf_addf(&sprefix, "%s v%d",
> >      -+               char ch;
> >      -+               size_t i = 0 , reroll_count_len = strlen(reroll_count);
> >      +-                           rev.subject_prefix, reroll_count);
> >      +-               rev.reroll_count = reroll_count;
> >      ++               char *endp;
> >       +
> >      -+               for (; i != reroll_count_len; i++) {
> >      -+                       ch = reroll_count[i];
> >      -+                       if(!isdigit(ch))
> >      -+                               break;
> >      ++               reroll_count = strtoul(reroll_count_string, &endp, 10);
> >      ++               if (!*endp && 0 < reroll_count) {
> >      ++                       previous_count_string = xstrfmt("%d", reroll_count - 1);
> >       +               }
> >      -+               reroll_count_is_integer = i == reroll_count_len ? 1 : 0;
> >       +               strbuf_addf(&sprefix, "%s v%s",
> >      -                            rev.subject_prefix, reroll_count);
> >      -                rev.reroll_count = reroll_count;
> >      ++                           rev.subject_prefix, reroll_count_string);
> >      ++               rev.reroll_count = reroll_count_string;
> >                       rev.subject_prefix = strbuf_detach(&sprefix, NULL);
> >      +        }
> >      +
> >       @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >      +                        die(_("--interdiff requires --cover-letter or single patch"));
> >                       rev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];
> >                       rev.idiff_oid2 = get_commit_tree_oid(list[0]);
> >      -                rev.idiff_title = diff_title(&idiff_title, reroll_count,
> >      +-               rev.idiff_title = diff_title(&idiff_title, reroll_count,
> >       -                                            _("Interdiff:"),
> >       -                                            _("Interdiff against v%d:"));
> >      -+                       reroll_count_is_integer, previous_count, _("Interdiff:"),
> >      -+                               reroll_count_is_integer ? _("Interdiff against v%d:") :
> >      ++               rev.idiff_title = diff_title(&idiff_title, reroll_count_string,
> >      ++                                       previous_count_string,
> >      ++                                       _("Interdiff:"),
> >       +                                       _("Interdiff against v%s:"));
> >               }
> >
> >               if (creation_factor < 0)
> >       @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >      +                rev.rdiff1 = rdiff1.buf;
> >                       rev.rdiff2 = rdiff2.buf;
> >                       rev.creation_factor = creation_factor;
> >      -                rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
> >      +-               rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
> >       -                                            _("Range-diff:"),
> >       -                                            _("Range-diff against v%d:"));
> >      -+                       reroll_count_is_integer, previous_count, _("Range-diff:"),
> >      -+                               reroll_count_is_integer ? _("Range-diff against v%d:") :
> >      ++               rev.rdiff_title = diff_title(&rdiff_title, reroll_count_string,
> >      ++                                       previous_count_string,
> >      ++                                       _("Range-diff:"),
> >       +                                       _("Range-diff against v%s:"));
> >               }
> >
> >      @@ t/t3206-range-diff.sh: test_expect_success 'format-patch --range-diff as comment
> >               grep "> 1: .* new message" 0001-*
> >        '
> >
> >      -+test_expect_success 'format-patch --range-diff reroll-count with a non-integer and previous-count ' '
> >      -+       git format-patch --range-diff=HEAD~1 -v2.9 --previous-count=2.8 HEAD~1 >actual &&
> >      ++test_expect_success 'format-patch --range-diff reroll-count with a non-integer' '
> >      ++       git format-patch --range-diff=HEAD~1 -v2.9 HEAD~1 >actual &&
> >       +       test_when_finished "rm v2.9-0001-*" &&
> >       +       test_line_count = 1 actual &&
> >      -+       test_i18ngrep "^Range-diff ..* v2.8:$" v2.9-0001-* &&
> >      ++       test_i18ngrep "^Range-diff:$" v2.9-0001-* &&
> >       +       grep "> 1: .* new message" v2.9-0001-*
> >       +'
> >       +
> >      -+test_expect_success 'format-patch --range-diff reroll-count with a integer previous-count' '
> >      -+       git format-patch --range-diff=HEAD~1 -v2 --previous-count=1.8 HEAD~1 >actual &&
> >      ++test_expect_success 'format-patch --range-diff reroll-count with a integer' '
> >      ++       git format-patch --range-diff=HEAD~1 -v2 HEAD~1 >actual &&
> >       +       test_when_finished "rm v2-0001-*" &&
> >       +       test_line_count = 1 actual &&
> >       +       test_i18ngrep "^Range-diff ..* v1:$" v2-0001-* &&
> >      @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: reroll-count' '
> >       +       test_i18ngrep "^Interdiff:$" v2.2-0000-cover-letter.patch
> >       +'
> >       +
> >      -+test_expect_success 'interdiff: reroll-count with a non-integer and previous-count ' '
> >      -+       git format-patch --cover-letter --interdiff=boop~2 -v2.2 --previous-count=2.1 -1 boop &&
> >      -+       test_i18ngrep "^Interdiff ..* v2.1:$" v2.2-0000-cover-letter.patch
> >      -+'
> >      -+
> >      -+test_expect_success 'interdiff: reroll-count with a integer and previous-count ' '
> >      -+       git format-patch --cover-letter --interdiff=boop~2 -v2 --previous-count=1.5 -1 boop &&
> >      ++test_expect_success 'interdiff: reroll-count with a integer' '
> >      ++       git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop &&
> >       +       test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
> >       +'
> >      -+test_expect_success 'interdiff: previous-count without reroll-count ' '
> >      -+       test_must_fail git format-patch --cover-letter --interdiff=boop~2 --previous-count=1.5 -1 boop
> >      -+'
> >      ++
> >        test_expect_success 'interdiff: solo-patch' '
> >               cat >expect <<-\EOF &&
> >                 +fleep
> >
> >
> >  Documentation/git-format-patch.txt |  1 +
> >  builtin/log.c                      | 46 +++++++++++++++++++-----------
> >  log-tree.c                         |  4 +--
> >  revision.h                         |  2 +-
> >  t/t3206-range-diff.sh              | 16 +++++++++++
> >  t/t4014-format-patch.sh            | 26 +++++++++++++++++
> >  6 files changed, 75 insertions(+), 20 deletions(-)
> >
> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> > index 3e49bf221087..8af0d2923118 100644
> > --- a/Documentation/git-format-patch.txt
> > +++ b/Documentation/git-format-patch.txt
> > @@ -221,6 +221,7 @@ populated with placeholder text.
> >       `--subject-prefix` option) has ` v<n>` appended to it.  E.g.
> >       `--reroll-count=4` may produce `v4-0001-add-makefile.patch`
> >       file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
> > +     now can support non-integrated version number like `-v1.1`.
>
> Perhaps something like:
>
>         +
>         `<n>` can be any string, such as `-v1.1`.  In the case where it
>         is a non-integral value, the "Range-diff" and "Interdiff"
>         headers will not include the previous version.
>
> >  --to=<email>::
> >       Add a `To:` header to the email headers. This is in addition
> > diff --git a/builtin/log.c b/builtin/log.c
> > index f67b67d80ed1..d135c30620b6 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -1662,13 +1662,15 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
> >       oidclr(&bases->base_commit);
> >  }
> >
> > -static const char *diff_title(struct strbuf *sb, int reroll_count,
> > -                    const char *generic, const char *rerolled)
> > +static const char *diff_title(struct strbuf *sb,
> > +                     const char *reroll_count_string,
> > +                     const char*previous_count_string,
> > +                     const char *generic, const char *rerolled)
> >  {
> > -     if (reroll_count <= 0)
> > +     if (!reroll_count_string || !previous_count_string)
> >               strbuf_addstr(sb, generic);
> > -     else /* RFC may be v0, so allow -v1 to diff against v0 */
> > -             strbuf_addf(sb, rerolled, reroll_count - 1);
> > +     else if (previous_count_string)
> > +             strbuf_addf(sb, rerolled, previous_count_string);
> >       return sb->buf;
> >  }
>
> I don't think it's necessary to do this at all. We can just leave
> `reroll_count < 0` here.
Truly.
>
> > @@ -1718,6 +1720,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >       int use_patch_format = 0;
> >       int quiet = 0;
> >       int reroll_count = -1;
> > +     const char *reroll_count_string = NULL;
> > +     const char *previous_count_string = NULL;
> >       char *cover_from_description_arg = NULL;
> >       char *branch_name = NULL;
> >       char *base_commit = NULL;
> > @@ -1751,8 +1755,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >                           N_("use <sfx> instead of '.patch'")),
> >               OPT_INTEGER(0, "start-number", &start_number,
> >                           N_("start numbering patches at <n> instead of 1")),
> > -             OPT_INTEGER('v', "reroll-count", &reroll_count,
> > -                         N_("mark the series as Nth re-roll")),
> > +             OPT_STRING('v', "reroll-count", &reroll_count_string, N_("reroll-count"),
> > +                         N_("mark the series as specified version re-roll")),
>
> Others may disagree but I'm okay with leaving this as "Nth re-roll".
> It's just a synopsis. More information can be found in the docs.
Does Nth feel that reroll_count must be an integer? If you think it is
possible, I can restore it to the original state.
>
> >               OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max,
> >                           N_("max length of output filename")),
> >               OPT_CALLBACK_F(0, "rfc", &rev, NULL,
> > @@ -1862,11 +1866,17 @@ 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 (0 < reroll_count) {
> > +     if (reroll_count_string) {
> >               struct strbuf sprefix = STRBUF_INIT;
> > -             strbuf_addf(&sprefix, "%s v%d",
> > -                         rev.subject_prefix, reroll_count);
> > -             rev.reroll_count = reroll_count;
> > +             char *endp;
> > +
> > +             reroll_count = strtoul(reroll_count_string, &endp, 10);
> > +             if (!*endp && 0 < reroll_count) {
>
> This 0 < reroll_count check is unnecessary; it was initialised to -1 and
> it hasn't changed since here.
>
> Also, we can take advantage of the strtol_i() function, which can
> perform bounds checking and error checking for us. This allows us to
> assign reroll_count only when a valid integer is found so we can
> eliminate previous_count_string().
>
> Something like:
>
>         strtol_i(reroll_count_string, 10, &reroll_count);
>
My code does make the program a lot more complicated,
and your advice is good.
> > +                     previous_count_string = xstrfmt("%d", reroll_count - 1);
> > +             }
> > +             strbuf_addf(&sprefix, "%s v%s",
> > +                         rev.subject_prefix, reroll_count_string);
> > +             rev.reroll_count = reroll_count_string;
> >               rev.subject_prefix = strbuf_detach(&sprefix, NULL);
> >       }
> >
> > @@ -2078,9 +2088,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >                       die(_("--interdiff requires --cover-letter or single patch"));
> >               rev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];
> >               rev.idiff_oid2 = get_commit_tree_oid(list[0]);
> > -             rev.idiff_title = diff_title(&idiff_title, reroll_count,
> > -                                          _("Interdiff:"),
> > -                                          _("Interdiff against v%d:"));
> > +             rev.idiff_title = diff_title(&idiff_title, reroll_count_string,
> > +                                     previous_count_string,
> > +                                     _("Interdiff:"),
> > +                                     _("Interdiff against v%s:"));
> >       }
> >
> >       if (creation_factor < 0)
> > @@ -2097,9 +2108,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >               rev.rdiff1 = rdiff1.buf;
> >               rev.rdiff2 = rdiff2.buf;
> >               rev.creation_factor = creation_factor;
> > -             rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
> > -                                          _("Range-diff:"),
> > -                                          _("Range-diff against v%d:"));
> > +             rev.rdiff_title = diff_title(&rdiff_title, reroll_count_string,
> > +                                     previous_count_string,
> > +                                     _("Range-diff:"),
> > +                                     _("Range-diff against v%s:"));
> >       }
> >
> >       if (!signature) {
> > diff --git a/log-tree.c b/log-tree.c
> > index 4531cebfab38..5f2e08ebcaab 100644
> > --- a/log-tree.c
> > +++ b/log-tree.c
> > @@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename,
> >       int start_len = filename->len;
> >       int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);
> >
> > -     if (0 < info->reroll_count)
> > -             strbuf_addf(filename, "v%d-", info->reroll_count);
> > +     if (info->reroll_count)
> > +             strbuf_addf(filename, "v%s-", info->reroll_count);
> >       strbuf_addf(filename, "%04d-%s", nr, subject);
> >
> >       if (max_len < filename->len)
> > diff --git a/revision.h b/revision.h
> > index e6be3c845e66..097d08354c61 100644
> > --- a/revision.h
> > +++ b/revision.h
> > @@ -235,7 +235,7 @@ struct rev_info {
> >       const char      *mime_boundary;
> >       const char      *patch_suffix;
> >       int             numbered_files;
> > -     int             reroll_count;
> > +     const char      *reroll_count;
> >       char            *message_id;
> >       struct ident_split from_ident;
> >       struct string_list *ref_message_ids;
> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> > index 1b26c4c2ef91..dc419c087e07 100755
> > --- a/t/t3206-range-diff.sh
> > +++ b/t/t3206-range-diff.sh
> > @@ -521,6 +521,22 @@ test_expect_success 'format-patch --range-diff as commentary' '
> >       grep "> 1: .* new message" 0001-*
> >  '
> >
> > +test_expect_success 'format-patch --range-diff reroll-count with a non-integer' '
> > +     git format-patch --range-diff=HEAD~1 -v2.9 HEAD~1 >actual &&
> > +     test_when_finished "rm v2.9-0001-*" &&
> > +     test_line_count = 1 actual &&
> > +     test_i18ngrep "^Range-diff:$" v2.9-0001-* &&
> > +     grep "> 1: .* new message" v2.9-0001-*
> > +'
> > +
> > +test_expect_success 'format-patch --range-diff reroll-count with a integer' '
> > +     git format-patch --range-diff=HEAD~1 -v2 HEAD~1 >actual &&
> > +     test_when_finished "rm v2-0001-*" &&
> > +     test_line_count = 1 actual &&
> > +     test_i18ngrep "^Range-diff ..* v1:$" v2-0001-* &&
> > +     grep "> 1: .* new message" v2-0001-*
> > +'
> > +
> >  test_expect_success 'range-diff overrides diff.noprefix internally' '
> >       git -c diff.noprefix=true range-diff HEAD^...
> >  '
> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index 66630c8413d5..59dff38065ab 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -378,6 +378,14 @@ test_expect_success 'reroll count' '
> >       ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
> >  '
> >
> > +test_expect_success 'reroll count with a non-integer' '
> > +     rm -fr patches &&
> > +     git format-patch -o patches --cover-letter --reroll-count 4.4 main..side >list &&
> > +     ! grep -v "^patches/v4.4-000[0-3]-" list &&
> > +     sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> > +     ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> > +'
> > +
> >  test_expect_success 'reroll count (-v)' '
> >       rm -fr patches &&
> >       git format-patch -o patches --cover-letter -v4 main..side >list &&
> > @@ -386,6 +394,14 @@ test_expect_success 'reroll count (-v)' '
> >       ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
> >  '
> >
> > +test_expect_success 'reroll count (-v) with a non-integer' '
> > +     rm -fr patches &&
> > +     git format-patch -o patches --cover-letter -v4.4 main..side >list &&
> > +     ! grep -v "^patches/v4.4-000[0-3]-" list &&
> > +     sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> > +     ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> > +'
> > +
> >  check_threading () {
> >       expect="$1" &&
> >       shift &&
> > @@ -2255,6 +2271,16 @@ test_expect_success 'interdiff: reroll-count' '
> >       test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
> >  '
> >
> > +test_expect_success 'interdiff: reroll-count with a non-integer' '
> > +     git format-patch --cover-letter --interdiff=boop~2 -v2.2 -1 boop &&
> > +     test_i18ngrep "^Interdiff:$" v2.2-0000-cover-letter.patch
> > +'
> > +
> > +test_expect_success 'interdiff: reroll-count with a integer' '
> > +     git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop &&
> > +     test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
> > +'
> > +
> >  test_expect_success 'interdiff: solo-patch' '
> >       cat >expect <<-\EOF &&
> >         +fleep
> >
> > base-commit: 966e671106b2fd38301e7c344c754fd118d0bb07
> > --
> > gitgitgadget

[gitgitgadget]

This patch series was integrated into seen via git@8b29bfa.

[gitgitgadget]

This patch series was integrated into seen via git@d7292b4.

[gitgitgadget]

There was a status update about the branch zh/format-patch-fractional-reroll-count on the Git mailing list:

"git format-patch -v<n>" learned to allow a reroll count that is
not an integer.

Will merge to 'next'.

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Sun, Mar 21, 2021 at 5:00 AM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> [...]
> Allow `format-patch` to take such a non-integral iteration
> number.
> [...]
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>

Just a few nits below; nothing very important (except perhaps the
final comment about the potential for people to get confused while
reading the tests). Junio already has this marked as ready to merge to
"next", so these nits may not be worth a re-roll.

> diff --git a/log-tree.c b/log-tree.c
> @@ -368,9 +368,14 @@ void fmt_output_subject(struct strbuf *filename,
>         int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);
> +       struct strbuf temp = STRBUF_INIT;
>
> +       if (info->reroll_count) {
> +               strbuf_addf(&temp, "v%s", info->reroll_count);
> +               format_sanitized_subject(filename, temp.buf, temp.len);
> +               strbuf_addstr(filename, "-");
> +               strbuf_release(&temp);
> +       }

The new `temp` strbuf is use only inside the conditional, so it
could/should have been declared in that block rather than in the outer
block:

    if (info->reroll_count) {
        struct strbuf temp = STRBUF_INIT;

        strbuf_addf(&temp, "v%s", info->reroll_count);
        ...
    }

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> @@ -378,6 +378,22 @@ test_expect_success 'reroll count' '
> +test_expect_success 'reroll count with a fractional number' '
> +       rm -fr patches &&
> +       git format-patch -o patches --cover-letter --reroll-count 4.4 main..side >list &&
> +       ! grep -v "^patches/v4.4-000[0-3]-" list &&
> +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> +       ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> +'
> +
> +test_expect_success 'reroll count with a non number' '
> +       rm -fr patches &&
> +       git format-patch -o patches --cover-letter --reroll-count 4rev2 main..side >list &&
> +       ! grep -v "^patches/v4rev2-000[0-3]-" list &&
> +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> +       ! grep -v "^Subject: \[PATCH v4rev2 [0-3]/3\] " subjects
> +'

The above two tests...

> @@ -386,6 +402,38 @@ test_expect_success 'reroll count (-v)' '
> +test_expect_success 'reroll count (-v) with a fractional number' '
> +       rm -fr patches &&
> +       git format-patch -o patches --cover-letter -v4.4 main..side >list &&
> +       ! grep -v "^patches/v4.4-000[0-3]-" list &&
> +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> +       ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> +'
> +
> +test_expect_success 'reroll (-v) count with a non number' '
> +       rm -fr patches &&
> +       git format-patch -o patches --cover-letter -v4rev2 main..side >list &&
> +       ! grep -v "^patches/v4rev2-000[0-3]-" list &&
> +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> +       ! grep -v "^Subject: \[PATCH v4rev2 [0-3]/3\] " subjects
> +'

... are repeated here with the only difference being `--reroll-count`
versus `-v`. Since other tests have already established that
`--reroll-count` and `-v` are identical, it's not really necessary to
do that work again with these duplicate tests.

> +test_expect_success 'reroll (-v) count with a "injection (1)"' '
> +       rm -fr patches &&
> +       git format-patch -o patches --cover-letter -v4..././../1/.2//  main..side >list &&
> +       ! grep -v "^patches/v4.-.-.-1-.2-000[0-3]-" list &&
> +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> +       ! grep -v "^Subject: \[PATCH v4..././../1/.2// [0-3]/3\] " subjects
> +'

A couple comments:

The test title might be easier for other people to understand if it
says "non-pathname character" or "non filename character" rather than
"injection".

Note that the `grep -v` is casting a wider net than it seems at first
glance. The `.` matches any character, not just a period ".". To
tighten the matching and make `.` match just a ".", you can use `grep
-vF`.

> +test_expect_success 'reroll (-v) count with a "injection (2)"' '
> +       rm -fr patches &&
> +       git format-patch -o patches --cover-letter -v4-----//1//--.--  main..side >list &&
> +       ! grep -v "^patches/v4-1-000[0-3]-" list &&
> +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> +       ! grep -v "^Subject: \[PATCH v4-----//1//--.-- [0-3]/3\] " subjects
> +'

Presumably the coverage of format_sanitized_subject() is already being
tested elsewhere, so it's not clear that this second "injection" test
adds any value over the first test. Moreover, this second test can
confuse readers into thinking that it is testing something that the
first test didn't cover, but that isn't the case (as far as I can
tell).

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Mar 21, 2021 at 5:00 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> [...]
>> Allow `format-patch` to take such a non-integral iteration
>> number.
>> [...]
>> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
>
> Just a few nits below; nothing very important (except perhaps the
> final comment about the potential for people to get confused while
> reading the tests). Junio already has this marked as ready to merge to
> "next", so these nits may not be worth a re-roll.

Oh, we just rebuilt 'next' and anything not in 'next' is not too
late to reroll.  Your comments on the test part were all sensible.

Thanks.

[gitgitgadget]

On the Git mailing list, ZheNing Hu wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> 于2021年3月23日周二 下午1:31写道：
>
> On Sun, Mar 21, 2021 at 5:00 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > [...]
> > Allow `format-patch` to take such a non-integral iteration
> > number.
> > [...]
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
>
> Just a few nits below; nothing very important (except perhaps the
> final comment about the potential for people to get confused while
> reading the tests). Junio already has this marked as ready to merge to
> "next", so these nits may not be worth a re-roll.
>

Thanks, Eric, these suggestions are worth considering.

> > diff --git a/log-tree.c b/log-tree.c
> > @@ -368,9 +368,14 @@ void fmt_output_subject(struct strbuf *filename,
> >         int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);
> > +       struct strbuf temp = STRBUF_INIT;
> >
> > +       if (info->reroll_count) {
> > +               strbuf_addf(&temp, "v%s", info->reroll_count);
> > +               format_sanitized_subject(filename, temp.buf, temp.len);
> > +               strbuf_addstr(filename, "-");
> > +               strbuf_release(&temp);
> > +       }
>
> The new `temp` strbuf is use only inside the conditional, so it
> could/should have been declared in that block rather than in the outer
> block:
>

Sometimes I always forget this: which variables need to be placed locally,
It seems that I have to be more careful.

>     if (info->reroll_count) {
>         struct strbuf temp = STRBUF_INIT;
>
>         strbuf_addf(&temp, "v%s", info->reroll_count);
>         ...
>     }
>
> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > @@ -378,6 +378,22 @@ test_expect_success 'reroll count' '
> > +test_expect_success 'reroll count with a fractional number' '
> > +       rm -fr patches &&
> > +       git format-patch -o patches --cover-letter --reroll-count 4.4 main..side >list &&
> > +       ! grep -v "^patches/v4.4-000[0-3]-" list &&
> > +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> > +       ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> > +'
> > +
> > +test_expect_success 'reroll count with a non number' '
> > +       rm -fr patches &&
> > +       git format-patch -o patches --cover-letter --reroll-count 4rev2 main..side >list &&
> > +       ! grep -v "^patches/v4rev2-000[0-3]-" list &&
> > +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> > +       ! grep -v "^Subject: \[PATCH v4rev2 [0-3]/3\] " subjects
> > +'
>
> The above two tests...
>
> > @@ -386,6 +402,38 @@ test_expect_success 'reroll count (-v)' '
> > +test_expect_success 'reroll count (-v) with a fractional number' '
> > +       rm -fr patches &&
> > +       git format-patch -o patches --cover-letter -v4.4 main..side >list &&
> > +       ! grep -v "^patches/v4.4-000[0-3]-" list &&
> > +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> > +       ! grep -v "^Subject: \[PATCH v4.4 [0-3]/3\] " subjects
> > +'
> > +
> > +test_expect_success 'reroll (-v) count with a non number' '
> > +       rm -fr patches &&
> > +       git format-patch -o patches --cover-letter -v4rev2 main..side >list &&
> > +       ! grep -v "^patches/v4rev2-000[0-3]-" list &&
> > +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> > +       ! grep -v "^Subject: \[PATCH v4rev2 [0-3]/3\] " subjects
> > +'
>
> ... are repeated here with the only difference being `--reroll-count`
> versus `-v`. Since other tests have already established that
> `--reroll-count` and `-v` are identical, it's not really necessary to
> do that work again with these duplicate tests.
>

Makes sense.

> > +test_expect_success 'reroll (-v) count with a "injection (1)"' '
> > +       rm -fr patches &&
> > +       git format-patch -o patches --cover-letter -v4..././../1/.2//  main..side >list &&
> > +       ! grep -v "^patches/v4.-.-.-1-.2-000[0-3]-" list &&
> > +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> > +       ! grep -v "^Subject: \[PATCH v4..././../1/.2// [0-3]/3\] " subjects
> > +'
>
> A couple comments:
>
> The test title might be easier for other people to understand if it
> says "non-pathname character" or "non filename character" rather than
> "injection".
>
> Note that the `grep -v` is casting a wider net than it seems at first
> glance. The `.` matches any character, not just a period ".". To
> tighten the matching and make `.` match just a ".", you can use `grep
> -vF`.
>

Yes, `grep -vF` is very important for the correctness of the test.

> > +test_expect_success 'reroll (-v) count with a "injection (2)"' '
> > +       rm -fr patches &&
> > +       git format-patch -o patches --cover-letter -v4-----//1//--.--  main..side >list &&
> > +       ! grep -v "^patches/v4-1-000[0-3]-" list &&
> > +       sed -n -e "/^Subject: /p" $(cat list) >subjects &&
> > +       ! grep -v "^Subject: \[PATCH v4-----//1//--.-- [0-3]/3\] " subjects
> > +'
>
> Presumably the coverage of format_sanitized_subject() is already being
> tested elsewhere, so it's not clear that this second "injection" test
> adds any value over the first test. Moreover, this second test can
> confuse readers into thinking that it is testing something that the
> first test didn't cover, but that isn't the case (as far as I can
> tell).

The second test I just want to test character `-`, but now I think it can
merge to first test.

Thanks.

[gitgitgadget]

On the Git mailing list, ZheNing Hu wrote (reply to this):

> > Note that the `grep -v` is casting a wider net than it seems at first
> > glance. The `.` matches any character, not just a period ".". To
> > tighten the matching and make `.` match just a ".", you can use `grep
> > -vF`.
> >
>
> Yes, `grep -vF` is very important for the correctness of the test.
>
Correct it, I still have to use `grep -v`, but I need to change the
'.' to `\.` .

[adlternative]

/submit

[gitgitgadget]

Submitted as pull.885.v10.git.1616497946427.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-885/adlternative/format_patch_non_intergral-v10

To fetch this version to local tag pr-885/adlternative/format_patch_non_intergral-v10:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-885/adlternative/format_patch_non_intergral-v10

[gitgitgadget]

This patch series was integrated into seen via git@d6fc2ad.

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Tue, Mar 23, 2021 at 7:12 AM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> [...]
> Allow `format-patch` to take such a non-integral iteration
> number.
> [...]
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>     [GSOC] format-patch: allow a non-integral version numbers
>     A rollback was performed again under Eric's suggestion.

Thanks. I think this version addresses my previous review comments.

I did not find anything to comment about in this version.

[gitgitgadget]

On the Git mailing list, ZheNing Hu wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> 于2021年3月24日周三 上午11:59写道：
>
> On Tue, Mar 23, 2021 at 7:12 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > [...]
> > Allow `format-patch` to take such a non-integral iteration
> > number.
> > [...]
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> >     [GSOC] format-patch: allow a non-integral version numbers
> >     A rollback was performed again under Eric's suggestion.
>
> Thanks. I think this version addresses my previous review comments.
>
> I did not find anything to comment about in this version.

Eric, Thanks for all your comments and guidance! :)

--
ZheNing Hu

[gitgitgadget]

On the Git mailing list, Denton Liu wrote (reply to this):

On Sun, Mar 21, 2021 at 01:54:15AM -0400, Eric Sunshine wrote:
> On Sun, Mar 21, 2021 at 1:45 AM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > To protect against that problem, you may need to call
> > > format_sanitized_subject() manually after formatting "v%s-". (I'm just
> > > looking at this code for the first time, so I could be hopelessly
> > > wrong. There may be a better way to fix it.)
> >
> > Yes, slash is of course very problematic, but what we've been doing
> > to the patch filenames was to ensure that they will be free of $IFS
> > whitespaces and shell glob special characters as well, and we should
> > treat the "reroll count" just like the other end-user controlled
> > input, i.e. the title of the patch, and sanitize it the same way.
> >
> > So I am pretty sure format_sanitized_subject() is the right way to
> > go.
> 
> The pathname sanitization would also deserve a test.
> 
> Denton's seemingly simple feature request[1] has turned out to be
> quite a little project.

Sorry I've been quite busy the past couple of weeks so I haven't had the
bandwidth to review the patches as they've come up. Thanks for
implementing my feature request, ZheNing. And thanks for the careful
reviews, Eric.

> [1]: https://github.com/gitgitgadget/git/issues/882

[gitgitgadget]

This patch series was integrated into seen via git@77ee6f5.

[gitgitgadget]

This patch series was integrated into seen via git@e39b602.

[gitgitgadget]

This patch series was integrated into next via git@3c29ec9.

[gitgitgadget]

This patch series was integrated into seen via git@812f03f.

[gitgitgadget]

There was a status update about the branch zh/format-patch-fractional-reroll-count on the Git mailing list:

"git format-patch -v<n>" learned to allow a reroll count that is
not an integer.

Will merge to 'master'.

[gitgitgadget]

This patch series was integrated into seen via git@e7d870c.

[gitgitgadget]

This patch series was integrated into seen via git@8a4394d.

[gitgitgadget]

This patch series was integrated into next via git@8a4394d.

[gitgitgadget]

This patch series was integrated into master via git@8a4394d.

[gitgitgadget]

Closed via 8a4394d.