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

Eric Sunshine [off-list ref] 于2021年3月17日周三 上午7:36写道：

On Tue, Mar 16, 2021 at 4:25 AM ZheNing Hu via GitGitGadget
[off-list ref] wrote:

quoted

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

diff --git a/builtin/log.c b/builtin/log.c

@@ -1662,13 +1662,19 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
+static const char *diff_title(struct strbuf *sb,
+                             const char *reroll_count,
+                             const char *generic,
+                             const char *rerolled)
 {
+       int reroll_count_int = -1;
+
+       if (reroll_count)
+               strtol_i(reroll_count, 10, &reroll_count_int);
+       if (reroll_count_int <= 0)
                strbuf_addstr(sb, generic);
        else /* RFC may be v0, so allow -v1 to diff against v0 */
+               strbuf_addf(sb, rerolled, reroll_count_int - 1);
        return sb->buf;
 }

Thanks. The logic of this version is much easier to understand now
that the number parsing has been moved into diff_title().

It may still be a bit confusing for someone reading this code to
understand why you don't check the return value of strtol_i().
Therefore, it might be a good idea to add an /* in-code comment */
explaining why you don't check whether the parse succeeded or failed.
However, if we rewrite the code like this:

    int v;
    if (reroll_count && !strtol_i(reroll_count, 10, &v))
        strbuf_addf(sb, rerolled, v - 1);
    else
        strbuf_addstr(sb, generic);
    return sb->buf;

then the logic becomes obvious, and we don't even need a comment.
(Notice that I also shortened the variable name since the code is just
as clear with a short name as with a long spelled out name such as
"reroll_count_int".)

Yes, It is better to handle the return value of `strtol_i` in an if judgment and
use `v` instead of `reroll_count_int`.

Thanks.