Make gclient pack work again by not prefixing "thread_id>"

Make gclient pack work again by not prefixing "thread_id>" 
into stdout and creating the GitDiffFilterer for git repository

BUG=125894
TEST=

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

[Marc-Antoine Ruel (Google), 2012-05-03 00:51:50]

This is not exactly what you want, what you want is to enforce --jobs=1 for both
CMDdiff and CMDpack. This will achieve the same result.

[haitao.feng, 2012-05-03 01:02:23]

On 2012/05/03 00:51:50, Marc-Antoine Ruel (Google) wrote:
> This is not exactly what you want, what you want is to enforce --jobs=1 for
both
> CMDdiff and CMDpack. This will achieve the same result.

Thanks for your review. Yes, by using this option, the modification in
gclient.py is un-necessary. 

Even using --jobs=1, the generated patch may be not valid as the DiffFilterer
for "git" is not relative to the root ("src") directory. I fixed this problem in
gclient_scm.py.

[Marc-Antoine Ruel (Google), 2012-05-03 01:11:18]

On 2012/05/03 01:02:23, haitao.feng wrote:
> On 2012/05/03 00:51:50, Marc-Antoine Ruel (Google) wrote:
> > This is not exactly what you want, what you want is to enforce --jobs=1 for
> both
> > CMDdiff and CMDpack. This will achieve the same result.
> 
> Thanks for your review. Yes, by using this option, the modification in
> gclient.py is un-necessary. 

What I meant is that for these 2 commands, the default should be changed from 8
to 1.

The way in doing this is to do something like
parser.get_option('jobs').default = 1
in CMDdiff and CMDpack before the parse_args call. Not tested.

> Even using --jobs=1, the generated patch may be not valid as the DiffFilterer
> for "git" is not relative to the root ("src") directory. I fixed this problem
in
> gclient_scm.py.

Looking, can you upload a new patchset? Note that it's path 21:00 here so I may
only look at it tomorrow morning.

[Marc-Antoine Ruel (Google), 2012-05-03 01:30:53]

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode23
gclient_scm.py:23: index_string = ""
index_string = None
so the code would fail if not properly overridden.

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode31
gclient_scm.py:31: self._current_file = ""
None

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode32
gclient_scm.py:32: self._replacement_file = ""
self._replacement_file is not really needed, you can just use:
@property
def replacement_file(self):
  return posixpath.join(self._relpath, self._current_file)

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode45
gclient_scm.py:45: if (line.startswith(self.original_prefix) or
Note that this will fail to process git diff with a file move/rename.

You may want to look at patch.py, which handles all this in a much better way.
patch.py already has support to "rebase" a diff to a subdirectory.

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode49
gclient_scm.py:49: 
2 lines

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode56
gclient_scm.py:56: def __init__(self, relpath):
You can remove this constructor entirely. The class will work fine without it.

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode65
gclient_scm.py:65: 
2 lines

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode72
gclient_scm.py:72: def __init__(self, relpath):
Same, remove

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode80
gclient_scm.py:80: result = line.replace("a/" + self._current_file,
self._replacement_file)
that's a bit tricky though, if the file name is in the content.

[haitao.feng, 2012-05-03 09:15:18]

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode23
gclient_scm.py:23: index_string = ""
On 2012/05/03 01:30:53, Marc-Antoine Ruel (Google) wrote:
> index_string = None
> so the code would fail if not properly overridden.

Done.

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode31
gclient_scm.py:31: self._current_file = ""
On 2012/05/03 01:30:53, Marc-Antoine Ruel (Google) wrote:
> None

Done.

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode32
gclient_scm.py:32: self._replacement_file = ""
On 2012/05/03 01:30:53, Marc-Antoine Ruel (Google) wrote:
> self._replacement_file is not really needed, you can just use:
> @property
> def replacement_file(self):
>   return posixpath.join(self._relpath, self._current_file)

Done.

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode45
gclient_scm.py:45: if (line.startswith(self.original_prefix) or
On 2012/05/03 01:30:53, Marc-Antoine Ruel (Google) wrote:
> Note that this will fail to process git diff with a file move/rename.
> 
> You may want to look at patch.py, which handles all this in a much better way.
> patch.py already has support to "rebase" a diff to a subdirectory.

Thanks for the note. Do I need to submit a bug for it? It seems that set_relpath
in patch.py is not used by other commands. How to use the "rebase" functionality
via gclient/gcl/git cl?

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode49
gclient_scm.py:49: 
On 2012/05/03 01:30:53, Marc-Antoine Ruel (Google) wrote:
> 2 lines

Done.

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode56
gclient_scm.py:56: def __init__(self, relpath):
On 2012/05/03 01:30:53, Marc-Antoine Ruel (Google) wrote:
> You can remove this constructor entirely. The class will work fine without it.

Done.

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode65
gclient_scm.py:65: 
On 2012/05/03 01:30:53, Marc-Antoine Ruel (Google) wrote:
> 2 lines

Done.

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode72
gclient_scm.py:72: def __init__(self, relpath):
On 2012/05/03 01:30:53, Marc-Antoine Ruel (Google) wrote:
> Same, remove

Done.

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode80
gclient_scm.py:80: result = line.replace("a/" + self._current_file,
self._replacement_file)
On 2012/05/03 01:30:53, Marc-Antoine Ruel (Google) wrote:
> that's a bit tricky though, if the file name is in the content.

Replace with re.sub("[a|b]/" + self._current_file, self._replacement_file, line)

[M-A Ruel, 2012-05-03 13:37:07]

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10317002/diff/1/gclient_scm.py#newcode45
gclient_scm.py:45: if (line.startswith(self.original_prefix) or
On 2012/05/03 09:15:18, haitao.feng wrote:
> On 2012/05/03 01:30:53, Marc-Antoine Ruel (Google) wrote:
> > Note that this will fail to process git diff with a file move/rename.
> > 
> > You may want to look at patch.py, which handles all this in a much better
way.
> > patch.py already has support to "rebase" a diff to a subdirectory.
> 
> Thanks for the note. Do I need to submit a bug for it? It seems that
set_relpath
> in patch.py is not used by other commands. How to use the "rebase"
functionality
> via gclient/gcl/git cl?

It's used in the commit queue, which is in a separate directory, e.g.
chrome/trunk/tools/commit-queue.

But I realized the set_relpath probably has issues with renames too. :)

So it's fine to have an ad-hoc version here, I don't mind.

http://codereview.chromium.org/10317002/diff/8001/gclient.py
File gclient.py (right):

http://codereview.chromium.org/10317002/diff/8001/gclient.py#newcode1280
gclient.py:1280: parser.add_option('--deps', dest='deps_os', metavar='OS_LIST',
For completeness, I'd prefer to also do right before line 1284:
parser.remove_option('jobs')

otherwise it'd be confusing for the user.

http://codereview.chromium.org/10317002/diff/8001/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10317002/diff/8001/gclient_scm.py#newcode38
gclient_scm.py:38: pass
raise NotImplementedError()

http://codereview.chromium.org/10317002/diff/8001/gclient_scm.py#newcode41
gclient_scm.py:41: pass
raise NotImplementedError()

Or as a matter of fact, I'd use the methods implemented in SvnDiffFilterer. They
are fine as default methods.

http://codereview.chromium.org/10317002/diff/8001/gclient_scm.py#newcode74
gclient_scm.py:74: self._current_file = current_file.split()[0][2:]
current_file.split() will not behave correctly with space in file names, e.g.

D:\src\win\chrome\src>echo hi>"hello world.txt"

D:\src\win\chrome\src>git add "hello world.txt"

D:\src\win\chrome\src>git diff HEAD
diff --git a/hello world.txt b/hello world.txt
new file mode 100644
index 0000000..edf0eff
--- /dev/null
+++ b/hello world.txt
@@ -0,0 +1 @@
+hi^M

You'll have to tune that a bit. Note that in the general case it's not a big
deal as we don't have a lot of files with whitespace in them but we do have a
few.

[haitao.feng, 2012-05-04 00:42:55]

http://codereview.chromium.org/10317002/diff/8001/gclient.py
File gclient.py (right):

http://codereview.chromium.org/10317002/diff/8001/gclient.py#newcode1280
gclient.py:1280: parser.add_option('--deps', dest='deps_os', metavar='OS_LIST',
On 2012/05/03 13:37:07, Marc-Antoine Ruel wrote:
> For completeness, I'd prefer to also do right before line 1284:
> parser.remove_option('jobs')
> 
> otherwise it'd be confusing for the user.

Done.

http://codereview.chromium.org/10317002/diff/8001/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10317002/diff/8001/gclient_scm.py#newcode41
gclient_scm.py:41: pass
On 2012/05/03 13:37:07, Marc-Antoine Ruel wrote:
> raise NotImplementedError()
> 
> Or as a matter of fact, I'd use the methods implemented in SvnDiffFilterer.
They
> are fine as default methods.

Done.

http://codereview.chromium.org/10317002/diff/8001/gclient_scm.py#newcode74
gclient_scm.py:74: self._current_file = current_file.split()[0][2:]
On 2012/05/03 13:37:07, Marc-Antoine Ruel wrote:
> current_file.split() will not behave correctly with space in file names, e.g.
> 
> D:\src\win\chrome\src>echo hi>"hello world.txt"
> 
> D:\src\win\chrome\src>git add "hello world.txt"
> 
> D:\src\win\chrome\src>git diff HEAD
> diff --git a/hello world.txt b/hello world.txt
> new file mode 100644
> index 0000000..edf0eff
> --- /dev/null
> +++ b/hello world.txt
> @@ -0,0 +1 @@
> +hi^M
> 
> You'll have to tune that a bit. Note that in the general case it's not a big
> deal as we don't have a lot of files with whitespace in them but we do have a
> few.
Replace with self._current_file = current_file[:(len(current_file)/2)][2:]

[M-A Ruel, 2012-05-04 02:07:31]

The rest is good.

http://codereview.chromium.org/10317002/diff/13001/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10317002/diff/13001/gclient_scm.py#newcode55
gclient_scm.py:55: """Simple class which tracks which file is being diffed and
Remove the docstring, it's not informative.

http://codereview.chromium.org/10317002/diff/13001/gclient_scm.py#newcode69
gclient_scm.py:69: self._current_file = current_file[:(len(current_file)/2)][2:]
~/src/chrome/src> git mv AUTHORS foo
~/src/chrome/src> git diff HEAD -C
diff --git a/AUTHORS b/foo
similarity index 100%
rename from AUTHORS
rename to foo

:)

So I think you should use:
re.match(r'^diff \-\-git a/(.+?) b/(.+?)$', line)
or something like that.

[haitao.feng, 2012-05-04 04:26:02]

On 2012/05/04 02:07:31, Marc-Antoine Ruel wrote:
> The rest is good.
> 
> http://codereview.chromium.org/10317002/diff/13001/gclient_scm.py
> File gclient_scm.py (right):
> 
> http://codereview.chromium.org/10317002/diff/13001/gclient_scm.py#newcode55
> gclient_scm.py:55: """Simple class which tracks which file is being diffed and
> Remove the docstring, it's not informative.
> 
> http://codereview.chromium.org/10317002/diff/13001/gclient_scm.py#newcode69
> gclient_scm.py:69: self._current_file =
current_file[:(len(current_file)/2)][2:]
> ~/src/chrome/src> git mv AUTHORS foo
> ~/src/chrome/src> git diff HEAD -C
> diff --git a/AUTHORS b/foo
> similarity index 100%
> rename from AUTHORS
> rename to foo
> 
> :)
> 
> So I think you should use:
> re.match(r'^diff \-\-git a/(.+?) b/(.+?)$', line)
> or something like that.

:)

I have thought about this. It does not work if there is a directory ended with "
b", for example:
"diff --git a/c b/a.txt b/c b/a.txt"

I could determine whether it is a "git mv" by looking whether
current_file[:(len(current_file)/2)][2:] is equal to
current_file[(len(current_file)/2 +1):][2:]. The question is how to handle "git
mv" in gclient pack. I could write - all the lines from origin and write + all
the lines to destination file. For the file name, they should be gotten from
"rename from" and "rename to". I will  spend some time on this.

From the codes I read, it seems that the ideal implementation for "gclient pack"
is that a "gclient upload" command could unify the "gcl upload" and "git cl
upload" and provide an option ("pack") to write to local file system.

[haitao.feng, 2012-05-04 09:46:22]

http://codereview.chromium.org/10317002/diff/13001/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/10317002/diff/13001/gclient_scm.py#newcode55
gclient_scm.py:55: """Simple class which tracks which file is being diffed and
On 2012/05/04 02:07:31, Marc-Antoine Ruel wrote:
> Remove the docstring, it's not informative.

Done.

http://codereview.chromium.org/10317002/diff/13001/gclient_scm.py#newcode69
gclient_scm.py:69: self._current_file = current_file[:(len(current_file)/2)][2:]
On 2012/05/04 02:07:31, Marc-Antoine Ruel wrote:
> ~/src/chrome/src> git mv AUTHORS foo
> ~/src/chrome/src> git diff HEAD -C
> diff --git a/AUTHORS b/foo
> similarity index 100%
> rename from AUTHORS
> rename to foo
> 
> :)
> 
> So I think you should use:
> re.match(r'^diff \-\-git a/(.+?) b/(.+?)$', line)
> or something like that.

There is no option "-C" in the git diff command. The command is "git diff `git
merge-base HEAD origin`". So there is no "copy from" or "move from" in the
result. This code should be able to get the current file name correctly. As I
mentioned in the previous comment, "re.match(r'^diff \-\-git a/(.+?) b/(.+?)$',
line) does not work if there is a directory ended with " b".

[M-A Ruel, 2012-05-04 13:02:56]

lgtm, thanks for the patch.

[commit-bot: I haz the power, 2012-05-04 13:03:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haitao.feng@intel.com/10317002/17001

[M-A Ruel, 2012-05-04 13:05:16]

On 2012/05/04 04:26:02, haitao.feng wrote:
> From the codes I read, it seems that the ideal implementation for "gclient
pack"
> is that a "gclient upload" command could unify the "gcl upload" and "git cl
> upload" and provide an option ("pack") to write to local file system.

Note that the gclient pack command is confusing. gclient, unlike repo*, is a
meta checkout tool but doesn't know about code reviews at all. gcl and git-cl
know about code reviews but don't about meta-checkout. There's a clear
separation between the two. As such, gclient pack has never been used much.

* http://source.android.com/source/using-repo.html

[commit-bot: I haz the power, 2012-05-04 13:11:50]

Change committed as 135324