1021907 - Determine how to deal with extra commits you don't want reviewed

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Mark Côté [:mcote]]

It seems there has not been a clear decision on how we deal with the fact that you may be pushing commits to the review repo that are not part of the set you want reviewed.  The most common example would be if the review repo is not synced exactly to the production repo, so when you push your changes, you are including recent commits to production authored (presumably) by someone else.

This was discussed at length in a thread: https://groups.google.com/forum/#!topic/mozilla-code-review/d4Mx1dBiP5w.  But no decision has been made.

These were the proposals, for the short term at least:

1. Regularly pull changes from production to review.  This would lessen the chance of extraneous commits being pushed, but it wouldn't completely eliminate them.

2. Pull from production to review when commits are pushed.  This would either make pushes slow (but how slow, if we frequently pull via a cron job?) or make them asynchronous (possibly bad for usability, unless we notify the bug when the review is created).

3. Have the client optionally specify the revisions they want reviewed, e.g. HEAD~3 (defaulting to, say, HEAD~1, or whatever is at the head of the review repo).  If specified, this is unambiguous but requires the user to do a bit more work, although it could be folded into a mach command.

I am personally in favour of solution 3 because it seems to be the easiest and is unambiguous.

Comment 1

[Gregory Szorc [:gps]]

When a Mercurial client-side extension is written, it can do #3 transparently (at least to identify base revisions - trailing commits are another matter entirey). That implementation can be made more efficient if Mozilla were to set up a unified repository or were to operate a simple web service that published the tip changesets (+ a few ancestors for out-of-date clients). Of course, once we have a canonical "these are the tip changesets" repo/service, the review repo could also consult that at push.

Comment 2

[James Graham [:jgraham]]

I don't see how it can be transparent in both of the following situations:

1) I make a review for commit B based on commit A which is itself in review.
2) I make a review for commit as A and B and for some reason drop the review. I then decide commit A was fine and make a review from A and C.

Comment 3

[Gregory Szorc [:gps]]

(In reply to James Graham [:jgraham] from comment #2)
> I don't see how it can be transparent in both of the following situations:
> 
> 1) I make a review for commit B based on commit A which is itself in review.
> 2) I make a review for commit as A and B and for some reason drop the
> review. I then decide commit A was fine and make a review from A and C.

Both of these cases can be solved by obsolescence / changeset evolution. However, we have another 6+ months before that's stable for wide use and probably another year to infinity before Mozillians realize mq is a really bad way to do version control and abandon it.

As stated numerous times on the mailing list, we can also use the mq patch names or bookmark or branch names to provide persistent review identifiers. Transferring the mq patch names would require an extension on both client and server. The others don't require extensions - just RB to associate names with reviews. (This is presumably how Git will work with named branches turning into reviews.)

Comment 4

[Mark Côté [:mcote]]

It sounds like the way forward, then, is to go with option 3.  Ideally this will be hidden behind a mercurial extension before too long, but at least we'll have something that can be used in the interim.

Comment 5

[(no longer active)]

I think solution #3 is the best if someone is willing to write that tool right now (I don't think it would be too much work.)  George, did you agree to do that work?  Thanks!  :-)

Comment 6

[George Miroshnykov ( :gmiroshnykov )]

Sure thing, I'm on it :)

Comment 7

[George Miroshnykov ( :gmiroshnykov )]

So I've added one more thing that we'll have to hide behind a mach command - ability to override first commit ID via RT_NODE env var.

The full raw command now looks like this:
RT_BUG=31337 RT_USERNAME=[e-mail] RT_PASSWORD=secret RT_NODE=b4365e7f103b0d99d52128dea6b591d07aa29b9b hg push -f -e 'ssh -o SendEnv=RT_*' review

Comment 8

[Mark Côté [:mcote]]

This is not quite what was suggested (a more general form of specifying revisions and revision ranges like what git does), but I think it's good enough for now.

Comment 9

[Mark Côté [:mcote]]

This is deployed to reviewboard-dev and seems to work fine; I just want to do a little more testing before resolving.

Comment 10

[Mark Côté [:mcote]]

Also, we should still have a cron job set up, since early adoption might be slow, and even if pushing 100 commits works, it'll be a little painful.  Filing a separate bug.

Comment 11

[Mark Côté [:mcote]]

This is fixed, in both the original hook and the new Python one.