'git try': a) Give users help when they ask for it, b) Don't start try job if...

'git try': a) Give users help when they ask for it, b) Don't start try job if you don't understand the command.

BUG=none
TEST=Make change in local sandbox and confirm proper things happen on "git try help", "git try xyzzy", and "git try -b linux."


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

[Randy Smith (Not in Mondays), 2010-07-29 22:46:33]

Evan, you want to do the review, since you were mocking me for not committing
the fix?  :-}

Also, a question.  Coming up to speed on the depot_tools/SVN sandbox took me
just about as long as I expected, but one thing I still don't get: When I
uploaded, I got the following error.  Is it relevant to this change?  (I don't
think it is, but i'm not sure.)  What's the right response to it?

astibar:../Sandboxen/depot_tools $ gcl upload tryfix
** Presubmit Messages **
1 unit tests failed.

***************
Test 'tests.gcl_unittest' failed with code 1

FFF..............
======================================================================
FAIL: testNew (__main__.CMDuploadUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
1618, in new_method
    func(self, *args, **kwargs)
  File "/home/rdsmith/Sandboxen/depot_tools/tests/gcl_unittest.py", line 220, in
testNew
    gcl.CMDupload(['naame', '-r', 'georges@example.com'])
  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 672, in hook
    return function(change_info, args)
  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 807, in CMDupload
    print "*** Upload does not submit a try; use gcl try to submit a try. ***"
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line 771,
in __call__
    expected_method = self._VerifyMethodCall()
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line 816,
in _VerifyMethodCall
    expected = self._PopNextMethod()
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line 802,
in _PopNextMethod
    raise UnexpectedMethodCallError(self, None)
UnexpectedMethodCallError: Unexpected method call Stub for <open file
'<stdout>', mode 'w' at 0x7eff682f9150>.write('*** Upload does not submit a try;
use gcl try to submit a try. ***') -> None

======================================================================
FAIL: testNormal (__main__.CMDuploadUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
1618, in new_method
    func(self, *args, **kwargs)
  File "/home/rdsmith/Sandboxen/depot_tools/tests/gcl_unittest.py", line 278, in
testNormal
    gcl.CMDupload(['naame', '--no_watchlists'])
  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 672, in hook
    return function(change_info, args)
  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 807, in CMDupload
    print "*** Upload does not submit a try; use gcl try to submit a try. ***"
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line 771,
in __call__
    expected_method = self._VerifyMethodCall()
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line 816,
in _VerifyMethodCall
    expected = self._PopNextMethod()
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line 802,
in _PopNextMethod
    raise UnexpectedMethodCallError(self, None)
UnexpectedMethodCallError: Unexpected method call Stub for <open file
'<stdout>', mode 'w' at 0x7eff682f9150>.write('*** Upload does not submit a try;
use gcl try to submit a try. ***') -> None

======================================================================
FAIL: testServerOverride (__main__.CMDuploadUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
1618, in new_method
    func(self, *args, **kwargs)
  File "/home/rdsmith/Sandboxen/depot_tools/tests/gcl_unittest.py", line 249, in
testServerOverride
    gcl.CMDupload(['naame', '--server=a', '--no_watchlists'])
  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 672, in hook
    return function(change_info, args)
  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 807, in CMDupload
    print "*** Upload does not submit a try; use gcl try to submit a try. ***"
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line 771,
in __call__
    expected_method = self._VerifyMethodCall()
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line 816,
in _VerifyMethodCall
    expected = self._PopNextMethod()
  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line 802,
in _PopNextMethod
    raise UnexpectedMethodCallError(self, None)
UnexpectedMethodCallError: Unexpected method call Stub for <open file
'<stdout>', mode 'w' at 0x7eff682f9150>.write('*** Upload does not submit a try;
use gcl try to submit a try. ***') -> None

----------------------------------------------------------------------
Ran 17 tests in 0.018s

FAILED (failures=3)
***************

Presubmit checks took 3.2s to calculate.
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
Upload server: codereview.chromium.org (change with -s/--server)
Loaded authentication cookies from /home/rdsmith/.codereview_upload_cookies
Issue created. URL: http://codereview.chromium.org/3075009
Uploading base file for trychange.py
Loaded authentication cookies from /home/rdsmith/.codereview_upload_cookies
*** Upload does not submit a try; use gcl try to submit a try. ***
astibar:../Sandboxen/depot_tools $   C-c C-c
astibar:../Sandboxen/depot_tools $

[Evan Martin, 2010-07-29 22:52:26]

http://codereview.chromium.org/3075009/diff/1/2
File trychange.py (right):

http://codereview.chromium.org/3075009/diff/1/2#newcode605
trychange.py:605: if len(args) == 2 and args[0] == 'help':
Why 2 here?  It seems you've updated all of the arg checks by one, but it's not
clear to me why.

http://codereview.chromium.org/3075009/diff/1/2#newcode607
trychange.py:607: return 0
Heh, good catch!

Should we return 1 here?

[Randy Smith (Not in Mondays), 2010-07-29 23:25:08]

While going over your comments I noticed some problems in my initial submission:
I was testing the first argument, not the second, for equality with "help" (see
comments) and I was printing out all arguments, not eliminating the first, when
I was printing the "Huh?" message.  This patch includes fixes for both of those
problems.

http://codereview.chromium.org/3075009/diff/1/2
File trychange.py (right):

http://codereview.chromium.org/3075009/diff/1/2#newcode605
trychange.py:605: if len(args) == 2 and args[0] == 'help':
On 2010/07/29 22:52:26, Evan Martin wrote:
> Why 2 here?  It seems you've updated all of the arg checks by one, but it's
not
> clear to me why.

The args array includes argv[0], i.e. the script itself.  So if I put 
  print "Args: ", ", ".join(args)
inline in the code, I see
astibar:../chrome/src [CookieCrashFixFix] $ git try help
Args:  /home/rdsmith/Sandboxen/depot_tools/git-try, help

I'll put a comment to this effect in the code.

http://codereview.chromium.org/3075009/diff/1/2#newcode607
trychange.py:607: return 0
On 2010/07/29 22:52:26, Evan Martin wrote:
> Heh, good catch!
> 
> Should we return 1 here?

My thought was that we've given the user what they explicitly asked for (help),
so we haven't failed, and thus should have a successful exit status.

(I'll note that the exit status isn't making it back from this function to the
command line; exit status is always zero.  I haven't tracked that down because I
figured it was a different bug, and a much less relevant one than these, but I
can if you want.)

[Evan Martin, 2010-07-29 23:36:19]

LGTM

http://codereview.chromium.org/3075009/diff/5001/6001
File trychange.py (right):

http://codereview.chromium.org/3075009/diff/5001/6001#newcode605
trychange.py:605: # a single argument results in len(args) == 2.
Wow, I have probably written the same bug a million times then!  :(

I knew that argv included the script name, but it seems totally busted to me
that optparse also gives it to you.  :(

[M-A Ruel, 2010-07-30 01:20:40]

Ignore the exception, I'll fix it when I come back from vacation.

Le 29 juillet 2010 15:46, <rdsmith@chromium.org> a écrit :

> Reviewers: Evan Martin,
>
> Message:
> Evan, you want to do the review, since you were mocking me for not
> committing
> the fix?  :-}
>
> Also, a question.  Coming up to speed on the depot_tools/SVN sandbox took
> me
> just about as long as I expected, but one thing I still don't get: When I
> uploaded, I got the following error.  Is it relevant to this change?  (I
> don't
> think it is, but i'm not sure.)  What's the right response to it?
>
> astibar:../Sandboxen/depot_tools $ gcl upload tryfix
> ** Presubmit Messages **
> 1 unit tests failed.
>
> ***************
> Test 'tests.gcl_unittest' failed with code 1
>
> FFF..............
> ======================================================================
> FAIL: testNew (__main__.CMDuploadUnittest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 1618, in new_method
>    func(self, *args, **kwargs)
>  File "/home/rdsmith/Sandboxen/depot_tools/tests/gcl_unittest.py", line
> 220, in
> testNew
>    gcl.CMDupload(['naame', '-r', 'georges@example.com'])
>  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 672, in hook
>    return function(change_info, args)
>  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 807, in CMDupload
>    print "*** Upload does not submit a try; use gcl try to submit a try.
> ***"
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 771,
> in __call__
>    expected_method = self._VerifyMethodCall()
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 816,
> in _VerifyMethodCall
>    expected = self._PopNextMethod()
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 802,
> in _PopNextMethod
>    raise UnexpectedMethodCallError(self, None)
> UnexpectedMethodCallError: Unexpected method call Stub for <open file
> '<stdout>', mode 'w' at 0x7eff682f9150>.write('*** Upload does not submit a
> try;
> use gcl try to submit a try. ***') -> None
>
> ======================================================================
> FAIL: testNormal (__main__.CMDuploadUnittest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 1618, in new_method
>    func(self, *args, **kwargs)
>  File "/home/rdsmith/Sandboxen/depot_tools/tests/gcl_unittest.py", line
> 278, in
> testNormal
>    gcl.CMDupload(['naame', '--no_watchlists'])
>  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 672, in hook
>    return function(change_info, args)
>  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 807, in CMDupload
>    print "*** Upload does not submit a try; use gcl try to submit a try.
> ***"
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 771,
> in __call__
>    expected_method = self._VerifyMethodCall()
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 816,
> in _VerifyMethodCall
>    expected = self._PopNextMethod()
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 802,
> in _PopNextMethod
>    raise UnexpectedMethodCallError(self, None)
> UnexpectedMethodCallError: Unexpected method call Stub for <open file
> '<stdout>', mode 'w' at 0x7eff682f9150>.write('*** Upload does not submit a
> try;
> use gcl try to submit a try. ***') -> None
>
> ======================================================================
> FAIL: testServerOverride (__main__.CMDuploadUnittest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 1618, in new_method
>    func(self, *args, **kwargs)
>  File "/home/rdsmith/Sandboxen/depot_tools/tests/gcl_unittest.py", line
> 249, in
> testServerOverride
>    gcl.CMDupload(['naame', '--server=a', '--no_watchlists'])
>  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 672, in hook
>    return function(change_info, args)
>  File "/home/rdsmith/Sandboxen/depot_tools/gcl.py", line 807, in CMDupload
>    print "*** Upload does not submit a try; use gcl try to submit a try.
> ***"
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 771,
> in __call__
>    expected_method = self._VerifyMethodCall()
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 816,
> in _VerifyMethodCall
>    expected = self._PopNextMethod()
>  File "/home/rdsmith/Sandboxen/depot_tools/third_party/pymox/mox.py", line
> 802,
> in _PopNextMethod
>    raise UnexpectedMethodCallError(self, None)
> UnexpectedMethodCallError: Unexpected method call Stub for <open file
> '<stdout>', mode 'w' at 0x7eff682f9150>.write('*** Upload does not submit a
> try;
> use gcl try to submit a try. ***') -> None
>
> ----------------------------------------------------------------------
> Ran 17 tests in 0.018s
>
> FAILED (failures=3)
> ***************
>
> Presubmit checks took 3.2s to calculate.
> Was the presubmit check useful? Please send feedback & hate mail to
> maruel@chromium.org!
> Upload server: codereview.chromium.org (change with -s/--server)
> Loaded authentication cookies from /home/rdsmith/.codereview_upload_cookies
> Issue created. URL: http://codereview.chromium.org/3075009
> Uploading base file for trychange.py
> Loaded authentication cookies from /home/rdsmith/.codereview_upload_cookies
> *** Upload does not submit a try; use gcl try to submit a try. ***
> astibar:../Sandboxen/depot_tools $   C-c C-c
> astibar:../Sandboxen/depot_tools $
>
> Description:
> 'git try': a) Give users help when they ask for it, b) Don't start try job
> if
> you don't understand the command.
>
>
> Please review this at http://codereview.chromium.org/3075009/show
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/tools/depot_tools/
>
> Affected files:
>  M     trychange.py
>
>
> Index: trychange.py
> ===================================================================
> --- trychange.py        (revision 54207)
> +++ trychange.py        (working copy)
> @@ -600,9 +600,22 @@
>   parser.add_option_group(group)
>
>   options, args = parser.parse_args(argv)
> -  if len(args) == 1 and args[0] == 'help':
> +
> +  # If they've asked for help, give it to them
> +  if len(args) == 2 and args[0] == 'help':
>     parser.print_help()
> +    return 0
>
> +  # If they've said something confusing, don't spawn a try job until you
> +  # understand what they want.
> +  if len(args) > 1:
> +    plural = ""
> +    if len(args) > 2:
> +      plural = "s"
> +    print "Argument%s \"%s\" not understood" % (plural, " ".join(args))
> +    parser.print_help()
> +    return 1
> +
>   LOG_FORMAT = '%(levelname)s %(filename)s(%(lineno)d): %(message)s'
>   if not swallow_exception:
>     if options.verbose == 0:
>
>
>

[Randy Smith (Not in Mondays), 2010-07-30 20:08:33]

On 2010/07/29 23:36:19, Evan Martin wrote:
> http://codereview.chromium.org/3075009/diff/5001/6001#newcode605
> trychange.py:605: # a single argument results in len(args) == 2.
> Wow, I have probably written the same bug a million times then!  :(
> 
> I knew that argv included the script name, but it seems totally busted to me
> that optparse also gives it to you.  :(

I was going completely empirical in my fix, but your comments made me dig a
little deeper.  It turns out that we're doing this to ourselves; the optparse
library is indeed smart in the way you would expect.  Specifically, compare
(from the python docs):

---------
15.5.3.7. Parsing arguments
The whole point of creating and populating an OptionParser is to call its
parse_args() method:

(options, args) = parser.parse_args(args=None, values=None)
where the input parameters are

args
the list of arguments to process (default: sys.argv[1:])
...
-----

with our call:

-----
  options, args = parser.parse_args(argv)
-----

and things become a little clearer :-} :-J.

Gonna just commit it as is anyway--I'm getting tired of polishing the turd :-}.

[viettrungluu, 2010-08-03 20:44:43]

I think this change breaks "git try foobar".

[Evan Martin, 2010-08-03 20:46:18]

On 2010/08/03 20:44:43, viettrungluu wrote:
> I think this change breaks "git try foobar".

Dang, I just used that yesterday and then remembered LGTM'ing this patch and
wondering if it'd continue to work...

[rdsmith_google.com, 2010-08-05 03:28:47]

Sorry about this (and about being out on vacation for a couple of days
after committing it.)  Two questions:
1) What is "git try foobar" supposed to do?  I thought it wasn't any
different from "git try" and hence worth disabling.
2) Has someone reverted this, or are we still in whatever state the
answer to question 1 implies?

-- Randy

On Tue, Aug 3, 2010 at 4:46 PM,  <evan@chromium.org> wrote:
> On 2010/08/03 20:44:43, viettrungluu wrote:
>>
>> I think this change breaks "git try foobar".
>
> Dang, I just used that yesterday and then remembered LGTM'ing this patch and
> wondering if it'd continue to work...
>
> http://codereview.chromium.org/3075009/show
>

[Evan Martin, 2010-08-05 17:12:34]

On Wed, Aug 4, 2010 at 8:28 PM, Randy Smith <rdsmith@google.com> wrote:
> Sorry about this (and about being out on vacation for a couple of days
> after committing it.)  Two questions:
> 1) What is "git try foobar" supposed to do?  I thought it wasn't any
> different from "git try" and hence worth disabling.

It's not a very good UI, but the extra args are args to "git diff".
So "git try foobar" means "try the patch that is the diff of me and
foobar".
It's useful when you have multiple stacked braches:
 git checkout -b b1; hack; commit
 git checkout -b b2; hack; commit
 git try b1   # try just the stuff I did in b2

> 2) Has someone reverted this, or are we still in whatever state the
> answer to question 1 implies?

Not reverted.  If I get around to it I'll fix it.

[rdsmith_google.com, 2010-08-05 18:09:59]

On Thu, Aug 5, 2010 at 1:12 PM, Evan Martin <evan@chromium.org> wrote:
> On Wed, Aug 4, 2010 at 8:28 PM, Randy Smith <rdsmith@google.com> wrote:
>> 2) Has someone reverted this, or are we still in whatever state the
>> answer to question 1 implies?
>
> Not reverted.  If I get around to it I'll fix it.

Sounds good; I'll aim in the same direction (specifically, I'll try to
do this by EOD F if you don't beat me to it.)  If you do fix it, I'd
recommend bailing in some informative fashion if the named branch
doesn't exist and make sure that the meaning of the foobar is listed
in the help output; I think the original behavior was just to toss off
a try job as default if the branch wasn't recognized.

Let me know if you start work on this so I don't have to, and I'll do the same.

-- Randy

[Evan Martin, 2010-08-05 18:19:53]

On Thu, Aug 5, 2010 at 11:09 AM, Randy Smith <rdsmith@google.com> wrote:
> On Thu, Aug 5, 2010 at 1:12 PM, Evan Martin <evan@chromium.org> wrote:
>> On Wed, Aug 4, 2010 at 8:28 PM, Randy Smith <rdsmith@google.com> wrote:
>>> 2) Has someone reverted this, or are we still in whatever state the
>>> answer to question 1 implies?
>>
>> Not reverted.  If I get around to it I'll fix it.
>
> Sounds good; I'll aim in the same direction (specifically, I'll try to
> do this by EOD F if you don't beat me to it.)  If you do fix it, I'd
> recommend bailing in some informative fashion if the named branch
> doesn't exist and make sure that the meaning of the foobar is listed
> in the help output; I think the original behavior was just to toss off
> a try job as default if the branch wasn't recognized.
>
> Let me know if you start work on this so I don't have to, and I'll do the
same.

It's really not too important, I think.  I wouldn't even bother fixing
it.  Maybe Trung will be so inspired.  ;)