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

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

As the patch text itself (assuming that the vague "compared with the
previous iteration" is acceptable) is getting solid, let's nitpick
the proposed log message so that we can start considering a merge to
'next'.

I won't review the t/ part of the patch as I know others on CC are
better at reviewing the tests than I am ;-)

From: ZheNing Hu <redacted>

Usually we can only use `format-patch -v<n>` to generate integral
version numbers patches,

The "only" sounds as if you are saying that "there is no tool other
than 'format-patch -v' we can use", which might be technically true,
but is not what you want to stress to your readers here.  You are
sayig that usually people only use integral version numbers.

but sometimes a small 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.

Also, I do not think we want to encourage such use of fractional
iteration numbers.  We can merely enable, without saying it is a
good idea or a bad idea, leaving judgment to each project that may
or may not accept a patch versioned in such a way.  So avoid "should
be".

That makes the first part of the log message to be something like:

    The `-v<n>` option of `format-patch` can give nothing but an
    integral iteration number to patches in a series.  Some people,
    however, prefer to mark a new iteration with only a small fixup
    with a non integral iteration number (e.g. an "oops, that was
    wrong" fix-up patch for v4 iteration may be labeled as "v4.1").

    Allow `format-patch` to take such a non-integral iteration
    number.

`<n>` can be any string, such as '3.1' or '4rev2'. In the case
where it is a non-integral value, the "Range-diff" and "Interdiff"
headers will not include the previous version.

This part is well written and can stay as-is.

Signed-off-by: ZheNing Hu <redacted>
---
    [GSOC] format-patch: allow a non-integral version numbers
    
    There is a small question: in the case of --reroll-count=<n>, "n" is an
    integer, we output "n-1" in the patch instead of "m" specified by
    --previous-count=<m>,Should we switch the priority of these two: let "m"
    output?

Didn't I answer this question already?

Is it on purpose that `<n>` here has an extra SP before it?

I am not sure it is worth repeating the whole explanation already
given to "v4" for two other.  By just mentioning that the "v4" could
be "v4.4" or "v4vis", the readers are intelligent enough to infer
what you mean, I would think.  Also be honest to readers by telling
them what they lose if they use a non-integral reroll count.

	`<n>` does not have to be an integer (e.g. "--reroll-count=4.4",
	or "--reroll-count=4rev2" are allowed), but the downside of
	using such a reroll-count is that the range-diff/interdiff
	with the previous version does not state exactly which
	version the new interation is compared against.

or something like that, perhaps?

Thanks.

Junio C Hamano [off-list ref] 于2021年3月21日周日 上午3:55写道：

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

As the patch text itself (assuming that the vague "compared with the
previous iteration" is acceptable) is getting solid, let's nitpick
the proposed log message so that we can start considering a merge to
'next'.

I also think that this patch series has been on the mailing list for a
long time.

I won't review the t/ part of the patch as I know others on CC are
better at reviewing the tests than I am ;-)

Maybe I should look at Eric Sunshine's opinion :)

quoted

From: ZheNing Hu <redacted>

Usually we can only use `format-patch -v<n>` to generate integral
version numbers patches,

The "only" sounds as if you are saying that "there is no tool other
than 'format-patch -v' we can use", which might be technically true,
but is not what you want to stress to your readers here.  You are
sayig that usually people only use integral version numbers.

You are right, this may be another place where I am not clear.
"only" should be refers to "intergral version", not "format-patch".

quoted

but sometimes a small 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.

Also, I do not think we want to encourage such use of fractional
iteration numbers.  We can merely enable, without saying it is a
good idea or a bad idea, leaving judgment to each project that may
or may not accept a patch versioned in such a way.  So avoid "should
be".

Yes, it is more appropriate to use words similar to "can be" than "should be".

That makes the first part of the log message to be something like:

    The `-v<n>` option of `format-patch` can give nothing but an
    integral iteration number to patches in a series.  Some people,
    however, prefer to mark a new iteration with only a small fixup
    with a non integral iteration number (e.g. an "oops, that was
    wrong" fix-up patch for v4 iteration may be labeled as "v4.1").

    Allow `format-patch` to take such a non-integral iteration
    number.

quoted

`<n>` can be any string, such as '3.1' or '4rev2'. In the case
where it is a non-integral value, the "Range-diff" and "Interdiff"
headers will not include the previous version.

This part is well written and can stay as-is.

quoted

Signed-off-by: ZheNing Hu <redacted>
---
    [GSOC] format-patch: allow a non-integral version numbers

    There is a small question: in the case of --reroll-count=<n>, "n" is an
    integer, we output "n-1" in the patch instead of "m" specified by
    --previous-count=<m>,Should we switch the priority of these two: let "m"
    output?

Didn't I answer this question already?

Sorry, forgot to update GGG cover-letter messages.

quoted

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 3e49bf221087..e2c29a856451 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt

@@ -221,6 +221,11 @@ populated with placeholder text.
      `--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.
+      `<n>` may be a non-integer number.  E.g. `--reroll-count=4.4`

Is it on purpose that `<n>` here has an extra SP before it?

quoted

+     may produce `v4.4-0001-add-makefile.patch` file that has
+     "Subject: [PATCH v4.4 1/20] Add makefile" in it.
+     `--reroll-count=4rev2` may produce `v4rev2-0001-add-makefile.patch`
+     file that has "Subject: [PATCH v4rev2 1/20] Add makefile" in it.

I am not sure it is worth repeating the whole explanation already
given to "v4" for two other.  By just mentioning that the "v4" could
be "v4.4" or "v4vis", the readers are intelligent enough to infer
what you mean, I would think.  Also be honest to readers by telling
them what they lose if they use a non-integral reroll count.

        `<n>` does not have to be an integer (e.g. "--reroll-count=4.4",
        or "--reroll-count=4rev2" are allowed), but the downside of
        using such a reroll-count is that the range-diff/interdiff
        with the previous version does not state exactly which
        version the new interation is compared against.

or something like that, perhaps?

This way of writing is much more concise than what I did before.
I need to exercise my ability to write documents :p

Thanks.

Thanks, Junio.