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

[ZheNing Hu <adlternative@gmail.com>]

Eric Sunshine <sunshine@sunshineco.com> 于2021年3月16日周二 上午7:41写道：
>
> 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.
>
I agree with you.
> > 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).

Well, The reason `strtol_i` does not check the return value is that
`strtol_i` will
only modify the value of `reroll_count` if the `reroll_count_string`
we provide is
 an integer string, so if `reroll_count_string` is not an integer string, then
`reroll_count` will remain -1, and then `diff_title` will only execute

        if (reroll_count <= 0)
                strbuf_addstr(sb, generic);

So don't need check strtol_i return value. But what you said to put
`strtol_i` in
`diff_title` is indeed a good idea. Of course, we need to modify the declaration
 and parameters of `diff_title`.