Remove manual --tbr support and convert it into automatic TBR= detection.

Remove manual --tbr support and convert it into automatic TBR= detection.

It result in less code, forces uploading the change for eventual review and most
importantly enables the commit queue to correctly handle this.

BUG=
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91575

[M-A Ruel, 2011-06-27 21:17:46]

Dirk, can you review?

Evan, any opinion? I want to move the TBR check into the presubmit check so it
will enable commit queue TBR check-in and will ease the consistency to switch to
gerrit and achieve the same thing.

This change changes the semantic. It now requires to upload the change to be
able to commit it. We'll need to fix any documentation mentioning --tbr to put
TBR= in the description instead.

Note that gcl never implemented the equivalent functionality.

http://codereview.chromium.org/7253015/diff/1/tests/post-dcommit-hook-test.sh
File tests/post-dcommit-hook-test.sh (right):

http://codereview.chromium.org/7253015/diff/1/tests/post-dcommit-hook-test.sh...
tests/post-dcommit-hook-test.sh:16: git branch -m COMMITTED
Should probably test for presubmit_support -> tbr?

http://codereview.chromium.org/7253015/diff/1/tests/tbr.sh
File tests/tbr.sh (right):

http://codereview.chromium.org/7253015/diff/1/tests/tbr.sh#newcode21
tests/tbr.sh:21: git add test; git commit -q -m "work \
It's not doing any good testing anymore since there is no presubmit check. I
guess it's better to remove that test. Any opinion?

[Dirk Pranke, 2011-06-27 21:51:23]

The changes to the code looks fine, but I am a bit uncomfortable with the
implied change to the functionality of the tools.

In my mind, there were two use cases for --tbr. The first was an actual TBR
where you wanted to land a change that really needed a review, but could not
afford to wait (e.g., build breakage). 

The second was for changes that are/were essentially unreviewed, like webkit
rolls and expectations changes. 

I never particularly liked using --tbr for the latter, but there wasn't a good
alternative (and still isn't). 

Also, it was nice that there was a way to land the change without having to
upload it first; it was one command line to type, instead of two.

Granted, all of these things are little, but it's the little things that make
programming more or less fun.

If it's true that we're moving to a world where we really want every change to
be uploaded, maybe we should modify the tools to do upload & commit in one step?

Note that I am ignoring --bypass-hooks / --no_presubmit here. In my mind those
flag *only* exist for cases where you have to work around brokenness in the
tooling and should never be used for things that are supposed to work.

LGTM from the correctness point of view; debating whether we actually want this
change is a slightly different conversation and should perhaps be held somewhere
else (a note to chromium-dev@ maybe?)

http://codereview.chromium.org/7253015/diff/1/tests/post-dcommit-hook-test.sh
File tests/post-dcommit-hook-test.sh (right):

http://codereview.chromium.org/7253015/diff/1/tests/post-dcommit-hook-test.sh...
tests/post-dcommit-hook-test.sh:16: git branch -m COMMITTED
On 2011/06/27 21:17:46, Marc-Antoine Ruel wrote:
> Should probably test for presubmit_support -> tbr?

Didn't we get rid of all of the .git/hooks/* code, or was it just the other
hooks?

http://codereview.chromium.org/7253015/diff/1/tests/tbr.sh
File tests/tbr.sh (right):

http://codereview.chromium.org/7253015/diff/1/tests/tbr.sh#newcode21
tests/tbr.sh:21: git add test; git commit -q -m "work \
On 2011/06/27 21:17:46, Marc-Antoine Ruel wrote:
> It's not doing any good testing anymore since there is no presubmit check. I
> guess it's better to remove that test. Any opinion?

I would just remove the test.

[M-A Ruel, 2011-06-29 14:56:20]

Adding more context and discussion done over IM.

gcl never supported --tbr. Only git cl does.

The new workflow requires having TBR= in the description, this could be more
awkward to some people since it means writing the description accordingly. IMHO
it's a good thing, Dirk wasn't sure since git cl dcommit --tbr adds it for you.
It's less text to write in his opinion, it's one less character to type in my
opinion. :)

Forcing to upload means running 2 commands for things like webkit roll to commit
manually. This is a non-issue if the use case is to use the commit queue since
it's still be a single command: git cl upload -c

There is no other meaningful way to simulate --tbr with the commit queue.

We weren't sure if in gerrit land we can push a change without creating a review
(?)

Dirk, did I miss anything?

[Evan Martin, 2011-06-29 16:01:47]

On Mon, Jun 27, 2011 at 2:17 PM,  <maruel@chromium.org> wrote:
> Evan, any opinion?

I defer to you and Dirk.  I can break a tie if needed. ;)

[Dirk Pranke, 2011-06-29 17:16:26]

On Wed, Jun 29, 2011 at 7:56 AM,  <maruel@chromium.org> wrote:
> Adding more context and discussion done over IM.
>
> gcl never supported --tbr. Only git cl does.
>
> The new workflow requires having TBR= in the description, this could be more
> awkward to some people since it means writing the description accordingly.
> IMHO
> it's a good thing, Dirk wasn't sure since git cl dcommit --tbr adds it for
> you.
> It's less text to write in his opinion, it's one less character to type in
> my
> opinion. :)
>
> Forcing to upload means running 2 commands for things like webkit roll to
> commit
> manually. This is a non-issue if the use case is to use the commit queue
> since
> it's still be a single command: git cl upload -c
>
> There is no other meaningful way to simulate --tbr with the commit queue.
>
> We weren't sure if in gerrit land we can push a change without creating a
> review
> (?)
>
> Dirk, did I miss anything?
>

Sounds about right. Feel free to land the change if you don't get
strong objections.

-- Dirk

>
>
> http://codereview.chromium.org/7253015/
>

[Nico, 2011-07-24 19:59:33]

http://codereview.chromium.org/7253015/diff/8001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/7253015/diff/8001/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:68: """Checks that the issue was uploaded before
committing."""
This warns if I patch in someone else's patch with `git cl patch` and then
submit with `git cl dcommit -c 'Joe Contributor <contributor@joe.org>` :-(