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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 4, 2021, 03:27 UTC
Message-ID: <CAPig+cTncEC4njnu+FB9tKwu20xi_UuL4TWW3_zD3drD3fyrHw@mail.gmail.com>
In-Reply-To: <CAOLTT8RdXC+KQNupU1TQdPh-tQO+syd6WJe85GzieE3uWt2ibA@mail.gmail.com>

On Wed, Mar 3, 2021 at 9:08 PM ZheNing Hu <adlternative@gmail.com> wrote:

> What we are arguing now is whether it is necessary to add
> "aginst v<previous_count>" to the patch when the non-integer version
> number + rangediff/interdiff is required. Denton's point of view may be
> similar to that of Eric before.

Yes, it sounds as if Denton and I share the same point of view.

Show 11 quoted lines

> Here are my personal thoughts:
>
> Of course this `previous count` can be used in a very small range, but
> I think it
>  doesn't hurt to keep it, because even if you don't use it, `format
> patch` will still
> output "Range-diff", which will not break any known functions. It can
> only be said
> that `previous count` provides an option for submitters to know the
> previous version
>  for reviewers. In this regard, I agree with Junio's point of view.

I'm not outright opposed to supporting non-numeric, non-integer reroll-counts, but I also don't see a big need for it. As mentioned earlier, Denton is the only person I recall who sends fractional re-rolls, so it's not obvious that there is a big advantage to adding such support and complicating the code just for one person. Also, when Denton does send fractional re-rolls, he typically does so for just a single patch out of a longer series, and he doesn't (I think) provide a range-diff or interdiff for the patch. So, for Denton's intended use-case, this entire discussion about "Range-diff against v$V" and "Interdiff against v$V" seems superfluous. That is, the simple logic:

    if reroll_count specified and is integer:
        s = "Range-diff against v${reroll_count -1}:"
    else
        s = "Range-diff:"

satisfies Denton's case without the complication of adding a --previous-count switch. This probably explains why Denton doesn't see a need for the extra complexity of --previous-count.

So, some ways forward are:

(1) drop this topic altogether since it so far seems of interest to only a single person (Denton) -- nobody else has asked for it

(2) support non-integer reroll-count but just use the simple logic shown above for constructing the "Range-diff/Interdiff against" header; this leaves the door open for Junio's idea(s) of allowing the user to specify --previous-count and automatically determining the previous reroll-count by scanning a directory

(3) continue refining the changes made by this patch until reviewers are happy with it (which might take a few more re-rolls)

I lean toward #1, but wouldn't be opposed to #2 or #3 either.