Tweak GenerateDiff:

Tweak GenerateDiff:

* Update comment for accuracy
* Keep the base diff command in a variable; this fixes an oversight where GenerateDiff would run svn diff without the --config-dir hack, and makes later refactoring easy
* Apply review comments from https://codereview.chromium.org/14247007/ to existing code

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

[Peter Kasting, 2013-04-19 20:27:24]

https://codereview.chromium.org/14247007/ part 2.

[Peter Kasting, 2013-04-19 21:16:31]

This change seems to cause broken uploads for deleted files.  Specifically,
without this change, I could upload https://codereview.chromium.org/14188052 ,
but with the change in place, the same patch "failed to upload base files" and
resulted in https://codereview.chromium.org/14375002 .

The most obvious line would be 835:

SVN.Capture(command + ['-r', '%d:head' % rev, srcurl], cwd)):

As someone who doesn't actually know python, I don't know if there's an obvious
bug in this, e.g. "+" isn't the operator I want, or it modifies |command|, or
something.

[iannucci1, 2013-04-19 21:22:11]

On 2013/04/19 21:16:31, Peter Kasting wrote:
> This change seems to cause broken uploads for deleted files.  Specifically,
> without this change, I could upload https://codereview.chromium.org/14188052 ,
> but with the change in place, the same patch "failed to upload base files" and
> resulted in https://codereview.chromium.org/14375002 .
> 
> The most obvious line would be 835:
> 
> SVN.Capture(command + ['-r', '%d:head' % rev, srcurl], cwd)):
> 
> As someone who doesn't actually know python, I don't know if there's an
obvious
> bug in this, e.g. "+" isn't the operator I want, or it modifies |command|, or
> something.

The + would result in the concatenation of the leftside and rightside lists:

['foo', 'bar'] + ['baz'] == ['foo', 'bar', 'baz']

[M-A Ruel, 2013-04-19 21:30:03]

Let's have it committed on Monday since nobody is likely to revert over the
weekend.

https://codereview.chromium.org/14366029/diff/1/scm.py
File scm.py (right):

https://codereview.chromium.org/14366029/diff/1/scm.py#newcode865
scm.py:865: command.append(filename)
you should make a copy of command first:
command = command + [filename]
would do.

Otherwise the next file gets the filename of the previous call. Not good.

[Peter Kasting, 2013-04-19 22:02:51]

https://codereview.chromium.org/14366029/diff/1/scm.py
File scm.py (right):

https://codereview.chromium.org/14366029/diff/1/scm.py#newcode865
scm.py:865: command.append(filename)
On 2013/04/19 21:30:04, Marc-Antoine Ruel wrote:
> you should make a copy of command first:
> command = command + [filename]
> would do.
> 
> Otherwise the next file gets the filename of the previous call. Not good.

Did not realize that in python arguments are effectively passed by reference. 
Changed.  This seems to fix the problems I was having.

[M-A Ruel, 2013-04-19 22:06:45]

lgtm

[commit-bot: I haz the power, 2013-04-19 22:11:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkasting@chromium.org/14366029/7001

[commit-bot: I haz the power, 2013-04-19 22:14:33]

Change committed as 195308