git-cl-upload-hook: improved finding depot_tools algorithm...

git-cl-upload-hook: improved finding depot_tools algorithm

* also allow PATH entry to end in / (which is valid)
* in case depot_tools cannot be located in PATH, search for them using which
* added dependency on subprocess (new in python 2.4)

M      git-cl-upload-hook

TEST=run the hook manually with:
1) PATH containing the depot_tools, depot_tools/ (success)
2) checkout of depot_tools into ~/x, and PATH containing that (success)
3) PATH not containing any copy of depot_tools (fail)
4) Several PATH entries containing gclient, not called depot_tools (success)


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

[M-A Ruel, 2010-06-29 02:27:08]

http://codereview.chromium.org/2852032/diff/1/2
File git-cl-upload-hook (right):

http://codereview.chromium.org/2852032/diff/1/2#newcode20
git-cl-upload-hook:20: # if depot_tools dir is not called depot_tools, or some
other weirdness is involved
please keep it < 80 cols whenever possible.

http://codereview.chromium.org/2852032/diff/1/2#newcode24
git-cl-upload-hook:24: path = Popen(["sh", "-c", "which gclient | head -n 1"],
stdout=PIPE).communicate()[0]
why not subprocess.Popen(['which', 'gclient'],
stdout=subprocess.PIPE).communicate()[0].split()[0] ?

http://codereview.chromium.org/2852032/diff/1/2#newcode25
git-cl-upload-hook:25: if path != "":
if path:

[zbehan, 2010-06-29 03:11:37]

On 2010/06/29 02:27:08, Marc-Antoine Ruel wrote:
> http://codereview.chromium.org/2852032/diff/1/2
> File git-cl-upload-hook (right):
> 
> http://codereview.chromium.org/2852032/diff/1/2#newcode20
> git-cl-upload-hook:20: # if depot_tools dir is not called depot_tools, or some
> other weirdness is involved
> please keep it < 80 cols whenever possible.

All over 80 cols fixed.

> http://codereview.chromium.org/2852032/diff/1/2#newcode24
> git-cl-upload-hook:24: path = Popen(["sh", "-c", "which gclient | head -n 1"],
> stdout=PIPE).communicate()[0]
> why not subprocess.Popen(['which', 'gclient'],
> stdout=subprocess.PIPE).communicate()[0].split()[0] ?

Also possible. I just picked this way, I'll go ahead and change this too to make
it more pythonish.

> http://codereview.chromium.org/2852032/diff/1/2#newcode25
> git-cl-upload-hook:25: if path != "":
> if path:

Fixed.

[zbehan, 2010-06-29 03:13:38]

On 2010/06/29 03:06:19, zbehan wrote:
> 
Meh. Also, ignore the second diff, that shouldn't have gone online.

[zbehan, 2010-06-29 03:23:43]

On 2010/06/29 02:27:08, Marc-Antoine Ruel wrote:
> http://codereview.chromium.org/2852032/diff/1/2#newcode24
> git-cl-upload-hook:24: path = Popen(["sh", "-c", "which gclient | head -n 1"],
> stdout=PIPE).communicate()[0]
> why not subprocess.Popen(['which', 'gclient'],
> stdout=subprocess.PIPE).communicate()[0].split()[0] ?

.split()[0] actually fails with a runtime error if the string is empty. (no
matches)

However, after checking the man page more carefully, the default behaviour of
`which' is to print out just the first match, and -a gives all possible matches.
So actually neither head or split is necessary.



One thing I wasn't entirely sure about is whether I want to keep two methods
doing essentially the same thing, because `which' gives a superset of results of
trying just depot_tools in the path. It will also handle various nutty variants
of depot_tools, like depot_tools/, depot_tools/././, not to mention just about
any directory that contains gclient.

Only argument for keeping both I could come up is using the hook in an
enviroment without the `which' utility, which is something I can't quite
imagine, but who am I to say it doesn't exist.

[M-A Ruel, 2010-06-29 19:53:55]

lgtm with nits

http://codereview.chromium.org/2852032/diff/11001/12001
File git-cl-upload-hook (right):

http://codereview.chromium.org/2852032/diff/11001/12001#newcode8
git-cl-upload-hook:8: from subprocess import Popen,PIPE
from subprocess import Popen, PIPE

http://codereview.chromium.org/2852032/diff/11001/12001#newcode13
git-cl-upload-hook:13: # first parse PATH if there's a "depot_tools" inside
First

http://codereview.chromium.org/2852032/diff/11001/12001#newcode20
git-cl-upload-hook:20: # if depot_tools dir is not called depot_tools, or other
weirdness
If

http://codereview.chromium.org/2852032/diff/11001/12001#newcode22
git-cl-upload-hook:22: # grab a `which gclient' and pick the first line, in case
there's more
Grab

http://codereview.chromium.org/2852032/diff/11001/12001#newcode26
git-cl-upload-hook:26: depot_tools_path = path.replace("/gclient","")
this is a bit fragile but the dude is looking for trouble anyway.

[zbehan, 2010-06-29 20:31:43]

On 2010/06/29 19:53:55, Marc-Antoine Ruel wrote:
> lgtm with nits
> 
> http://codereview.chromium.org/2852032/diff/11001/12001
> File git-cl-upload-hook (right):
> 
> http://codereview.chromium.org/2852032/diff/11001/12001#newcode8
> git-cl-upload-hook:8: from subprocess import Popen,PIPE
> from subprocess import Popen, PIPE
Fixed

> http://codereview.chromium.org/2852032/diff/11001/12001#newcode13
> git-cl-upload-hook:13: # first parse PATH if there's a "depot_tools" inside
> First
Fixed

> http://codereview.chromium.org/2852032/diff/11001/12001#newcode20
> git-cl-upload-hook:20: # if depot_tools dir is not called depot_tools, or
other
> weirdness
> If
Fixed

> http://codereview.chromium.org/2852032/diff/11001/12001#newcode22
> git-cl-upload-hook:22: # grab a `which gclient' and pick the first line, in
case
> there's more
> Grab
Fixed, and slightly changed the comment to reflect the new reasoning

> http://codereview.chromium.org/2852032/diff/11001/12001#newcode26
> git-cl-upload-hook:26: depot_tools_path = path.replace("/gclient","")
> this is a bit fragile but the dude is looking for trouble anyway.

I think this is perfectly fine. `which gclient' gives you either nothing or
something that always ends with "/gclient".
It's true that you could look for some other file, but that's probably not a
major issue.

Also added a realpath call.

[M-A Ruel, 2010-06-29 20:37:23]

lgtm but you did a http checkout