Changed the behaviour of '--transitive' in gclient.py to use revision instead of timestamp for iden…

Changed the behaviour of '--transitive' in gclient.py to use revision instead of timestamp for identical repositories.

Here's some background why we need this:
We discovered that google code defines the timestamp of a revision to be the time when a commit was started
rather than when it was finished (apache subversion takes the timestamp when the commit transaction is finished).
This can result in a situation where revision R(i-1) has a higher timestamp than Ri.
See bug: https://code.google.com/p/support/issues/detail?id=30419

When using 'gclient --transitive' we effectively do date-based checkouts.
If a parent has a dependency (without a ...@revision) and that dependency lives in the same repository as the parent does
we'd like to checkout the exact same revision as the parent (if we do a date-based checkout as we do now the google code
bug can result in a situation where we don't get the same revision).

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

[ricow1, 2013-04-12 06:52:58]

Marc-Antoine: Could you take a look, this has been causing us headaches a number
of times due to the google code bug (and there does not seem to be any progress
on that - I have pinged them offline as well)

[ricow1, 2013-04-12 07:01:39]

Dan: FYI

[M-A Ruel, 2013-04-12 15:28:17]

Note that if you don't add a test, you'll be on the hook if it breaks.

https://codereview.chromium.org/13814012/diff/1/gclient.py
File gclient.py (right):

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode303
gclient.py:303: self._used_scm = None
I actually like the idea of keeping an initialized Scm instance per dependency.
Not sure why I didn't think about it before.

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode544
gclient.py:544: def maybeGetParentRevision(self,
keep self on the next line.

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode546
gclient.py:546: """If we are performing an update and --transitive is set, set
the
This sentence is hard to read. Try to capture what it does in a single line with
a short sentence, then an empty, then a paragraph explaining more in details.

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode556
gclient.py:556: assert (self.parent != None and self.parent.used_scm != None)
No need for () and checks for None, just do:
assert self.parent and self.parent.used_scm

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode567
gclient.py:567: print("Not updating revision override from %s to %s. Using "
Use '' instead of "". (We are slowly replacing them to match the coding style)

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode598
gclient.py:598: self._used_scm = gclient_scm.SVNWrapper(parsed_url.GetPath(),
Example of why I dislike multi-line argument alignment not at +4. :)

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode829
gclient.py:829: def used_scm(self):
Add a docstring. It's actually poor that the others don't have one.

[kustermann, 2013-04-15 08:59:13]

PTAL.

> Note that if you don't add a test, you'll be on the hook if it breaks.

I modified the '--transitive' test now to test if '--transitive' uses date or
revision based checkouts. It tests exactly the behaviour we expect.

It required more changes than I thought, but IMHO the test became a lot more
readable.

So I guess we're off the hook now? :-)

https://codereview.chromium.org/13814012/diff/1/gclient.py
File gclient.py (right):

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode544
gclient.py:544: def maybeGetParentRevision(self,
On 2013/04/12 15:28:17, Marc-Antoine Ruel wrote:
> keep self on the next line.

Done.

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode546
gclient.py:546: """If we are performing an update and --transitive is set, set
the
On 2013/04/12 15:28:17, Marc-Antoine Ruel wrote:
> This sentence is hard to read. Try to capture what it does in a single line
with
> a short sentence, then an empty, then a paragraph explaining more in details.

I hope this is better.

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode556
gclient.py:556: assert (self.parent != None and self.parent.used_scm != None)
On 2013/04/12 15:28:17, Marc-Antoine Ruel wrote:
> No need for () and checks for None, just do:
> assert self.parent and self.parent.used_scm

Done.

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode567
gclient.py:567: print("Not updating revision override from %s to %s. Using "
On 2013/04/12 15:28:17, Marc-Antoine Ruel wrote:
> Use '' instead of "". (We are slowly replacing them to match the coding style)

Done.

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode598
gclient.py:598: self._used_scm = gclient_scm.SVNWrapper(parsed_url.GetPath(),
On 2013/04/12 15:28:17, Marc-Antoine Ruel wrote:
> Example of why I dislike multi-line argument alignment not at +4. :)

Done.

https://codereview.chromium.org/13814012/diff/1/gclient.py#newcode829
gclient.py:829: def used_scm(self):
On 2013/04/12 15:28:17, Marc-Antoine Ruel wrote:
> Add a docstring. It's actually poor that the others don't have one.

Done.

https://codereview.chromium.org/13814012/diff/8004/testing_support/fake_repos.py
File testing_support/fake_repos.py (left):

https://codereview.chromium.org/13814012/diff/8004/testing_support/fake_repos...
testing_support/fake_repos.py:647: # static FakeRepos instance. Lazy loaded.
We have two subclasses of 'FakeReposBase' now, so we can't do lazy creation.

[ricow1, 2013-04-15 09:13:32]

LGTM

https://codereview.chromium.org/13814012/diff/8004/gclient.py
File gclient.py (right):

https://codereview.chromium.org/13814012/diff/8004/gclient.py#newcode568
gclient.py:568: # (The timestamps of commits in google code are broken -- which
can
which can can -> which can

https://codereview.chromium.org/13814012/diff/8004/testing_support/fake_repos.py
File testing_support/fake_repos.py (right):

https://codereview.chromium.org/13814012/diff/8004/testing_support/fake_repos...
testing_support/fake_repos.py:682: # So the 'oringin' files will change in every
commit.
oringin -> origin

[M-A Ruel, 2013-04-16 20:28:03]

https://codereview.chromium.org/13814012/diff/18001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/13814012/diff/18001/gclient.py#newcode546
gclient.py:546: """Use revision/timestamp from parent if no explicit revision
was specified.
Uses

https://codereview.chromium.org/13814012/diff/18001/gclient.py#newcode570
gclient.py:570: if self.url.startswith('/'):
Why not do this check because evaluating parent_revision and
parent_revision_date?

https://codereview.chromium.org/13814012/diff/18001/gclient.py#newcode834
gclient.py:834: checked out"""
"""SCMWrapper instance for this dependency or None if not processed yet."""

https://codereview.chromium.org/13814012/diff/18001/testing_support/fake_repo...
File testing_support/fake_repos.py (right):

https://codereview.chromium.org/13814012/diff/18001/testing_support/fake_repo...
testing_support/fake_repos.py:317: # Necessary to be able to change revision
properties
Sadly, this will break tests on Windows. But I don't run the presubmit checks on
Windows often so they could be broken at the moment, since it happens from time
to time.

I'm not sure how to manage that in an efficient way.

https://codereview.chromium.org/13814012/diff/18001/testing_support/fake_repo...
testing_support/fake_repos.py:651: 
2 lines between file-level symbols

https://codereview.chromium.org/13814012/diff/18001/testing_support/fake_repo...
testing_support/fake_repos.py:695: 
here too

https://codereview.chromium.org/13814012/diff/18001/testing_support/fake_repo...
testing_support/fake_repos.py:703: FakeReposTestBase.FAKE_REPOS =
self.FAKE_REPOS_CLASS()
That will significantly slow down the test but oh well.

https://codereview.chromium.org/13814012/diff/18001/tests/gclient_smoketest.py
File tests/gclient_smoketest.py (right):

https://codereview.chromium.org/13814012/diff/18001/tests/gclient_smoketest.p...
tests/gclient_smoketest.py:767: 
2 lines

[kustermann, 2013-04-17 13:55:06]

Thanks for the review. PTAL

https://codereview.chromium.org/13814012/diff/8004/gclient.py
File gclient.py (right):

https://codereview.chromium.org/13814012/diff/8004/gclient.py#newcode568
gclient.py:568: # (The timestamps of commits in google code are broken -- which
can
On 2013/04/15 09:13:32, ricow1 wrote:
> which can can -> which can

Done.

https://codereview.chromium.org/13814012/diff/18001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/13814012/diff/18001/gclient.py#newcode546
gclient.py:546: """Use revision/timestamp from parent if no explicit revision
was specified.
On 2013/04/16 20:28:04, Marc-Antoine Ruel wrote:
> Uses

Done.

https://codereview.chromium.org/13814012/diff/18001/gclient.py#newcode570
gclient.py:570: if self.url.startswith('/'):
On 2013/04/16 20:28:04, Marc-Antoine Ruel wrote:
> Why not do this check because evaluating parent_revision and
> parent_revision_date?

Both branches used parent_revision. I changed it so it's no longer used in the
if branch.

https://codereview.chromium.org/13814012/diff/18001/gclient.py#newcode834
gclient.py:834: checked out"""
On 2013/04/16 20:28:04, Marc-Antoine Ruel wrote:
> """SCMWrapper instance for this dependency or None if not processed yet."""

Done.

https://codereview.chromium.org/13814012/diff/18001/testing_support/fake_repo...
File testing_support/fake_repos.py (right):

https://codereview.chromium.org/13814012/diff/18001/testing_support/fake_repo...
testing_support/fake_repos.py:317: # Necessary to be able to change revision
properties
On 2013/04/16 20:28:04, Marc-Antoine Ruel wrote:
> Sadly, this will break tests on Windows. But I don't run the presubmit checks
on
> Windows often so they could be broken at the moment, since it happens from
time
> to time.
> 
> I'm not sure how to manage that in an efficient way.

I introduced a branch for windows, but I've no idea if this works. I tried to
test it on a windows VM but there were quite a few errors (not necessarily
caused by my change).

https://codereview.chromium.org/13814012/diff/18001/testing_support/fake_repo...
testing_support/fake_repos.py:651: 
On 2013/04/16 20:28:04, Marc-Antoine Ruel wrote:
> 2 lines between file-level symbols

Done.

https://codereview.chromium.org/13814012/diff/18001/testing_support/fake_repo...
testing_support/fake_repos.py:695: 
On 2013/04/16 20:28:04, Marc-Antoine Ruel wrote:
> here too

Done.

https://codereview.chromium.org/13814012/diff/18001/testing_support/fake_repo...
testing_support/fake_repos.py:703: FakeReposTestBase.FAKE_REPOS =
self.FAKE_REPOS_CLASS()
On 2013/04/16 20:28:04, Marc-Antoine Ruel wrote:
> That will significantly slow down the test but oh well.

We can cache it based on 'self.FAKE_REPOS_CLASS'.

https://codereview.chromium.org/13814012/diff/18001/tests/gclient_smoketest.py
File tests/gclient_smoketest.py (right):

https://codereview.chromium.org/13814012/diff/18001/tests/gclient_smoketest.p...
tests/gclient_smoketest.py:767: 
On 2013/04/16 20:28:04, Marc-Antoine Ruel wrote:
> 2 lines

Done.

[M-A Ruel, 2013-04-17 14:54:09]

lgtm with small nits, thanks for the tests.

https://codereview.chromium.org/13814012/diff/28001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/13814012/diff/28001/gclient.py#newcode561
gclient.py:561: assert self.parent and self.parent.used_scm != None
assert self.parent and self.parent.used_scm
is fine. By python code guide, we don't check for None unless necessary.

https://codereview.chromium.org/13814012/diff/28001/testing_support/fake_repo...
File testing_support/fake_repos.py (right):

https://codereview.chromium.org/13814012/diff/28001/testing_support/fake_repo...
testing_support/fake_repos.py:320: write("%s.bat" % revprop_hook_filename, "")
Add a # TODO(kustermann): Test on Windows one day.

and yes, the todo includes free irony.

https://codereview.chromium.org/13814012/diff/28001/testing_support/fake_repo...
testing_support/fake_repos.py:710: if not self.FAKE_REPOS_CLASS in
self.CACHED_FAKE_REPOS:
Thanks, that's a good idea.

https://codereview.chromium.org/13814012/diff/28001/tests/gclient_smoketest.py
File tests/gclient_smoketest.py (right):

https://codereview.chromium.org/13814012/diff/28001/tests/gclient_smoketest.p...
tests/gclient_smoketest.py:777: # TODO(maruel): safesync.
Na, I disown this TODO.

https://codereview.chromium.org/13814012/diff/28001/tests/gclient_smoketest.p...
tests/gclient_smoketest.py:819: # timestamp(r3) < timestamp(r2) svn will
checkout r1.
Add a note about why this could happen:
# This can occasionally happen on googlecode.com but is unexpected to happen on
svnserve.

[ricow1, 2013-04-18 07:11:30]

https://codereview.chromium.org/13814012/diff/28001/tests/gclient_smoketest.py
File tests/gclient_smoketest.py (right):

https://codereview.chromium.org/13814012/diff/28001/tests/gclient_smoketest.p...
tests/gclient_smoketest.py:819: # timestamp(r3) < timestamp(r2) svn will
checkout r1.
On 2013/04/17 14:54:09, Marc-Antoine Ruel wrote:
> Add a note about why this could happen:
> # This can occasionally happen on http://googlecode.com but is unexpected to
happen on
> svnserve.
Probably:
# This happens all the time on http://googlecode.com but is unexpected to happen
on svnserve (unless you manually mess with the time stamps)

[kustermann, 2013-04-18 08:28:40]

Thanks for taking another look.

https://codereview.chromium.org/13814012/diff/28001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/13814012/diff/28001/gclient.py#newcode561
gclient.py:561: assert self.parent and self.parent.used_scm != None
On 2013/04/17 14:54:09, Marc-Antoine Ruel wrote:
> assert self.parent and self.parent.used_scm
> is fine. By python code guide, we don't check for None unless necessary.

Done.

https://codereview.chromium.org/13814012/diff/28001/testing_support/fake_repo...
File testing_support/fake_repos.py (right):

https://codereview.chromium.org/13814012/diff/28001/testing_support/fake_repo...
testing_support/fake_repos.py:320: write("%s.bat" % revprop_hook_filename, "")
On 2013/04/17 14:54:09, Marc-Antoine Ruel wrote:
> Add a # TODO(kustermann): Test on Windows one day.
> 
> and yes, the todo includes free irony.

Done :-)

https://codereview.chromium.org/13814012/diff/28001/tests/gclient_smoketest.py
File tests/gclient_smoketest.py (right):

https://codereview.chromium.org/13814012/diff/28001/tests/gclient_smoketest.p...
tests/gclient_smoketest.py:777: # TODO(maruel): safesync.
On 2013/04/17 14:54:09, Marc-Antoine Ruel wrote:
> Na, I disown this TODO. 

Well, it was there in the old code as well. But I removed it now :-)

https://codereview.chromium.org/13814012/diff/28001/tests/gclient_smoketest.p...
tests/gclient_smoketest.py:819: # timestamp(r3) < timestamp(r2) svn will
checkout r1.
On 2013/04/18 07:11:30, ricow1 wrote:
> On 2013/04/17 14:54:09, Marc-Antoine Ruel wrote:
> > Add a note about why this could happen:
> > # This can occasionally happen on http://googlecode.com but is unexpected to
> happen on
> > svnserve.
> Probably:
> # This happens all the time on http://googlecode.com but is unexpected to
happen
> on svnserve (unless you manually mess with the time stamps)

Done.

[commit-bot: I haz the power, 2013-04-18 08:29:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kustermann@google.com/13814012/34001

[commit-bot: I haz the power, 2013-04-18 08:32:05]

Change committed as 194852