Denton Liu [off-list ref] writes:
Hmm, others may disagree but I don't really like the idea of
`--previous-count`. It may be useful for populating "Range-diff vs <n>"
instead of just "Range-diff" but I don't think it's worth the cost of
maintaining this option.
It really depends on the target audience. As a reviewer who may be
too busy to read every iteration of a series, I would probably find
it useless if it gives just "range-diff" or "range-diff with last"
without saying which exact round. Obviously, if you are not doing
range-diff, it will not be an issue. If the patch requires (I
didn't read the latest one) the previous-count to be given when
range-diff or interdiff is not requested, it should probably be
fixed.
I am also OK with any design decision, as long as it will not close
the door for the occasionally requested feature to carry over cover
letter material from the previous round to the current one.
Thanks.
Junio C Hamano [off-list ref] 于2021年3月4日周四 上午8:54写道：
Denton Liu [off-list ref] writes:
quoted
Hmm, others may disagree but I don't really like the idea of
`--previous-count`. It may be useful for populating "Range-diff vs <n>"
instead of just "Range-diff" but I don't think it's worth the cost of
maintaining this option.
It really depends on the target audience. As a reviewer who may be
too busy to read every iteration of a series, I would probably find
it useless if it gives just "range-diff" or "range-diff with last"
without saying which exact round. Obviously, if you are not doing
range-diff, it will not be an issue. If the patch requires (I
didn't read the latest one) the previous-count to be given when
range-diff or interdiff is not requested, it should probably be
fixed.
I am also OK with any design decision, as long as it will not close
the door for the occasionally requested feature to carry over cover
letter material from the previous round to the current one.
Thanks.
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.
Here are my personal thoughts:
Personally, I may use GGG more. When I see a title like "Range-diff vs v1:",
I can know that this is a change from the previous v1, and it may be better
than "Range-diff again v1" To be more specific, but if it is a small patch such
as "v1.2", we use previous_count to tell the reviewer that this is a
range-diff
change from "v1.1" or other versions.
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.
Thanks.
On Wed, Mar 3, 2021 at 9:08 PM ZheNing Hu [off-list ref] 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.
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.
Hi Eric,
On Wed, Mar 03, 2021 at 10:27:25PM -0500, Eric Sunshine wrote:
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.
Although it is quite rare, I'm definitely not the only one who's done
it. A cursory search on the list reveals a few examples from people
other than me:
* https://lore.kernel.org/git/20180308224458.5730-1-szeder.dev@gmail.com/
* https://lore.kernel.org/git/20190706162114.21169-1-szeder.dev@gmail.com/
* https://lore.kernel.org/git/20180225134015.26964-1-szeder.dev@gmail.com/
* https://lore.kernel.org/git/1435350166-7122-1-git-send-email-Matthieu.Moy@imag.fr/
* https://lore.kernel.org/git/574DED66.6050008@web.de/