Re: [PATCH v4] format-patch: allow a non-integral version numbers

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Mar 5, 2021 at 2:10 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 sometimes a same fixup should be
> labeled as a non-integral version like `v1.1`, so teach `format-patch`
> to allow a non-integral version which may be helpful to send those
> patches.
>
> `<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.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> @@ -221,6 +221,7 @@ populated with placeholder text.
>         `--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`.

Let's drop "now" from the beginning of the sentence since it is only
meaningful for people who have read this documentation previously, but
not for people newly learning about the option. Perhaps just say:

    `<n>` may be a fractional number.

> diff --git a/builtin/log.c b/builtin/log.c
> @@ -1862,11 +1863,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> -       if (0 < reroll_count) {
> +       if (reroll_count_string) {
>                 struct strbuf sprefix = STRBUF_INIT;
> +
> +               strtol_i(reroll_count_string, 10, &reroll_count);
> +               strbuf_addf(&sprefix, "%s v%s",
> +                           rev.subject_prefix, reroll_count_string);
> +               rev.reroll_count = reroll_count_string;

This code can be confusing to readers since it appears to ignore the
result of strtol_i(), and it's difficult for the reader to understand
the difference between `reroll_count` and `reroll_count_string` and
why you need both variables. I was going to suggest that you write an
/* in-code comment */ here explaining why you don't care if the reroll
count parsed correctly as a number. However, now that I'm examining
the code again, I think it would be clearer if you move the strtol_i()
call into the diff_title() function since -- following your changes --
that function is the only code which cares whether `reroll_count` can
be parsed as a number (the rest of the code, after your change, just
uses it as a string).