When switching to/from a cache git dir, try to reuse as much data as possible.

When switching to/from a cache git dir, try to reuse as much data as possible.

R=szager@chromium.org
BUG=

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

[iannucci, 2013-07-12 23:28:57]

On 2013/07/12 00:11:53, iannucci wrote:

PTAL when you get a chance :)

[iannucci, 2013-07-12 23:32:08]

https://codereview.chromium.org/18768004/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/18768004/diff/1/gclient_scm.py#newcode413
gclient_scm.py:413: os.remove(altfile)
Actually, we should do this in the non-switch case as well, in case we did the
url rewrite, but haven't broken the dependency yet.

[iannucci, 2013-07-16 02:12:16]

PTAL

[M-A Ruel, 2013-07-16 13:50:03]

Personally, I'm quite wary of not having any test. There's just so many things
that could go wrong.

https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py#newcode385
gclient_scm.py:385: return_early = False
Use try/finally instead.

https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py#newcode760
gclient_scm.py:760: self.checkout_path, '.git', 'objects', 'info', 'alternates')
+4

[iannucci, 2013-07-16 18:21:38]

On 2013/07/16 13:50:03, Marc-Antoine Ruel wrote:
> Personally, I'm quite wary of not having any test. There's just so many things
> that could go wrong.
> 
> https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py
> File gclient_scm.py (right):
> 
> https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py#newcode385
> gclient_scm.py:385: return_early = False
> Use try/finally instead.
> 
> https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py#newcode760
> gclient_scm.py:760: self.checkout_path, '.git', 'objects', 'info',
'alternates')
> +4

I'll see if I can whip up a test for this cache behavior. The gclient testing
framework is pretty... intense...

[M-A Ruel, 2013-07-16 18:23:06]

On 2013/07/16 18:21:38, iannucci wrote:
> On 2013/07/16 13:50:03, Marc-Antoine Ruel wrote:
> I'll see if I can whip up a test for this cache behavior. The gclient testing
> framework is pretty... intense...

I've learned a lot since I wrote it. Feel free to refactor as much as needed.

[szager1, 2013-07-16 21:25:24]

https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py#newcode814
gclient_scm.py:814: # If the clone has an object dependency on the existing
repo, break it
Would this code path ever get exercised for an existing cache dir?  Or can it be
added to the 'True' branch of the previous 'if' clause (and 'do_fetch'
eliminated)?

[iannucci, 2013-07-16 21:47:05]

https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py#newcode814
gclient_scm.py:814: # If the clone has an object dependency on the existing
repo, break it
On 2013/07/16 21:25:24, szager1 wrote:
> Would this code path ever get exercised for an existing cache dir?  Or can it
be
> added to the 'True' branch of the previous 'if' clause (and 'do_fetch'
> eliminated)?

So, this path would get executed if:
  * gclient sync (no cache dir)
  * gclient sync (cache dir). Ctrl-C, crash, etc.
  * (cache-dir exists with linkage)

[szager1, 2013-07-16 21:49:21]

On 2013/07/16 21:47:05, iannucci wrote:
> https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py
> File gclient_scm.py (right):
> 
> https://codereview.chromium.org/18768004/diff/7001/gclient_scm.py#newcode814
> gclient_scm.py:814: # If the clone has an object dependency on the existing
> repo, break it
> On 2013/07/16 21:25:24, szager1 wrote:
> > Would this code path ever get exercised for an existing cache dir?  Or can
it
> be
> > added to the 'True' branch of the previous 'if' clause (and 'do_fetch'
> > eliminated)?
> 
> So, this path would get executed if:
>   * gclient sync (no cache dir)
>   * gclient sync (cache dir). Ctrl-C, crash, etc.
>   * (cache-dir exists with linkage)

Fair enough.

lgtm

[iannucci, 2013-07-16 22:15:12]

@M-A: I think I'm going to pass on the test for now... even the smoketests are
pretty inscrutable, and I don't want to spend a ton of time refactoring them.

That said, I think we could get good mileage out of a test framework which
treats gclient as a black box, and takes snapshots of the filesystem tree at
given points in time. Something that would look like:

def SomeTest():
  gclient('config', '--spec', ...)
  assertFileContents(root, '.gclient')
  gclient('sync')
  assertTree(root)
  ...
  manipulate files, run gclient commands, assert more things
  ...

Then whenever the assert functions are hit, they check against some checked-in
expectation files. If you run the test in 'train' mode, it would just record
over the expectations instead (meaning that a svn/git diff will show you what
changed). Each test case could be run in parallel with different root
directories, and the framework could provide easy/quick ways to set up local
'upstream' repos.

Such a system would also be usable for other SCM wrappers which we may write in
the future as well, since it doesn't depend on mocking anything.

I can work on such a system, but I do not have time to work on it right now.

[M-A Ruel, 2013-07-16 22:26:36]

On 2013/07/16 22:15:12, iannucci wrote:
> @M-A: I think I'm going to pass on the test for now... even the smoketests are
> pretty inscrutable, and I don't want to spend a ton of time refactoring them.
> 
> That said, I think we could get good mileage out of a test framework which
> treats gclient as a black box, and takes snapshots of the filesystem tree at
> given points in time. Something that would look like:
> 
> def SomeTest():
>   gclient('config', '--spec', ...)
>   assertFileContents(root, '.gclient')
>   gclient('sync')
>   assertTree(root)
>   ...
>   manipulate files, run gclient commands, assert more things
>   ...
> 
> Then whenever the assert functions are hit, they check against some checked-in
> expectation files. If you run the test in 'train' mode, it would just record
> over the expectations instead (meaning that a svn/git diff will show you what
> changed). Each test case could be run in parallel with different root
> directories, and the framework could provide easy/quick ways to set up local
> 'upstream' repos.
> 
> Such a system would also be usable for other SCM wrappers which we may write
in
> the future as well, since it doesn't depend on mocking anything.
> 
> I can work on such a system, but I do not have time to work on it right now.

Well, beside the training part, what you describe is what GClientSmokeBase does.
GClientSmokeSVN sets up a svn repository, GClientSmokeGIT sets up a git
repository, GClientSmokeBoth sets up both, which is useful to make sure
interaction between a svn and git dependency works.

GClientSmokeGIT.testRecurse is a good example.

FakeRepos.populateGit() in testing_support/fake_repos.py can be overridden if
the default repositories created does not fit your needs.

[iannucci, 2013-07-16 23:31:46]

On 2013/07/16 22:26:36, Marc-Antoine Ruel wrote:
> On 2013/07/16 22:15:12, iannucci wrote:
> > @M-A: I think I'm going to pass on the test for now... even the smoketests
are
> > pretty inscrutable, and I don't want to spend a ton of time refactoring
them.
> > 
> > That said, I think we could get good mileage out of a test framework which
> > treats gclient as a black box, and takes snapshots of the filesystem tree at
> > given points in time. Something that would look like:
> > 
> > def SomeTest():
> >   gclient('config', '--spec', ...)
> >   assertFileContents(root, '.gclient')
> >   gclient('sync')
> >   assertTree(root)
> >   ...
> >   manipulate files, run gclient commands, assert more things
> >   ...
> > 
> > Then whenever the assert functions are hit, they check against some
checked-in
> > expectation files. If you run the test in 'train' mode, it would just record
> > over the expectations instead (meaning that a svn/git diff will show you
what
> > changed). Each test case could be run in parallel with different root
> > directories, and the framework could provide easy/quick ways to set up local
> > 'upstream' repos.
> > 
> > Such a system would also be usable for other SCM wrappers which we may write
> in
> > the future as well, since it doesn't depend on mocking anything.
> > 
> > I can work on such a system, but I do not have time to work on it right now.
> 
> Well, beside the training part, what you describe is what GClientSmokeBase
does.
> GClientSmokeSVN sets up a svn repository, GClientSmokeGIT sets up a git
> repository, GClientSmokeBoth sets up both, which is useful to make sure
> interaction between a svn and git dependency works.
> 
> GClientSmokeGIT.testRecurse is a good example.
> 
> FakeRepos.populateGit() in testing_support/fake_repos.py can be overridden if
> the default repositories created does not fit your needs.

The expectations are really not at all clear to me. I failed to find a readme or
some sort of documentation for the testing harnesses, so I don't really
understand how they work (and the docstrings aren't terribly helpful either:
"""This is vaguely inspired by twistd.""" isn't too helpful, for example). 
There are many layers of indirection and inheritance which make it difficult to
grok the flow of even a single test case. I've spent a couple hours looking at
them now, and I feel like I'll need to stare at them for another couple hours
before I really understand how they work. :-/

Additionally, all the tests must be run serially (because they appear to all
share the same root directory?).

The training is a pretty essential part of a testing framework. It means the
test-writer doesn't need to understand the intermediate format for expectations,
and only needs to do minimal work to maintain them (i.e. they run the test in
train mode). It means that the test writer only needs to express /that/ they
care about the state of the FS tree at some point in the test, not that they
care about what the particular state is (they can view the state in the
auto-generated expectations and verify its correctness that way).

So, I don't think what I described is actually like what GClientSmokeBase does
at all.

I would like to build the new framework though, I just can't do it right away :)

[iannucci, 2013-07-17 02:31:39]

OK, one last look, I promise. Aaron pointed out that there was a missed
opportunity in the case:
  * Turn cache on + sync
  * Turn cache off + sync
  * <time passes, syncs happen, cache is dormant>
  * Turn cache on

In the previous patch, it would just do a fetch from the remote. In the new
patch, it adds the local one as a reference, and then does the fetch. This can't
hurt anything, and could save quite a bit of bandwidth.

That said, I'm not sure how common it will be. On the upside, it makes the
functionality more symmetric between cache clones and cache fetches.

[szager1, 2013-07-17 21:43:06]

https://codereview.chromium.org/18768004/diff/28001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/18768004/diff/28001/gclient_scm.py#newcode833
gclient_scm.py:833: with open(altfile) as f:
open(altfile, 'w')

[iannucci, 2013-07-17 21:52:03]

yes, trying it locally works, after the file is writable.

FWIW, I did test it in an intermediate refactor, but omitted the 'w' for the
final patch.

*mumbles something about tests*

https://codereview.chromium.org/18768004/diff/28001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/18768004/diff/28001/gclient_scm.py#newcode833
gclient_scm.py:833: with open(altfile) as f:
On 2013/07/17 21:43:06, szager1 wrote:
> open(altfile, 'w')

>_< ... good catch

[commit-bot: I haz the power, 2013-07-17 21:54:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/iannucci@chromium.org/18768004/34001

[commit-bot: I haz the power, 2013-07-17 21:56:50]

Change committed as 212140