Try harder to determine lastchange in case of git-svn repository with some local changes.

Try harder to determine lastchange in case of git-svn repository with some local changes.

BUG=http://crosbug.com/7254
TEST=Manual

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

[Denis Lagno, 2010-10-04 09:21:52]

please take a look

[Denis Lagno, 2010-10-04 10:42:04]

looks like sgk@ have been inactive this year.
Tossing in more reviewers.

[whywhat, 2010-10-04 11:03:08]

I'm not familiar with this code anyhow and not sure when and where it's
executed. Some general comments though.

http://codereview.chromium.org/3570006/diff/3001/4001
File build/util/lastchange.py (right):

http://codereview.chromium.org/3570006/diff/3001/4001#newcode2
build/util/lastchange.py:2: # Copyright (c) 2009 The Chromium Authors. All
rights reserved.
Could you fix this header to 2010 while you're here?

http://codereview.chromium.org/3570006/diff/3001/4001#newcode63
build/util/lastchange.py:63: awk
\'/^[[:space:]]*git-svn-id:[[:space:]]+[^[:space:]]+@[0-9]+/\
Perhaps, you could rewrite this in Python to avoid dependency on awk, no?

http://codereview.chromium.org/3570006/diff/3001/4001#newcode71
build/util/lastchange.py:71: m = git_re.search(p.stdout.read())
Some code now is duplicated. Perhaps, create an array of command lines to try
and handle them in a loop until id is found or array is iterated to the end?
This contradicts with my previous comment though.

[Denis Lagno, 2010-10-04 12:09:46]

http://codereview.chromium.org/3570006/diff/3001/4001
File build/util/lastchange.py (right):

http://codereview.chromium.org/3570006/diff/3001/4001#newcode2
build/util/lastchange.py:2: # Copyright (c) 2009 The Chromium Authors. All
rights reserved.
On 2010/10/04 11:03:08, whywhat wrote:
> Could you fix this header to 2010 while you're here?

Done.

http://codereview.chromium.org/3570006/diff/3001/4001#newcode63
build/util/lastchange.py:63: awk
\'/^[[:space:]]*git-svn-id:[[:space:]]+[^[:space:]]+@[0-9]+/\
On 2010/10/04 11:03:08, whywhat wrote:
> Perhaps, you could rewrite this in Python to avoid dependency on awk, no?

AWK does not look like a real dependency.  POSIX mandates presence of awk.  It
is ubiquitous everywhere including cygwin.

Problem with rewriting it in python is that proper python implementation will
require more bloat.
Full git log is quite large (26 megabytes ATM), so it is bad to just read all
log as a whole and then parse it.  For example if you simply replace "git log
-1" with "git log" then this script runs in more than second on my machine as
opposed to milliseconds using awk solution.
Solution with awk just reads git log stream until first svn-id line, then
immediately closes pipe and exits.  I afraid python analogue of that will be
more bloated.

http://codereview.chromium.org/3570006/diff/3001/4001#newcode71
build/util/lastchange.py:71: m = git_re.search(p.stdout.read())
On 2010/10/04 11:03:08, whywhat wrote:
> Some code now is duplicated. Perhaps, create an array of command lines to try
> and handle them in a loop until id is found or array is iterated to the end?
> This contradicts with my previous comment though.

Done.

[Denis Lagno, 2010-10-04 12:31:28]

rephrasing: that awk line is entirely optimization detail.  Script will work the
same if you replace "git log | awk ...." with just "git log" except that it will
work order of magnitude slower.

[whywhat, 2010-10-04 14:50:01]

LGTM from me, but please, don't submit until someone more familiar with this
part of repository confirms it.

http://codereview.chromium.org/3570006/diff/3001/4001
File build/util/lastchange.py (right):

http://codereview.chromium.org/3570006/diff/3001/4001#newcode63
build/util/lastchange.py:63: awk
\'/^[[:space:]]*git-svn-id:[[:space:]]+[^[:space:]]+@[0-9]+/\
On 2010/10/04 12:09:46, Denis Lagno wrote:
> On 2010/10/04 11:03:08, whywhat wrote:
> > Perhaps, you could rewrite this in Python to avoid dependency on awk, no?
> 
> AWK does not look like a real dependency.  POSIX mandates presence of awk.  It
> is ubiquitous everywhere including cygwin.
> 
> Problem with rewriting it in python is that proper python implementation will
> require more bloat.
> Full git log is quite large (26 megabytes ATM), so it is bad to just read all
> log as a whole and then parse it.  For example if you simply replace "git log
> -1" with "git log" then this script runs in more than second on my machine as
> opposed to milliseconds using awk solution.
> Solution with awk just reads git log stream until first svn-id line, then
> immediately closes pipe and exits.  I afraid python analogue of that will be
> more bloated.
> 

Ok, sounds reasonable. Thanks for the insight!

[Evan Martin, 2010-10-04 19:03:28]

Why not:

$ git svn info | grep ^Revision
Revision: 61385

[Denis Lagno, 2010-10-05 09:58:37]

> $ git svn info | grep ^Revision
> Revision: 61385

Great.  Unfortunately it does not work really good.

It works perfectly on trunk:

$ git checkout trunk
Switched to branch 'trunk'
$ git svn info | grep ^Revision
Revision: 61351
$ git log | grep svn-id | head -n1 | awk '{ print $2 }' | cut -d@ -f2
61351

So on trunk both ways give identical revision.  However it does not work really
good on some branch that was spawned off from trunk earlier and since then was
merged with updated trunk:

$ git checkout peruserlang 
Switched to branch 'peruserlang'
$ git svn info | grep ^Revision
Revision: 61056
$ git log | grep svn-id | head -n1 | awk '{ print $2 }' | cut -d@ -f2
61351

So, git svn info gives earlier revision: revision from time when branch was
created, not current revision.

[Evan Martin, 2010-10-05 15:51:01]

Ah, bummer!  It seems to query for HEAD explicitly.  That's too bad.
        my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');

Anyway, was hoping to resolve your argument in an easy way.  :)
I also think the awk is weird.  Here's the relevant code in git-cl
that does what you want.
Especially since you have to do the regexp in python anyway (to extra
the revision number) this is probably less code than what you have
anyway.  The critical bit is cutting the pipe so that the log
subprocess stops as soon as you find the git-svn-id line.

       # The -100 is an arbitrary limit so we don't search forever.
        cmd = ['git', 'log', '-100', '--pretty=medium']
        proc = subprocess.Popen(cmd, stdout=subprocess.PIPE)
        for line in proc.stdout:
          match = git_svn_re.match(line)
          if match:
            url = match.group(1)
            if url in svn_refs:
              self.svn_branch = svn_refs[url]
              proc.stdout.close()  # Cut pipe.
              break


On Tue, Oct 5, 2010 at 2:58 AM,  <dilmah@chromium.org> wrote:
>> $ git svn info | grep ^Revision
>> Revision: 61385
>
> Great.  Unfortunately it does not work really good.
>
> It works perfectly on trunk:
>
> $ git checkout trunk
> Switched to branch 'trunk'
> $ git svn info | grep ^Revision
> Revision: 61351
> $ git log | grep svn-id | head -n1 | awk '{ print $2 }' | cut -d@ -f2
> 61351
>
> So on trunk both ways give identical revision.  However it does not work
> really
> good on some branch that was spawned off from trunk earlier and since then
> was
> merged with updated trunk:
>
> $ git checkout peruserlang
> Switched to branch 'peruserlang'
> $ git svn info | grep ^Revision
> Revision: 61056
> $ git log | grep svn-id | head -n1 | awk '{ print $2 }' | cut -d@ -f2
> 61351
>
> So, git svn info gives earlier revision: revision from time when branch was
> created, not current revision.
>
>
> http://codereview.chromium.org/3570006/show
>

[Evan Martin, 2010-10-05 15:52:08]

PS: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming

"Names to Avoid
single character names except for counters or iterators"

Sorry to be picky, but after reading lots of clear code the
single-letter names start sticking out.

On Tue, Oct 5, 2010 at 8:50 AM, Evan Martin <evan@chromium.org> wrote:
> Ah, bummer!  It seems to query for HEAD explicitly.  That's too bad.
>        my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
>
> Anyway, was hoping to resolve your argument in an easy way.  :)
> I also think the awk is weird.  Here's the relevant code in git-cl
> that does what you want.
> Especially since you have to do the regexp in python anyway (to extra
> the revision number) this is probably less code than what you have
> anyway.  The critical bit is cutting the pipe so that the log
> subprocess stops as soon as you find the git-svn-id line.
>
>       # The -100 is an arbitrary limit so we don't search forever.
>        cmd = ['git', 'log', '-100', '--pretty=medium']
>        proc = subprocess.Popen(cmd, stdout=subprocess.PIPE)
>        for line in proc.stdout:
>          match = git_svn_re.match(line)
>          if match:
>            url = match.group(1)
>            if url in svn_refs:
>              self.svn_branch = svn_refs[url]
>              proc.stdout.close()  # Cut pipe.
>              break
>
>
> On Tue, Oct 5, 2010 at 2:58 AM,  <dilmah@chromium.org> wrote:
>>> $ git svn info | grep ^Revision
>>> Revision: 61385
>>
>> Great.  Unfortunately it does not work really good.
>>
>> It works perfectly on trunk:
>>
>> $ git checkout trunk
>> Switched to branch 'trunk'
>> $ git svn info | grep ^Revision
>> Revision: 61351
>> $ git log | grep svn-id | head -n1 | awk '{ print $2 }' | cut -d@ -f2
>> 61351
>>
>> So on trunk both ways give identical revision.  However it does not work
>> really
>> good on some branch that was spawned off from trunk earlier and since then
>> was
>> merged with updated trunk:
>>
>> $ git checkout peruserlang
>> Switched to branch 'peruserlang'
>> $ git svn info | grep ^Revision
>> Revision: 61056
>> $ git log | grep svn-id | head -n1 | awk '{ print $2 }' | cut -d@ -f2
>> 61351
>>
>> So, git svn info gives earlier revision: revision from time when branch was
>> created, not current revision.
>>
>>
>> http://codereview.chromium.org/3570006/show
>>
>

[Denis Lagno, 2010-10-05 16:29:57]

Closing pipe in python works fine.  So got rid of awk.

Also fixed names (they were inherited from original code).

[Evan Martin, 2010-10-05 17:09:29]

http://codereview.chromium.org/3570006/diff/12002/17001
File build/util/lastchange.py (right):

http://codereview.chromium.org/3570006/diff/12002/17001#newcode48
build/util/lastchange.py:48: proc = subprocess.Popen(['git', 'log'],
The reason for the -100 in the command I pasted is that if there aren't any
git-svn-id lines in your tree (for whatever reason, like say you're not in a
git-svn tree), this will run through the entire git log of the code you're
looking at.  Maybe this isn't a concern for how this script is used; I leave it
to your judgement.

http://codereview.chromium.org/3570006/diff/12002/17001#newcode61
build/util/lastchange.py:61: True
I think you want "pass" here instead

[Denis Lagno, 2010-10-05 17:25:27]

http://codereview.chromium.org/3570006/diff/12002/17001
File build/util/lastchange.py (right):

http://codereview.chromium.org/3570006/diff/12002/17001#newcode48
build/util/lastchange.py:48: proc = subprocess.Popen(['git', 'log'],
On 2010/10/05 17:09:29, Evan Martin wrote:
> The reason for the -100 in the command I pasted is that if there aren't any
> git-svn-id lines in your tree (for whatever reason, like say you're not in a
> git-svn tree), this will run through the entire git log of the code you're
> looking at.  Maybe this isn't a concern for how this script is used; I leave
it
> to your judgement.

Done.

http://codereview.chromium.org/3570006/diff/12002/17001#newcode61
build/util/lastchange.py:61: True
On 2010/10/05 17:09:29, Evan Martin wrote:
> I think you want "pass" here instead

Done.
I will learn python this way:)

[Evan Martin, 2010-10-05 17:34:04]

LGTM