Jeff King <peff@xxxxxxxx> writes:
> Like you, I have occasionally been bitten by Junio doing a fixup, and
> then I end up re-rolling, and lose that fixup.
... which usually is caught when I receive the reroll, as I try to
apply to the same base and compare the result with the previous
round.
> But I think such fixups are a calculated risk. Sometimes they save a lot
> of time, both for the maintainer and the contributor, when they manage
> to prevent another round-trip of the patch series to the list.
Yes.
> IOW, if the flow is something like:
>
> 1. Contributor sends patches. People review.
>
> 2. Minor fixups noticed by maintainer, fixed while applying.
This includes different kinds of things:
a) Trivially correct fixes given in other people's review.
b) Minor fixups by the maintainer, to code.
c) Minor fixups by the maintainer, to proposed log message.
d) "apply --whitespace=fix" whose result I do not even actively
keep track of.
> 3. Only one small fixup needed from review. Contributor sends
> squashable patch. Maintainer squashes.
>
> then I think that is a net win over sending the whole series again, for
> the contributor (who does not bother sending), reviewers (who really
> only need to look at the interdiff, which is what that squash is in the
> first place), and the maintainer (who can squash just as easily as
> re-applying the whole series).
> And that is the flip side. If the flow above does not happen, then step
> 2 just becomes a pain.
I think I can
* stop taking 2-a). This is less work for me, but some
contributors are leaky and can lose obviously good suggestions,
so I am not sure if that is an overall win for the quality of the
end product;
* do a separate "SQUASH???" commit and send them out for 2-b).
This is a lot more work for a large series, but about the same
amount of work (except for "remembering to send them out" part)
for a small-ish topic. A contributor needs to realize that I
deal with _all_ the incoming topics, not just from topics from
one contributor, and small additional work tends to add up.
to reduce #2. Essentially, doing these two are about moving more
responsibility of keeping track of good suggestions in the review
discussion to the contributor, but a bad thing is that it does not
mean that the maintainer can stop keeping track of them. We need a
way to make sure leaky contributors do not repeat the same issue in
their reroll that has already been pointed out and whose solutions
presented in the previous review. Fetching from 'pu' and compare
has been one way to do so (that is why I publish 'pu' in the first
place, not to "build on", but to "view"), but apparently not many
contributors are comfortable with that idea.
There is no good way to reduce 2-c) and 2-d), but on the other hand,
there is no strong need for a special channel to propagate these
back. 2-c) can be a regular review comment (but again that risks
"the leaky contributor" problem) and 2-d) will happen automatically
when applying the rerolled version.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html