Try harder to dig git-svn revision.

Try harder to dig git-svn revision.

Previous behaviour for git-svn repositories is to return git hash (this hash then appears on about:version page and can be copy-pasted into bugreport).
However for developer branches with local modifications this hash bears no information to the outside world.

Now we return svn revision for trunk git-svn checkouts and svn-revision plus "-dirty" suffix for git branches.

The magic required to run git on cygwin is factored out into separate function.

BUG=chromium:70909
TEST=Manual

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

[Denis Lagno, 2011-02-11 19:47:07]

please take a look

[evanm, 2011-02-25 19:06:59]

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

http://codereview.chromium.org/6474044/diff/3001/build/util/lastchange.py#new...
build/util/lastchange.py:62: def RunGitCommand(directory, command, agent):
Can you add docs for this like the other functions?

http://codereview.chromium.org/6474044/diff/3001/build/util/lastchange.py#new...
build/util/lastchange.py:63: command.insert(0, 'git');
Rather than modifying the caller's array, it might be better to do
  command = ['git'] + command

I doubt it matters for this code but it's good style.

http://codereview.chromium.org/6474044/diff/3001/build/util/lastchange.py#new...
build/util/lastchange.py:75: return agent(proc)
Why do this continuation-passing style rather than just "return proc"?  It seems
that would make the control flow clearer.

http://codereview.chromium.org/6474044/diff/3001/build/util/lastchange.py#new...
build/util/lastchange.py:123: git_command = ['svn', 'info', '--url']
This will hang the build if the user has a git clone of the git-svn repo without
having git-svn set up.  This is the setup of many non-committers.

[Denis Lagno, 2011-03-01 12:33:52]

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

http://codereview.chromium.org/6474044/diff/3001/build/util/lastchange.py#new...
build/util/lastchange.py:62: def RunGitCommand(directory, command, agent):
On 2011/02/25 19:06:59, evanm wrote:
> Can you add docs for this like the other functions?

Done.

http://codereview.chromium.org/6474044/diff/3001/build/util/lastchange.py#new...
build/util/lastchange.py:63: command.insert(0, 'git');
On 2011/02/25 19:06:59, evanm wrote:
> Rather than modifying the caller's array, it might be better to do
>   command = ['git'] + command
> 
> I doubt it matters for this code but it's good style.

Done.

http://codereview.chromium.org/6474044/diff/3001/build/util/lastchange.py#new...
build/util/lastchange.py:75: return agent(proc)
On 2011/02/25 19:06:59, evanm wrote:
> Why do this continuation-passing style rather than just "return proc"?  It
seems
> that would make the control flow clearer.

Done.

http://codereview.chromium.org/6474044/diff/3001/build/util/lastchange.py#new...
build/util/lastchange.py:123: git_command = ['svn', 'info', '--url']
On 2011/02/25 19:06:59, evanm wrote:
> This will hang the build if the user has a git clone of the git-svn repo
without
> having git-svn set up.  This is the setup of many non-committers.

this function is never called if IsGitSvn() returned false.
Added explicit check.

[Evan Martin, 2011-03-02 00:03:59]

LGTM

please be prepared to revert or quick-fix when people say you broke them :)

http://codereview.chromium.org/6474044/diff/5002/build/util/lastchange.py
File build/util/lastchange.py (right):

http://codereview.chromium.org/6474044/diff/5002/build/util/lastchange.py#new...
build/util/lastchange.py:170: return LookupGitSVNRevision(directory, 1) is None
I guess elsewhere people call local changes "dirty" as well.  But that will
require extra computation so I don't care too much.

[Denis Lagno, 2011-03-02 13:16:19]

On 2011/03/02 00:03:59, Evan Martin wrote:
> please be prepared to revert or quick-fix when people say you broke them :)

sure

http://codereview.chromium.org/6474044/diff/5002/build/util/lastchange.py
File build/util/lastchange.py (right):

http://codereview.chromium.org/6474044/diff/5002/build/util/lastchange.py#new...
build/util/lastchange.py:170: return LookupGitSVNRevision(directory, 1) is None
On 2011/03/02 00:03:59, Evan Martin wrote:
> I guess elsewhere people call local changes "dirty" as well.  But that will
> require extra computation so I don't care too much.

Current code should mark any local changes as "dirty".

Each commit message in git log is either git-svn tagged or non-tagged.  (I
assume that nobody fakes git-svn tags).

Observation is that if there are any local changes then:
(1) obviously some of commit logs are not git-svn tagged.
(2) moreover, first (most recent) commit message is not git-svn tagged (because
it is either commit message of some local change or commit message of some
merge).

[fta, 2011-03-03 11:36:12]

On 2011/03/02 00:03:59, Evan Martin wrote:
> LGTM
> 
> please be prepared to revert or quick-fix when people say you broke them :)

eh, indeed, it broke my Ubuntu builds:

  ACTION Extracting last change to
/build/buildd/chromium-browser-11.0.690.0~svn20110303r76696/build-tree/src/out/Release/obj/gen/build/LASTCHANGE
out/Release/obj/gen/build/LASTCHANGE.always
Traceback (most recent call last):
  File "./lastchange.py", line 258, in <module>
    sys.exit(main())
  File "./lastchange.py", line 243, in main
    version_info = FetchVersionInfo(opts.default_lastchange)
  File "./lastchange.py", line 194, in FetchVersionInfo
    FetchGitSVNRevision(directory) or FetchGitRevision(directory))
  File "./lastchange.py", line 180, in FetchGitSVNRevision
    revision = LookupGitSVNRevision(directory, 999)
  File "./lastchange.py", line 147, in LookupGitSVNRevision
    if not IsGitSVN(directory):
  File "./lastchange.py", line 118, in IsGitSVN
    return (proc.wait() == 0)
AttributeError: 'NoneType' object has no attribute 'wait'
make[1]: *** [out/Release/obj/gen/build/LASTCHANGE.always] Error 1

The reason is that at build time, i no longer have access to VCS files.
Back in early Aug 2009, agl introduced for me build/LASTCHANGE.in that I
populate with the revision id before the build.

[Denis Lagno, 2011-03-03 12:03:15]

On 2011/03/03 11:36:12, fta wrote:
> > please be prepared to revert or quick-fix when people say you broke them :)
> 
> eh, indeed, it broke my Ubuntu builds:

Thanks for report.
Hopefully this should fix it: http://codereview.chromium.org/6610013/
I've committed it on trunk.