I made git-try also find diffs in one extra gip sub-repository...

I made git-try also find diffs in one extra gip sub-repository
and append them to the patch sent to the try bots. This is 
very useful for chromium+webkit tries.


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

[yaar, 2009-09-21 18:04:00]

Evan, thanks for reviewing this.

[Evan Martin, 2009-09-21 18:08:05]

I think M-A needs to comment on the prefix stuff

http://codereview.chromium.org/212034/diff/1/2
File git-try (right):

http://codereview.chromium.org/212034/diff/1/2#newcode66
Line 66: """Get the diff we'll send to the try server.  We munge paths to match
svn."""
Can you doc what "prefix" means now?  It's not clear from the code.

http://codereview.chromium.org/212034/diff/1/2#newcode80
Line 80: prefix = prefix + '/'
tabbing wrong

http://codereview.chromium.org/212034/diff/1/2#newcode96
Line 96: # Add src/... prefix
I believe src/ is passed in as a parameter for a reason, but you've gone back to
hardcoding it in here.

http://codereview.chromium.org/212034/diff/1/2#newcode110
Line 110: def OneRepositoryDiff(diff_file, patch_names, path, branch):
Docs on this.  Sorry for not having docs on the rest of the code... :(

http://codereview.chromium.org/212034/diff/1/2#newcode169
Line 169: " with")
extra space (you'll get two when the strings concat)

[M-A Ruel, 2009-09-21 18:37:37]

http://codereview.chromium.org/212034/diff/1/2
File git-try (right):

http://codereview.chromium.org/212034/diff/1/2#newcode75
Line 75: difftree_args = ['git', 'diff-tree', '-p']
nit: We usually use 'command' as the variable name.

http://codereview.chromium.org/212034/diff/1/2#newcode79
Line 79: if not prefix.endswith('/'):
I'd rather use posixpath.join()

http://codereview.chromium.org/212034/diff/1/2#newcode86
Line 86: diff = subprocess.Popen(difftree_args,
use cwd parameter.

http://codereview.chromium.org/212034/diff/1/2#newcode110
Line 110: def OneRepositoryDiff(diff_file, patch_names, path, branch):
Add prefix parameter here. No default.

http://codereview.chromium.org/212034/diff/1/2#newcode113
Line 113: print 'calculating diff for %s against %s' % (path, branch)
print('calculating...)

http://codereview.chromium.org/212034/diff/1/2#newcode124
Line 124: diff = GetMungedDiff(branch, prefix=path)
Add current_dir parameter instead of using os.chdir.
Use prefix parameter with posixpath.join().

http://codereview.chromium.org/212034/diff/1/2#newcode170
Line 170: parser.add_option("--webkit",
--root parameter with 'src' as default.

[yaar, 2009-09-21 20:31:39]

Incorporated your feedback. 

Also added --dry_run option, which if found useful for debugging and I guess
other people will find useful too.

[M-A Ruel, 2009-09-21 20:46:53]

http://codereview.chromium.org/212034/diff/5001/2003
File git-try (right):

http://codereview.chromium.org/212034/diff/5001/2003#newcode65
Line 65: def GetMungedDiff(branch, prefix='src/', sub_rep_path=None):
No default parameter since they are always provided. Update doc accordingly.

http://codereview.chromium.org/212034/diff/5001/2003#newcode99
Line 99: for i in range(len(diff)):
for line in diff:

I don't see why you do it this way, unless this is to do in-place modification.

http://codereview.chromium.org/212034/diff/5001/2003#newcode113
Line 113: line = line[0:4] + prefix + line[4:]
line = line[0:4] + posixpath.join(prefix, line[4:])
?

http://codereview.chromium.org/212034/diff/5001/2003#newcode115
Line 115: 
whitespaces

http://codereview.chromium.org/212034/diff/5001/2003#newcode123
Line 123: """Computes a diff for one git repository at a given path
nit: why do you align at 60 cols?

http://codereview.chromium.org/212034/diff/5001/2003#newcode134
Line 134: patch_names.extend([patch_name])
nit: Maybe patch_names should be an return value?

http://codereview.chromium.org/212034/diff/5001/2003#newcode171
Line 171: parser.add_option("--sub_rep",
nit: should both these be simple argument like git behaves instead of named
parameters?

Also this seems quite hackish to me and unclear to the user.

http://codereview.chromium.org/212034/diff/5001/2003#newcode224
Line 224: 
nit: why this empty line?

[Evan Martin, 2009-09-21 20:47:56]

http://codereview.chromium.org/212034/diff/5001/2003
File git-try (right):

http://codereview.chromium.org/212034/diff/5001/2003#newcode67
Line 67: If sub_rep_path is provided, the path is also added to the prefix"""
this comment doesn't make sense -- if that's all it does, why not have the
caller just append it to prefix in the first place?

http://codereview.chromium.org/212034/diff/5001/2003#newcode72
Line 72: print "munging diff for %s %s..." % (sub_rep_path, branch)
extra debugging prints

http://codereview.chromium.org/212034/diff/5001/2003#newcode88
Line 88: # Execute in the currect directory
typo: "correct"

http://codereview.chromium.org/212034/diff/5001/2003#newcode172
Line 172: help="Specify a path to a git sub-repository")
Can you have this explain why you'd want to do that?

http://codereview.chromium.org/212034/diff/5001/2003#newcode223
Line 223: args.extend(['-n', patch_name])
You could just move the definition of args below this point so you don't need to
call .extend() and have the code as it was before.  *shrug*

[yaar, 2009-09-22 00:54:31]

Again, thanks for the feedback. Its all incorporated inside.
1. I discovered that patch names are not calculated right and fixed that.
2. I made sub_rep require 2 arguments and omitted sub_rep_branch.
3. I also made it possible to specify more than 1 sub_rep.
Please review 3rd patch.

[M-A Ruel, 2009-09-22 01:26:23]

lgtm

http://codereview.chromium.org/212034/diff/1002/4
File git-try (right):

http://codereview.chromium.org/212034/diff/1002/4#newcode20
Line 20: return subprocess.Popen(cmd, cwd=cwd,
stdout=subprocess.PIPE).communicate()[0].strip()
80 cols

http://codereview.chromium.org/212034/diff/1002/4#newcode114
Line 114: for i in range(len(diff)):
for line in diff:

http://codereview.chromium.org/212034/diff/1002/4#newcode202
Line 202: if options.sub_rep:
The condition is unnecessary.