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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 15, 2021, 23:41 UTC
Message-ID: <CAPig+cSd-e76bOQY8yoZnRDbuQbZ_FNHJ5TV6BC+dAofPmk7DQ@mail.gmail.com>
In-Reply-To: <pull.885.v4.git.1614928211635.gitgitgadget@gmail.com>

On Fri, Mar 5, 2021 at 2:10 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 17 quoted lines

> 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.

Show 10 quoted lines

> 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).