On 2016-01-23 1:20 PM, Gregory Szorc wrote:
On Fri, Jan 22, 2016 at 1:45 PM, Boris Zbarsky <bzbar...@mit.edu> wrote:
On 1/22/16 3:52 PM, Gregory Szorc wrote:
I would say that pushing cherry-picked commits for review that depend on
other commits not in the commit's ancestry is just wrong. If you pushed
this to Try, it would fail. So why are you pushing a "bad" commit/tree for
review? If your commits depend on something else, that something else
should be in the ancestry [and available to MozReview to inspect].
I'm going to assume this is a non-rhetorical question.
Here's a concrete example of a reason to do this. Say I have two patches
for two separate bugs. They're totally unrelated to each other in general,
but they happen to both need to touch the same spot of code, possibly in
the same way, but possibly in slightly different ways). So the patches
conflict with each other, effectively.
Now I have several options:
1) Develop the patches on separate branches (or on separate repos),
request separate reviews on them, whichever one loses the review/land race
needs to get updated in mozreview before getting pushed.
2) Develop the patches on the same branch, request reviews on them, if
the one that's "second" gets review first, reorder the commits, making the
minor changes needed to handle that one conflict, land the thing that has
review, update the other one in mozreview.
3) Develop the patches on the same branch, request reviews on them, wait
until the "first" one has reviews before landing either one.
In practice, I believe option 3 is the worst one for the situation I
described, because it gates an unrelated bug on another bug. And this
situation happens all the time. I'd estimate that at least 10% of my
patches have self-conflicts of the type I described above.
FWIW, option 3 is basically my usual workflow, and you didn't really
respond to this.
The reason why I need to do things this way by default is that out build
system is extremely hostile to topic branches, for example,
<https://bugzilla.mozilla.org/show_bug.cgi?id=1122851>, or the need to
spend ~20 minutes rebuilding everything if the branch you switched to
happens to change a configure variable, etc.
Many times I also have to do this because it's unclear which reviewer
will review first, and serializing the review requests in the order in
which the commits appear in my repo will make the process of landing
code take too long.
Option 2 is nice in needing a bit less overhead in terms of working trees
or rebuilds on branch-switches. It also just happens automatically quite
often, unless you're using one branch per bug always. I believe you're
saying this option is unsupported by MozReview if the actual branch is
pushed (because it's two separate bugs), so the natural way to use
MozReview here would be to cherry-pick.
FWIW I should also mention that I regularly run into situations where I
need to maintain a 30-40 commit long local queue of unlanded stuff, and
in which case option 2 will just be way too painful if something gets
landed which has 10 commits on top of it waiting for review.
While MozReview defaults to submitting all pushed commits for review, you
can override these defaults to pick a) any single commit b) a range of
commits at the bottom c) middle or d) top of the series.
I think that doesn't really address any of the aforementioned problems.
The problem is not in submitting specific commits through MozReview.
Option 1 just happens if you use one branch per bug, or just happen to
work in a different local repo and don't realize you have the
self-conflict. Happens all the time, again. In practice it will lead to
precisely the "cherrypicked" setup Ehsan described: as soon as one patch
lands, the other will behave as if it had been cherrypicked.
In general, I don't want MozReview to dictate client-side branching
workflows. It does that today courtesy of not handling multi-bug series and
advanced commit tracking that well. Support will improve over time.
Note that there are also other things that might dictate people's local
workflows, as I gave a few examples above.
It's worth noting that I also want to put less emphasis on bugs as our unit
of work.
That's fine, but it's orthogonal to the problems above. You can just
re-read everything above and remove all mentions of "bugs" and I believe
all problems stated so far will remain.
> Modern version control workflows revolve around lines of work, or
topic branches. It is a generally accepted best practice to use multiple
topic branches. But some people just lump everything together because it
can be easier than wrestling with endless history editing and merge
conflicts in your version control tool. Anyway. sometimes a feature branch
is related to a single bug. Sometimes multiple bugs. Sometimes you start
work on something that has no bug yet! I'd like our code development
workflow to flow more around the code than bugs. Bugs may be the driver for
the work and the ultimate mechanism to track that work. But the
code/commits - not the bugs - should be driving the workflow. It feels
wrong to me that we currently have to practice sub-optimal code writing and
review workflows to accommodate the relationship between bugs.
Again, that's not really the case. You're positing that the reviewer
controlling what gets landed when is the optional code writing and
review workflow, and we're disagreeing with that (at least as the
default rule -- the suggestion of the author indicating somehow that the
reviewer can push the "Autoland" button is fine.)
Related to this, I always found it a bit surprising we perform so much
activity on the patch author side before submission. Part of me thinks
reviewers should take one quick glance at the interdiff before the final
version lands and should be gate keepers. To not do this is somewhat
undermining the review process.
I am not the busiest reviewer out there but I review a decent number of
patches. I can't think of _ever_ wanting to look at the interdiff
resulted from a rebase.
Also, I believe we have data showing that most of our reviews are done
by a relatively small portion of people. I'd like to humbly suggest
that changing processes to put even more burden on the reviewers may not
necessarily be the best course of action going forward.
> Another part of me realizes that we're
generally smart, well-intentioned people that get the rewriting correct 95%
of the time and that requiring a cursory re-review is much unwanted process
overhead and delay. MozReview does carry forward r+ if it was granted,
effectively trusting author-side rewrites and allowing Autoland to occur. I
think this is the correct model: if a reviewer absolutely wants to see the
final version before landing, they can withhold r+ until all issues are
addressed.
This is exactly how our review process has ever worked.
A benefit to re-publishing the final commit state to MozReview before
landing is that this shows up as an interdiff, which kind of closes the
loop on the patch's lifecycle (although we could have Autoland publish
landed commits to MozReview if we wanted to).
I'm not sure about Boris, but I'm not objecting to republishing the
final commit state to MozReview before landing. My objection is to the
part about the reviewer initiating the landing process.
I hope this addressed your concerns. If you want more detail, perhaps a
centralized document would be a better medium than a mailing list.
So what's the concrete solution you're proposing in response to this?
Speaking only for myself, I don't feel that I have really seen a
suggestion as to how to deal with the issues I raised.
I'd be fine with continuing the discussion on a google doc if you
_really_ want to, but I prefer to talk about this here where many of the
authors and reviewers who your proposal will impact hang out.
Cheers,
Ehsan
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
