Jeff King [off-list ref] 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.
On Thu, Aug 04, 2016 at 02:12:22PM -0700, Junio C Hamano wrote:
quoted
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.
[...]
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;
Actually, I think the 2-a class is what often saves a re-roll. Somebody
points out a typo in a commit message or a comment, and it quite often
gets picked up by you without having another round-trip to the list.
If you want to save work by not doing so, that's fine with me. But this
is the gamble I was talking about. I think it's actually often less work
to do the fixup than to look at another re-roll (especially with the
"leaky contributor" thing where you have to make sure all fixes were
applied). So it's a win if it saves the re-roll, but sometimes you end
up having to look at the re-roll anyway.
* 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.
I think these are largely the same as 2-a. You are just wearing two
hats, reviewer and maintainer. Which I guess lets you take a shortcut
sometimes (and just fix without mentioning it), but fundamentally the
"gamble" aspect is the same, I think.
-Peff
Hi Junio,
On Thu, 4 Aug 2016, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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.
I find it incredibly nice of you to do those fix-ups, sorry to state that
as clearly only now. It's just that I hate to see your time wasted, in
particular when *I* waste it because I missed the fix-ups in `pu`.
quoted
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.
FWIW I am more than just willing to spend a little more time on applying
fix-ups and re-rolling patch series (and dual-publishing them via my
public repository), if it helps lighten your burden.
quoted
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.
quoted
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).
quoted
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;
If you had a `git commit --reword` command to touch up commit messages,
would that help you, together with the `git commit --fixup` command for
code changes? The branches in `pu` would have your fix-ups as strictly
separate commits on top of the contributed patches, and the branches would
need to be sent through rebase -i before merging to `next`, of course.
The idea would be to not forget your fixups in subsequent iterations, but
simply rebase them on top of the new iteration.
It would still not solve my problem that there is no convenient way to
jump from your commits in `pu` to the corresponding ones in my local
branch. But that is my problem, not yours.
Ciao,
Dscho