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

[Eric Sunshine <sunshine@sunshineco.com>]

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