git_utils.py: allow checkouts of local repositories at any commithash along master branch

git_utils.py: allow checkouts of local repositories at any commithash along master branch

Needed so that we can create new GM/SKP baselines on a shared
rebaseline_server instance, *efficiently*.  (Create checkouts pinned to certain
commithashes as copies of the local Skia checkout, rather than checking out
copies of the main Skia repo across the internet.)

BUG=skia:1918
NOTREECHECKS=true
R=borenet@google.com

Committed: https://skia.googlesource.com/common/+/5dd2918

[epoger, 2014-08-19 14:23:42]

Eric- PTAL at patchset 1.

[borenet, 2014-08-19 14:30:32]

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py
File py/utils/git_utils.py (right):

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py#newcod...
py/utils/git_utils.py:199: self._run_in_git_root(args=[GIT, 'checkout',
local_branch_name])
Why not just do:
$ git clone <repository>
$ git checkout -b <branch> -t <commithash|upstream_branch>
?

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py#newcod...
py/utils/git_utils.py:217: return self
Shouldn't the bulk of the work be done in __enter__, not __init__?

[epoger, 2014-08-19 15:16:03]

Thanks, Eric.  Please see patchset 2.

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py
File py/utils/git_utils.py (right):

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py#newcod...
py/utils/git_utils.py:199: self._run_in_git_root(args=[GIT, 'checkout',
local_branch_name])
On 2014/08/19 14:30:31, borenet wrote:
> Why not just do:
> $ git clone <repository>
> $ git checkout -b <branch> -t <commithash|upstream_branch>
> ?

Seems to work (with some adjustments).  I think the new way is less efficient
for large remote repositories, because "git clone" fetches the history for all
advertised branches.  But if you prefer it this way, it's fine with me.

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py#newcod...
py/utils/git_utils.py:217: return self
On 2014/08/19 14:30:31, borenet wrote:
> Shouldn't the bulk of the work be done in __enter__, not __init__?

Done.

[borenet, 2014-08-19 15:24:28]

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py
File py/utils/git_utils.py (right):

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py#newcod...
py/utils/git_utils.py:199: self._run_in_git_root(args=[GIT, 'checkout',
local_branch_name])
On 2014/08/19 15:16:03, epoger wrote:
> On 2014/08/19 14:30:31, borenet wrote:
> > Why not just do:
> > $ git clone <repository>
> > $ git checkout -b <branch> -t <commithash|upstream_branch>
> > ?
> 
> Seems to work (with some adjustments).  I think the new way is less efficient
> for large remote repositories, because "git clone" fetches the history for all
> advertised branches.  But if you prefer it this way, it's fine with me.

What repository are we checking out?  I think Skia's repo is small enough that
it shouldn't make much difference.  In fact, since everything gets pushed onto
master, the only thing we'd miss are the Chrome release branches.

https://codereview.chromium.org/484143002/diff/20001/py/utils/git_utils.py
File py/utils/git_utils.py (right):

https://codereview.chromium.org/484143002/diff/20001/py/utils/git_utils.py#ne...
py/utils/git_utils.py:155: def __init__(self, repository, commithash='HEAD',
Should the default for commithash be None, since you use "if self._commithash" ?
 "git checkout HEAD" will be a no-op.

https://codereview.chromium.org/484143002/diff/20001/py/utils/git_utils.py#ne...
py/utils/git_utils.py:220: self._run_in_git_root(args=[GIT, 'checkout',
self._commithash])
This doesn't create a new branch like your old code did.  Is that what you want?
 Also, in this case self._commithash can actually be a branch name, tag, or
commit hash.  It may be worth naming it that way.

[epoger, 2014-08-19 15:42:06]

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py
File py/utils/git_utils.py (right):

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py#newcod...
py/utils/git_utils.py:199: self._run_in_git_root(args=[GIT, 'checkout',
local_branch_name])
On 2014/08/19 15:24:28, borenet wrote:
> What repository are we checking out?  I think Skia's repo is small enough that
> it shouldn't make much difference.  In fact, since everything gets pushed onto
> master, the only thing we'd miss are the Chrome release branches.

The primary use case is indeed the Skia repo.  Personally, I think it would be
better to call the component steps rather than "git clone", for efficient
operation if someone *does* use this to check out from a large repo.  But I'm
happy either way.

https://codereview.chromium.org/484143002/diff/20001/py/utils/git_utils.py
File py/utils/git_utils.py (right):

https://codereview.chromium.org/484143002/diff/20001/py/utils/git_utils.py#ne...
py/utils/git_utils.py:155: def __init__(self, repository, commithash='HEAD',
On 2014/08/19 15:24:28, borenet wrote:
> Should the default for commithash be None, since you use "if self._commithash"
?
>  "git checkout HEAD" will be a no-op.

I'd rather make the default behavior explicit, and minimize the number of
different code paths.  So I kept the default of 'HEAD' here and got rid of the
branch within __enter__().

https://codereview.chromium.org/484143002/diff/20001/py/utils/git_utils.py#ne...
py/utils/git_utils.py:220: self._run_in_git_root(args=[GIT, 'checkout',
self._commithash])
On 2014/08/19 15:24:28, borenet wrote:
> This doesn't create a new branch like your old code did.  Is that what you
want?


We don't really need the new branch, so this is fine.  (The self-test exercises
all required functionality, and it still passes.)

>  Also, in this case self._commithash can actually be a branch name, tag, or
> commit hash.  It may be worth naming it that way.

I have followed the lead of "git help checkout" and named this parameter
"commit".

[borenet, 2014-08-19 15:50:20]

LGTM.  I don't particularly care about the clone vs init/fetch/checkout workflow
- I suggested clone because it was fewer commands.

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py
File py/utils/git_utils.py (right):

https://codereview.chromium.org/484143002/diff/1/py/utils/git_utils.py#newcod...
py/utils/git_utils.py:199: self._run_in_git_root(args=[GIT, 'checkout',
local_branch_name])
On 2014/08/19 15:42:05, epoger wrote:
> On 2014/08/19 15:24:28, borenet wrote:
> > What repository are we checking out?  I think Skia's repo is small enough
that
> > it shouldn't make much difference.  In fact, since everything gets pushed
onto
> > master, the only thing we'd miss are the Chrome release branches.
> 
> The primary use case is indeed the Skia repo.  Personally, I think it would be
> better to call the component steps rather than "git clone", for efficient
> operation if someone *does* use this to check out from a large repo.  But I'm
> happy either way.

SGTM.  You're welcome to change it back if you want.

[epoger, 2014-08-19 19:02:46]

Committed patchset #4 manually as 5dd2918 (presubmit successful).