Add upload and issue to git_cl recipe module

Add upload and issue to git_cl recipe module

Depends on https://codereview.chromium.org/2088343002

Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/808d7ea4f6feeb1cc6df0ca7bde9a9ebb48c3f30

[martiniss, 2016-06-22 00:02:54]

martiniss@chromium.org changed reviewers:
+ iannucci@chromium.org, tandrii@chromium.org

[martiniss, 2016-06-22 00:02:55]

FYI

Almost good to go, but expect_tests is unhappy about the example.py. Robbie, are
we not allowed to put unicode in expectation json files? It gives me this error
when I test it:

/s/depot_tools/.recipe_deps/recipe_engine/recipe_engine/third_party/expect_tests/serialize.py:119:
UnicodeWarning: Unicode equal comparison failed to convert both arguments to
Unicode - interpreting them as being unequal
  if old == new:
F

======================================================================
FAIL: git_cl:example.basic
(/s/depot_tools/recipe_modules/git_cl/example.expected/basic.json)
----------------------------------------------------------------------
--- expected
+++ current
@@ -7,9 +7,9 @@
           'description',
           '-n',
           '-'],
   'name': 'git_cl set description',
-  'stdin': 'Do the thing foobar\nNow with emoji:
\xc3\xb0\xc2\x9f\xc2\x98\xc2\x84',
+  'stdin': u'Do the thing foobar\nNow with emoji: \xf0\x9f\x98\x84',
   'stdout': '/path/to/tmp/'},
  {'cmd': ['RECIPE_PACKAGE_REPO[depot_tools]/git_cl.py', 'issue'],
   'name': 'git_cl issue',
   'stdout': '/path/to/tmp/'},


----------------------------------------------------------------------

[tandrii(chromium), 2016-06-22 14:41:15]

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.py
File recipe_modules/git_cl/api.py (right):

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
recipe_modules/git_cl/api.py:12: for i in range(len(msg)):
1. Pythonic way:
  for i, c in enumerate(msg):
    if not c in string.printable
      return msg[:i]
  return msg

2. I don't see how truncating message means "cleaning".
3. If msg is a unicode string, I am not sure certain your algo is 100% working
as intended. For utf8 string, maybe, because ASCII is its subset, so utf8 has to
use first byte as '\0' for multi-byte chars. But at least document that here.

>>> u = unichr(40960) + u'abcd' + unichr(1972)
>>> len(u)
9

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
recipe_modules/git_cl/api.py:58: # message can have unicode, which we can't pass
on the command line, so
why not pass it in a temp file, or stdin? Patching git_cl for that is trivial.

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
recipe_modules/git_cl/api.py:60: one_line_message =
clean_message(message.split('\n')[0]) + "..."
message.splitlines()[0]

[martiniss, 2016-06-22 22:47:34]

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.py
File recipe_modules/git_cl/api.py (right):

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
recipe_modules/git_cl/api.py:12: for i in range(len(msg)):
On 2016/06/22 at 14:41:15, tandrii(chromium) wrote:
> 1. Pythonic way:
>   for i, c in enumerate(msg):
>     if not c in string.printable
>       return msg[:i]
>   return msg
Great, thanks.
> 
> 2. I don't see how truncating message means "cleaning".
Clarified; it's not really cleaning, just making something that I can pass on
the command line.
> 3. If msg is a unicode string, I am not sure certain your algo is 100% working
as intended. For utf8 string, maybe, because ASCII is its subset, so utf8 has to
use first byte as '\0' for multi-byte chars. But at least document that here.
> 
> >>> u = unichr(40960) + u'abcd' + unichr(1972)
> >>> len(u)
> 9

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
recipe_modules/git_cl/api.py:58: # message can have unicode, which we can't pass
on the command line, so
On 2016/06/22 at 14:41:15, tandrii(chromium) wrote:
> why not pass it in a temp file, or stdin? Patching git_cl for that is trivial.

I do that below, in the call to set description.

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
recipe_modules/git_cl/api.py:60: one_line_message =
clean_message(message.split('\n')[0]) + "..."
On 2016/06/22 at 14:41:15, tandrii(chromium) wrote:
> message.splitlines()[0]

Done.

[martiniss, 2016-06-23 00:12:54]

Description was changed from

==========
Add upload and issue to git_cl recipe module
==========

to

==========
Add upload and issue to git_cl recipe module

Depends on https://codereview.chromium.org/2088343002
==========

[tandrii(chromium), 2016-06-23 11:23:50]

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.py
File recipe_modules/git_cl/api.py (right):

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
recipe_modules/git_cl/api.py:58: # message can have unicode, which we can't pass
on the command line, so
On 2016/06/22 22:47:33, martiniss wrote:
> On 2016/06/22 at 14:41:15, tandrii(chromium) wrote:
> > why not pass it in a temp file, or stdin? Patching git_cl for that is
trivial.
> 
> I do that below, in the call to set description. 

oh, well, then why do you even need to clean the message? Just put "this message
will be overwritten later".

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
recipe_modules/git_cl/api.py:62: upload_args.extend(['-m', one_line_message])
s/one_line_message/"whatever"

[martiniss, 2016-06-23 18:35:41]

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.py
File recipe_modules/git_cl/api.py (right):

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
recipe_modules/git_cl/api.py:58: # message can have unicode, which we can't pass
on the command line, so
On 2016/06/23 at 11:23:50, tandrii(chromium) wrote:
> On 2016/06/22 22:47:33, martiniss wrote:
> > On 2016/06/22 at 14:41:15, tandrii(chromium) wrote:
> > > why not pass it in a temp file, or stdin? Patching git_cl for that is
trivial.
> > 
> > I do that below, in the call to set description. 
> 
> oh, well, then why do you even need to clean the message? Just put "this
message will be overwritten later".

It sends out an email immediately, which has the subject of the message, and
which I believe stays if you send out email later. I'll double check that
though.

[martiniss, 2016-06-23 18:44:15]

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.py
File recipe_modules/git_cl/api.py (right):

https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
recipe_modules/git_cl/api.py:58: # message can have unicode, which we can't pass
on the command line, so
On 2016/06/23 at 18:35:41, martiniss wrote:
> On 2016/06/23 at 11:23:50, tandrii(chromium) wrote:
> > On 2016/06/22 22:47:33, martiniss wrote:
> > > On 2016/06/22 at 14:41:15, tandrii(chromium) wrote:
> > > > why not pass it in a temp file, or stdin? Patching git_cl for that is
trivial.
> > > 
> > > I do that below, in the call to set description. 
> > 
> > oh, well, then why do you even need to clean the message? Just put "this
message will be overwritten later".
> 
> It sends out an email immediately, which has the subject of the message, and
which I believe stays if you send out email later. I'll double check that
though.

Look at https://codereview.chromium.org/2098603002. I uploaded with `git cl
upload -m "whatever"`, went online and sent a message, then did `git cl
description` and changed the message. The original title `whatever` is still
what what's used in the email, which I think we'd like to avoid.

[tandrii(chromium), 2016-06-23 19:10:54]

On 2016/06/23 18:44:15, martiniss wrote:
> https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.py
> File recipe_modules/git_cl/api.py (right):
> 
>
https://codereview.chromium.org/2087093002/diff/1/recipe_modules/git_cl/api.p...
> recipe_modules/git_cl/api.py:58: # message can have unicode, which we can't
pass
> on the command line, so
> On 2016/06/23 at 18:35:41, martiniss wrote:
> > On 2016/06/23 at 11:23:50, tandrii(chromium) wrote:
> > > On 2016/06/22 22:47:33, martiniss wrote:
> > > > On 2016/06/22 at 14:41:15, tandrii(chromium) wrote:
> > > > > why not pass it in a temp file, or stdin? Patching git_cl for that is
> trivial.
> > > > 
> > > > I do that below, in the call to set description. 
> > > 
> > > oh, well, then why do you even need to clean the message? Just put "this
> message will be overwritten later".
> > 
> > It sends out an email immediately, which has the subject of the message, and
> which I believe stays if you send out email later. I'll double check that
> though.
> 
> Look at https://codereview.chromium.org/2098603002. I uploaded with `git cl
> upload -m "whatever"`, went online and sent a message, then did `git cl
> description` and changed the message. The original title `whatever` is still
> what what's used in the email, which I think we'd like to avoid.
You are right (though you can avoid sending email on upload, and send it
afterwards).
How about this to avoid the hack: https://codereview.chromium.org/2087393004/

[martiniss, 2016-06-23 19:35:47]

Ok, new patch up. Needs https://codereview.chromium.org/2095663003 to land this.

[tandrii(chromium), 2016-06-23 19:53:54]

LGTM % but I don't understand why latin1 and not utf8

https://codereview.chromium.org/2087093002/diff/20001/recipe_modules/git_cl/e...
File recipe_modules/git_cl/example.py (right):

https://codereview.chromium.org/2087093002/diff/20001/recipe_modules/git_cl/e...
recipe_modules/git_cl/example.py:1: # -*- coding: latin-1 -*-
why not utf8?

[martiniss, 2016-06-23 20:41:05]

https://codereview.chromium.org/2087093002/diff/20001/recipe_modules/git_cl/e...
File recipe_modules/git_cl/example.py (right):

https://codereview.chromium.org/2087093002/diff/20001/recipe_modules/git_cl/e...
recipe_modules/git_cl/example.py:1: # -*- coding: latin-1 -*-
On 2016/06/23 at 19:53:54, tandrii(chromium) wrote:
> why not utf8?

Blind copy-pasta. Makes much more sense

[tandrii(chromium), 2016-06-23 20:56:06]

lgtm

[martiniss, 2016-06-27 20:50:32]

The CQ bit was checked by martiniss@chromium.org

[commit-bot: I haz the power, 2016-06-27 20:50:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 20:58:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-27 20:58:30]

Try jobs failed on following builders:
  Depot Tools Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)

[martiniss, 2016-06-28 19:06:45]

The CQ bit was checked by martiniss@chromium.org

[martiniss, 2016-06-28 19:06:45]

The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org
Link to the patchset: https://codereview.chromium.org/2087093002/#ps60001
(title: "Re-gen expectations")

[commit-bot: I haz the power, 2016-06-28 19:06:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 19:10:28]

Description was changed from

==========
Add upload and issue to git_cl recipe module

Depends on https://codereview.chromium.org/2088343002
==========

to

==========
Add upload and issue to git_cl recipe module

Depends on https://codereview.chromium.org/2088343002

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/808d7ea4f6feeb...
==========

[commit-bot: I haz the power, 2016-06-28 19:10:29]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/808d7ea4f6feeb...