Indiciate the tree status in the rietveld message when bypassing hooks

Indiciate the tree status in the rietveld message when bypassing hooks

BUG=none
R=iannucci@chromium.org,maruel@chromium.org

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

[iannucci, 2014-01-14 10:18:29]

On 2014/01/14 10:09:02, jochen wrote:

(rietveld fail... looks like patch may need to be reuploaded)

[iannucci, 2014-01-14 10:21:44]

I'm OK with the basic concept though, and raw diff lgtm.

Could we also kepp gcl in parity too?

[jochen (gone - plz use gerrit), 2014-01-14 10:21:48]

ptal

[jochen (gone - plz use gerrit), 2014-01-14 10:22:18]

On 2014/01/14 10:21:44, iannucci wrote:
> I'm OK with the basic concept though, and raw diff lgtm.
> 
> Could we also kepp gcl in parity too?

i didn't find an obvious way in gcl to get the tree status?

[iannucci, 2014-01-14 10:28:15]

On 2014/01/14 10:22:18, jochen wrote:
> On 2014/01/14 10:21:44, iannucci wrote:
> > I'm OK with the basic concept though, and raw diff lgtm.
> > 
> > Could we also kepp gcl in parity too?
> 
> i didn't find an obvious way in gcl to get the tree status?

Looks like gcl imports git_cl (ironically), so you should be able to just do
git_cl.GetTreeStatus(), I think.

[jochen (gone - plz use gerrit), 2014-01-14 10:48:23]

ptal

[M-A Ruel, 2014-01-14 19:07:33]

https://codereview.chromium.org/130803004/diff/170001/gcl.py
File gcl.py (right):

https://codereview.chromium.org/130803004/diff/170001/gcl.py#newcode738
gcl.py:738: return git_cl.GetTreeStatus(tree_status_url) if tree_status_url else
"unset"
return None otherwise.

https://codereview.chromium.org/130803004/diff/170001/gcl.py#newcode1074
gcl.py:1074: comment += ' (tree status was "%s")' % GetTreeStatus()
Do not add this suffix "if not GetTreeStatus()"

[iannucci, 2014-01-14 19:23:41]

https://codereview.chromium.org/130803004/diff/170001/gcl.py
File gcl.py (right):

https://codereview.chromium.org/130803004/diff/170001/gcl.py#newcode738
gcl.py:738: return git_cl.GetTreeStatus(tree_status_url) if tree_status_url else
"unset"
On 2014/01/14 19:07:34, M-A Ruel wrote:
> return None otherwise.

Ah, oops, this is why I shouldn't do code reviews at 2am :/

[jochen (gone - plz use gerrit), 2014-01-14 19:41:31]

ptal

I left the status in for the breakpad call, so we have a full audit trail

[iannucci, 2014-01-14 19:45:57]

https://codereview.chromium.org/130803004/diff/210001/gcl.py
File gcl.py (right):

https://codereview.chromium.org/130803004/diff/210001/gcl.py#newcode738
gcl.py:738: return git_cl.GetTreeStatus(tree_status_url) if tree_status_url else
"unset"
this should still probably return None instead of 'unset'

[jochen (gone - plz use gerrit), 2014-01-14 19:49:32]

On 2014/01/14 19:45:57, iannucci wrote:
> https://codereview.chromium.org/130803004/diff/210001/gcl.py
> File gcl.py (right):
> 
> https://codereview.chromium.org/130803004/diff/210001/gcl.py#newcode738
> gcl.py:738: return git_cl.GetTreeStatus(tree_status_url) if tree_status_url
else
> "unset"
> this should still probably return None instead of 'unset'

the git_cl.py version returns 'unset' if url is None. I think keeping the two
methods in sync is preferable

[iannucci, 2014-01-14 19:56:13]

On 2014/01/14 19:49:32, jochen wrote:
> On 2014/01/14 19:45:57, iannucci wrote:
> > https://codereview.chromium.org/130803004/diff/210001/gcl.py
> > File gcl.py (right):
> > 
> > https://codereview.chromium.org/130803004/diff/210001/gcl.py#newcode738
> > gcl.py:738: return git_cl.GetTreeStatus(tree_status_url) if tree_status_url
> else
> > "unset"
> > this should still probably return None instead of 'unset'
> 
> the git_cl.py version returns 'unset' if url is None. I think keeping the two
> methods in sync is preferable

Well that's just silly. In that case, lgtm.

[commit-bot: I haz the power, 2014-01-14 19:58:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/130803004/210001

[commit-bot: I haz the power, 2014-01-14 20:00:04]

Change committed as 244793

[Dirk Pranke, 2014-01-14 22:56:20]

https://codereview.chromium.org/130803004/diff/210001/gcl.py
File gcl.py (right):

https://codereview.chromium.org/130803004/diff/210001/gcl.py#newcode1074
gcl.py:1074: comment += ' (tree was closed).' if GetTreeStatus() == 'closed'
else '.'
Since "tree was closed" is not the same thing as "presubmit was bypassed", it's
too bad that we can't distinguish between these two cases. I.e., it would be
nice to know if the other hooks ran and passed, but we can't currently do this
without a fair amount of rejiggering, I think.

[iannucci, 2014-01-14 23:23:33]

On 2014/01/14 22:56:20, Dirk Pranke wrote:
> https://codereview.chromium.org/130803004/diff/210001/gcl.py
> File gcl.py (right):
> 
> https://codereview.chromium.org/130803004/diff/210001/gcl.py#newcode1074
> gcl.py:1074: comment += ' (tree was closed).' if GetTreeStatus() == 'closed'
> else '.'
> Since "tree was closed" is not the same thing as "presubmit was bypassed",
it's
> too bad that we can't distinguish between these two cases. I.e., it would be
> nice to know if the other hooks ran and passed, but we can't currently do this
> without a fair amount of rejiggering, I think.

Agreed.