gclient_scm.py: Make working with git more reliable

gclient_scm.py: Make working with git more reliable

I found including a git repo in my DEPS file to be unreliable, esp
since I pinning to a specific commit. Whenever I changed the commit
in the DEPS file, gclient would attempt to do a rebase and this was
failing due to how rebase was being invoked.

While investigating the problem, I decided it might be better to take
a different approach. Namely, when cloning gclient should just checkout
the working tree to a detached HEAD. In this way, gclient can more
easily determine if the user has made any changes in the cloned repo.
Future updates (as long as there are no changes) become a much simpler
operation w/no need to invoke rebase.

This is a series of five commits, but sadly, git cl will squash them
into this single review. Here are the original commit messages:

commit 8cd2213f006a6f4b3f6b8c448a1362b9410d47f1
Author: Jay Soffian <jaysoffian@gmail.com>;
Date:   Wed Apr 14 18:29:18 2010 -0400

    Use rev-parse to determine current branch
    
    Git branch is a so-called porcelain and its output cannot be relied upon;
    use git rev-parse instead.

 gclient_scm.py |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

commit 1a09e04554acfa2671f9588ee9eef0bdbe677ed2
Author: Jay Soffian <jaysoffian@gmail.com>;
Date:   Wed Apr 14 22:16:53 2010 -0400

    Detached HEAD does not always imply rebasing; use an _IsRebasing()
    function instead.

 gclient_scm.py |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

commit 45308a58c3f1e30b760f13abe3a6288267265fa8
Author: Jay Soffian <jaysoffian@gmail.com>;
Date:   Wed Apr 14 22:19:10 2010 -0400

    Clarify comments to use common git terminology

 gclient_scm.py |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

commit 5e5a661b7dd9c83b2c9c35950f3267d15b7e840a
Author: Jay Soffian <jaysoffian@gmail.com>;
Date:   Tue May 4 12:15:40 2010 -0400

    Make CaptureStatus use GetUpstreamBranch() instead of assuming 'origin'

 scm.py |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

commit 42a8bfebd2e1b1be01025c1324d75920ac6eb0e1
Author: Jay Soffian <jaysoffian@gmail.com>;
Date:   Wed Apr 14 22:19:29 2010 -0400

    Use a detached HEAD when checking out a tag or commit
    
    After cloning, if a tag or commit was specified, leave a detached HEAD. This way
    we can reliably detect if the user changed the working tree (since HEAD would no
    longer be detached). Further, this simplifies the code path when the dependency
    is updated to a new tag/commit. As long as HEAD is detached when we update, we
    simply checkout whatever we fetched w/o needing to worry about rebasing.

 gclient_scm.py            |  126 +++++++++++++++++++++++++++++++-------------
 tests/gclient_scm_test.py |    6 +--
 2 files changed, 91 insertions(+), 41 deletions(-)

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

[Nasser Grainawi, 2010-04-15 15:15:05]

Changing the default to a detached HEAD is a big workflow change as people will
likely forget to do 'git checkout -b working_branch origin/master'. This was
certainly the case with users trying to use repo the first time.

That said, this does make working with git much more reliable.

_CheckDetachedHead and _isRebasing might belong in scm.py.

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

http://codereview.chromium.org/1652007/diff/1/2#newcode562
gclient_scm.py:562: def _isRebasing(self):
Seems naming convention would suggest '_IsRebasing'.

[jaysoffian, 2010-04-15 15:22:04]

On 2010/04/15 15:15:05, Nasser Grainawi wrote:
> Changing the default to a detached HEAD is a big workflow change as people
will
> likely forget to do 'git checkout -b working_branch origin/master'. This was
> certainly the case with users trying to use repo the first time.

Okay, how about making a detached HEAD the default only if a tag or hash is
given as the revision, otherwise keep the existing behavior. And/or add a
"--detach-head" switch?

> That said, this does make working with git much more reliable.

Indeed, it seems to me that the current code is trying to be much too clever.

> _CheckDetachedHead and _isRebasing might belong in scm.py.

Hmpfh, I never know what to do with "might" comments, so I usually ignore them.
:-)

> http://codereview.chromium.org/1652007/diff/1/2
> File gclient_scm.py (right):
> 
> http://codereview.chromium.org/1652007/diff/1/2#newcode562
> gclient_scm.py:562: def _isRebasing(self):
> Seems naming convention would suggest '_IsRebasing'.

OK. I'll wait for comments from msb & M-A before submitting an update.

Thanks for the review so far.

[M-A Ruel, 2010-04-15 16:25:07]

I didn't review the code, adding Evan to do an actual control flow analysis.

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

http://codereview.chromium.org/1652007/diff/1/2#newcode227
gclient_scm.py:227: # 2) current branch is tracking a remote branch with local
committed changes,
80 cols

[Evan Martin, 2010-04-15 16:29:26]

I guess whoever wrote the rebase stuff in gclient would be a better reviewer; I
think using it confuses things considerably like it has here.

For reference, take a glance at tools/sync-webkit-git.py for a different
approach to the same problem (pinning to a specific version specified by deps). 
The approach I took there was to say that the branch name "gclient" is special
and that it is the hash that DEPS wanted.  Then when you ask for an update,
there are two broad cases:
- current branch is gclient? then they just want whatever DEPS wanted, update
the branch and check out
- current branch is not gclient? update the gclient branch and warn the person
that they'll need to deal with it appropriately (either by throwing away their
current changes, rebasing, merging etc. the gclient branch)

[jaysoffian, 2010-04-15 16:39:21]

On Thu, Apr 15, 2010 at 12:29 PM,  <evan@chromium.org> wrote:
> I guess whoever wrote the rebase stuff in gclient would be a better
> reviewer; I
> think using it confuses things considerably like it has here.
>
> For reference, take a glance at tools/sync-webkit-git.py for a different
> approach to the same problem (pinning to a specific version specified by
> deps).
> The approach I took there was to say that the branch name "gclient" is
> special
> and that it is the hash that DEPS wanted.  Then when you ask for an update,
> there are two broad cases:
> - current branch is gclient? then they just want whatever DEPS wanted,
> update
> the branch and check out
> - current branch is not gclient? update the gclient branch and warn the
> person
> that they'll need to deal with it appropriately (either by throwing away
> their
> current changes, rebasing, merging etc. the gclient branch)

I agree that workflow is about 1000% better.

I thought about having a specially named branch, but decided a user
might not notice (not everyone puts the branch name in their prompt)
and end up committing to "gclient". Then when gclient updates that
branch it throws away their work w/o warning (of course, a git user
familiar with the reflog could get back the work, but then, a git user
familiar with reflog is unlikely to commit to the wrong branch).

So I figured a detached HEAD was less likely to get committed to by
accident, and further, if it does, I still preserve the unreferenced
commits just in case.

j.

[Evan Martin, 2010-04-15 16:56:47]

On 2010/04/15 16:39:21, jaysoffian wrote:
> On Thu, Apr 15, 2010 at 12:29 PM,  <mailto:evan@chromium.org> wrote:
> > I guess whoever wrote the rebase stuff in gclient would be a better
> > reviewer; I
> > think using it confuses things considerably like it has here.
> >
> > For reference, take a glance at tools/sync-webkit-git.py for a different
> > approach to the same problem (pinning to a specific version specified by
> > deps).
> > The approach I took there was to say that the branch name "gclient" is
> > special
> > and that it is the hash that DEPS wanted. &nbsp;Then when you ask for an
update,
> > there are two broad cases:
> > - current branch is gclient? then they just want whatever DEPS wanted,
> > update
> > the branch and check out
> > - current branch is not gclient? update the gclient branch and warn the
> > person
> > that they'll need to deal with it appropriately (either by throwing away
> > their
> > current changes, rebasing, merging etc. the gclient branch)
> 
> I agree that workflow is about 1000% better.
> 
> I thought about having a specially named branch, but decided a user
> might not notice (not everyone puts the branch name in their prompt)
> and end up committing to "gclient". Then when gclient updates that
> branch it throws away their work w/o warning (of course, a git user
> familiar with the reflog could get back the work, but then, a git user
> familiar with reflog is unlikely to commit to the wrong branch).
> 
> So I figured a detached HEAD was less likely to get committed to by
> accident, and further, if it does, I still preserve the unreferenced
> commits just in case.

That's a good point.  At one point I made gclient just be a tag (so you could do
"git diff gclient" to see what was new on your branch).  I can't remember why I
eventually moved away from that...

[jaysoffian, 2010-04-19 22:29:07]

Other than these two nits, I'm not sure whether anything else is desired based
on the discussion. What say you?

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

http://codereview.chromium.org/1652007/diff/1/2#newcode227
gclient_scm.py:227: # 2) current branch is tracking a remote branch with local
committed changes,
On 2010/04/15 16:25:08, Marc-Antoine Ruel wrote:
> 80 cols

Done.

http://codereview.chromium.org/1652007/diff/1/2#newcode562
gclient_scm.py:562: def _isRebasing(self):
On 2010/04/15 15:15:05, Nasser Grainawi wrote:
> Seems naming convention would suggest '_IsRebasing'.

Done.

[Nasser Grainawi, 2010-04-20 20:59:37]

lgtm, but msb should take a look since he did the original rebase code (I
believe).

[Mandeep Singh Baines, 2010-04-20 23:57:50]

LGTM

[Nasser Grainawi, 2010-04-26 22:42:33]

On 2010/04/19 22:29:07, jaysoffian wrote:
> Other than these two nits, I'm not sure whether anything else is desired based
> on the discussion. What say you?
> 

Sorry this sat for so long. Would it be possible for you to fix the one test
that is failing?
You can run the tests with:
python presubmit_support.py

Also, the default message that git prints about a detached head is a bit
undesirable. If users don't set up a tracking branch git-cl will fail to upload
(or perhaps will fail to upload to the right place for non-master branch
uploads). Can we add a nice message somewhere about that?

Also, syncing an existing tree failed for me.
Initialized empty Git repository in
/home/nassser/chromiumos.test/chromiumos.git/src/third_party/kernel-nvidia/.git/
remote: Counting objects: 1410489, done.
remote: Compressing objects: 100% (238908/238908), done.
remote: Total 1410489 (delta 1172114), reused 1399330 (delta 1161517)
Receiving objects: 100% (1410489/1410489), 387.15 MiB | 13.00 MiB/s, done.
Resolving deltas: 100% (1172114/1172114), done.
Checking out files: 100% (32314/32314), done.
error: pathspec 'refs/heads/nvidia-2.6.31.12^0' did not match any file(s) known
to git.
Error: git command checkout returned 1

Thanks,
Nasser

[jaysoffian, 2010-05-05 02:27:13]

- Fixed broken test
- Investigated Nasser's existing tree sync failure but cannot reproduce
- I decided to only detach HEAD for DEPS that specific a tag/commit. When
checking out a branch, I left it as is (do not detach HEAD).
- I refactored _Clone a bit to take advantage of git clone's -b option to
specify the branch to checkout after clonging, and --no-checkout otherwise.

[Mandeep Singh Baines, 2010-05-05 16:23:06]

On 2010/05/05 02:27:13, jaysoffian wrote:
> - Fixed broken test
> - Investigated Nasser's existing tree sync failure but cannot reproduce
> - I decided to only detach HEAD for DEPS that specific a tag/commit. When
> checking out a branch, I left it as is (do not detach HEAD).

Why?

> - I refactored _Clone a bit to take advantage of git clone's -b option to
> specify the branch to checkout after clonging, and --no-checkout otherwise.

[jaysoffian, 2010-05-05 16:38:05]

> > - I decided to only detach HEAD for DEPS that specific a tag/commit. When
> > checking out a branch, I left it as is (do not detach HEAD).
> 
> Why?

This is totally up to you guys as my use case is checking out a specific SHA1.

But I checked out chromiumos.git to sanity check my changes and it seemed to me
that for that repo (where most of the DEPS just default to master), it's a minor
hassle to make the user then need to switch to a branch after the initial clone.

I'm happy to make it work either way though. 

Aside, frankly, the existing code is much much to complicated for my taste… I'd
really like to simplify those (now 5) separate cases down quite a bit … I'll try
to submit a followup patch when I've got the time.

[msb, 2010-05-05 17:06:19]

On Wed, May 5, 2010 at 9:38 AM,  <jaysoffian@gmail.com> wrote:
>> > - I decided to only detach HEAD for DEPS that specific a tag/commit.
>> > When
>> > checking out a branch, I left it as is (do not detach HEAD).
>
>> Why?
>
> This is totally up to you guys as my use case is checking out a specific
> SHA1.
>
> But I checked out chromiumos.git to sanity check my changes and it seemed to
> me
> that for that repo (where most of the DEPS just default to master), it's a
> minor
> hassle to make the user then need to switch to a branch after the initial
> clone.
>
> I'm happy to make it work either way though.
>
> Aside, frankly, the existing code is much much to complicated for my taste…
> I'd
> really like to simplify those (now 5) separate cases down quite a bit … I'll
> try
> to submit a followup patch when I've got the time.
>

Totally agree. I really like the direction you are heading in. Look
forward to follow-up patches:)

> http://codereview.chromium.org/1652007/show
>

[piman, 2010-05-10 23:28:34]

People on this list may want to take a look at an alternative (didn't know about
this one, so I went onto another path):
http://codereview.chromium.org/1787013/show

That other one addresses the problem differently, by keeping the synced hash as
a branch-specific config option, that allows rebase if one commits on top of the
sync point. It's sorta equivalent to Evan's approach, without hard-coding a
branch name.

I'm not particularly attached to it, but I think it allows more traditional
workflows.

[Nasser Grainawi, 2010-05-28 15:51:08]

I've been testing this out for over a week and I think it's pretty solid. Any
reason not to merge it? (It does need a rebase onto the tip, and probably needs
modification to the new smoke tests).

[M-A Ruel, 2010-05-28 15:55:50]

It didn't apply cleanly here so I couldn't try it. I can rubberstamp it but I'd
prefer to have my remaining patches in first.

http://codereview.chromium.org/1652007/diff/16001/17003
File tests/gclient_scm_test.py (right):

http://codereview.chromium.org/1652007/diff/16001/17003#newcode697
tests/gclient_scm_test.py:697: exception = \
Please use () instead.

exception = ('\n...'
             'foo'
             'bar')

above too.

[Nasser Grainawi, 2010-05-28 17:21:26]

maruel@chromium.org wrote:
> It didn't apply cleanly here so I couldn't try it. I can rubberstamp it 
> but I'd
> prefer to have my remaining patches in first.
> 
> 
> http://codereview.chromium.org/1652007/diff/16001/17003
> File tests/gclient_scm_test.py (right):
> 
> http://codereview.chromium.org/1652007/diff/16001/17003#newcode697
> tests/gclient_scm_test.py:697: exception = \
> Please use () instead.
> 
> exception = ('\n...'
>              'foo'
>              'bar')
> 
> above too.
> 
> http://codereview.chromium.org/1652007/show

You can take jay's version of the conflict in gclient_scm as-is and it 
should work.

I'd run the smoke tests for you, but I'm getting this error:
$ ./tests/gclient_smoketest.py
Traceback (most recent call last):
   File "./tests/gclient_smoketest.py", line 656, in <module>
     FAKE.setUp()
   File 
"/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", 
line 126, in setUp
     self.setUpSVN()
   File 
"/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", 
line 178, in setUpSVN
     self.populateSvn()
   File 
"/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", 
line 227, in populateSvn
     }""" % { 'host': '127.0.0.1' }))
   File 
"/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", 
line 348, in _commit_svn
     commit_svn(self.svn_root)
   File 
"/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", 
line 82, in commit_svn
     rev = re.search(r'revision (\d+).', out).group(1)
AttributeError: 'NoneType' object has no attribute 'group'

[M-A Ruel, 2010-05-28 17:24:23]

Le 28 mai 2010 13:21, Nasser Grainawi <nasser@codeaurora.org> a écrit :
> maruel@chromium.org wrote:
>>
>> It didn't apply cleanly here so I couldn't try it. I can rubberstamp it
>> but I'd
>> prefer to have my remaining patches in first.
>>
>>
>> http://codereview.chromium.org/1652007/diff/16001/17003
>> File tests/gclient_scm_test.py (right):
>>
>> http://codereview.chromium.org/1652007/diff/16001/17003#newcode697
>> tests/gclient_scm_test.py:697: exception = \
>> Please use () instead.
>>
>> exception = ('\n...'
>>             'foo'
>>             'bar')
>>
>> above too.
>>
>> http://codereview.chromium.org/1652007/show
>
> You can take jay's version of the conflict in gclient_scm as-is and it
> should work.
>
> I'd run the smoke tests for you, but I'm getting this error:
> $ ./tests/gclient_smoketest.py
> Traceback (most recent call last):
>  File "./tests/gclient_smoketest.py", line 656, in <module>
>    FAKE.setUp()
>  File
> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", line
> 126, in setUp
>    self.setUpSVN()
>  File
> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", line
> 178, in setUpSVN
>    self.populateSvn()
>  File
> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", line
> 227, in populateSvn
>    }""" % { 'host': '127.0.0.1' }))
>  File
> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", line
> 348, in _commit_svn
>    commit_svn(self.svn_root)
>  File
> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", line
> 82, in commit_svn
>    rev = re.search(r'revision (\d+).', out).group(1)
> AttributeError: 'NoneType' object has no attribute 'group'

Can you print the actual ouput of stdout and stderr at line 79?

[Nasser Grainawi, 2010-05-28 17:27:19]

Marc-Antoine Ruel wrote:
> Le 28 mai 2010 13:21, Nasser Grainawi <nasser@codeaurora.org> a écrit :
>> maruel@chromium.org wrote:
>>> It didn't apply cleanly here so I couldn't try it. I can rubberstamp it
>>> but I'd
>>> prefer to have my remaining patches in first.
>>>
>>>
>>> http://codereview.chromium.org/1652007/diff/16001/17003
>>> File tests/gclient_scm_test.py (right):
>>>
>>> http://codereview.chromium.org/1652007/diff/16001/17003#newcode697
>>> tests/gclient_scm_test.py:697: exception = \
>>> Please use () instead.
>>>
>>> exception = ('\n...'
>>>             'foo'
>>>             'bar')
>>>
>>> above too.
>>>
>>> http://codereview.chromium.org/1652007/show
>> You can take jay's version of the conflict in gclient_scm as-is and it
>> should work.
>>
>> I'd run the smoke tests for you, but I'm getting this error:
>> $ ./tests/gclient_smoketest.py
>> Traceback (most recent call last):
>>  File "./tests/gclient_smoketest.py", line 656, in <module>
>>    FAKE.setUp()
>>  File
>> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", line
>> 126, in setUp
>>    self.setUpSVN()
>>  File
>> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", line
>> 178, in setUpSVN
>>    self.populateSvn()
>>  File
>> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", line
>> 227, in populateSvn
>>    }""" % { 'host': '127.0.0.1' }))
>>  File
>> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", line
>> 348, in _commit_svn
>>    commit_svn(self.svn_root)
>>  File
>> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py", line
>> 82, in commit_svn
>>    rev = re.search(r'revision (\d+).', out).group(1)
>> AttributeError: 'NoneType' object has no attribute 'group'
> 
> Can you print the actual ouput of stdout and stderr at line 79?

That was much more informative.
svn: Can't connect to host '127.0.0.1': Connection refused
Am I missing a host package for the svn server?

[M-A Ruel, 2010-05-28 17:30:01]

fake_repos.py, line 177 probably failed.

Le 28 mai 2010 13:27, Nasser Grainawi <nasser@codeaurora.org> a écrit :
> Marc-Antoine Ruel wrote:
>>
>> Le 28 mai 2010 13:21, Nasser Grainawi <nasser@codeaurora.org> a écrit :
>>>
>>> maruel@chromium.org wrote:
>>>>
>>>> It didn't apply cleanly here so I couldn't try it. I can rubberstamp it
>>>> but I'd
>>>> prefer to have my remaining patches in first.
>>>>
>>>>
>>>> http://codereview.chromium.org/1652007/diff/16001/17003
>>>> File tests/gclient_scm_test.py (right):
>>>>
>>>> http://codereview.chromium.org/1652007/diff/16001/17003#newcode697
>>>> tests/gclient_scm_test.py:697: exception = \
>>>> Please use () instead.
>>>>
>>>> exception = ('\n...'
>>>>            'foo'
>>>>            'bar')
>>>>
>>>> above too.
>>>>
>>>> http://codereview.chromium.org/1652007/show
>>>
>>> You can take jay's version of the conflict in gclient_scm as-is and it
>>> should work.
>>>
>>> I'd run the smoke tests for you, but I'm getting this error:
>>> $ ./tests/gclient_smoketest.py
>>> Traceback (most recent call last):
>>>  File "./tests/gclient_smoketest.py", line 656, in <module>
>>>   FAKE.setUp()
>>>  File
>>> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py",
>>> line
>>> 126, in setUp
>>>   self.setUpSVN()
>>>  File
>>> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py",
>>> line
>>> 178, in setUpSVN
>>>   self.populateSvn()
>>>  File
>>> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py",
>>> line
>>> 227, in populateSvn
>>>   }""" % { 'host': '127.0.0.1' }))
>>>  File
>>> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py",
>>> line
>>> 348, in _commit_svn
>>>   commit_svn(self.svn_root)
>>>  File
>>> "/local/mnt/workspace/chrome/tools/depot_tools_rw/tests/fake_repos.py",
>>> line
>>> 82, in commit_svn
>>>   rev = re.search(r'revision (\d+).', out).group(1)
>>> AttributeError: 'NoneType' object has no attribute 'group'
>>
>> Can you print the actual ouput of stdout and stderr at line 79?
>
> That was much more informative.
> svn: Can't connect to host '127.0.0.1': Connection refused
> Am I missing a host package for the svn server?
>

[Mandeep Singh Baines, 2010-06-02 04:30:18]

Rebased and tested this change. I get 5 failures for gclient_smoketest.py. We'll
need to fix these before we can commit.

A few of the errors are of this variety:

self.assertEquals(13, len(out))

So these aren't really errors. Just the output text has changed.

[M-A Ruel, 2010-06-02 10:53:31]

On 2010/06/02 04:30:18, Mandeep Singh Baines wrote:
> Rebased and tested this change. I get 5 failures for gclient_smoketest.py.
We'll
> need to fix these before we can commit.
> 
> A few of the errors are of this variety:
> 
> self.assertEquals(13, len(out))
> 
> So these aren't really errors. Just the output text has changed.

Yes you can commit, I need to improve this part. I'm thinking about using a
regexp to split blocks of lines and count the number of blocks. I may need to
disable the % remaining status line to get more consistent results.