Bugfix in gclient_scm.py: Remove the correct directories if svn checkout is in a broken state

Bugfix in gclient_scm.py: Remove the correct directories if svn checkout is in a broken state

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

[kustermann, 2013-08-14 12:12:55]

We encountered this issue today. It can happen in the following situation:

If a developer runs 'gclient sync -f' and presses <Ctrl-C> to abort it, it might
leave svn directories in an inconsistent state.
The next time the developer does a 'gclient sync -f' it will run 'svn cleanup'
on it. In case this is unsuccessful, we'll remove the directories.

Two of our developers ran into this and the second gclient removed their *whole*
checkout (including .git directories ...)!

https://codereview.chromium.org/22886004/diff/1/gclient_scm.py
File gclient_scm.py (left):

https://codereview.chromium.org/22886004/diff/1/gclient_scm.py#oldcode1234
gclient_scm.py:1234: gclient_utils.rmtree(d[1])
We call 'scm.SVN.CaptureStatus()' with cwd=self.checkout_path. Which will just
call '$ svn status ...' inside cwd. The reported paths will be relative to cwd.
For example: 

$ svn status | head
! L     .
! L     3.8.2
! L     3.8.2/plugins
  L     3.7.0
  L     3.7.0/plugins

Then we call 'gclient_utils.rmtree(d[1])'. This will potentially delete the
wrong directories.

In the above example we will remove '.'! So we need to do
'os.path.join(self.checkout_path, d[1]).

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

https://codereview.chromium.org/22886004/diff/1/gclient_scm.py#newcode1233
gclient_scm.py:1233: if os.path.isabs(d[1]):
AFAIK "d[1]" will always be relative. I added this just to double check.

[M-A Ruel, 2013-08-14 16:50:37]

On 2013/08/14 12:12:55, kustermann wrote:
> We encountered this issue today. It can happen in the following situation:
> 
> If a developer runs 'gclient sync -f' and presses <Ctrl-C> to abort it, it
might
> leave svn directories in an inconsistent state.
> The next time the developer does a 'gclient sync -f' it will run 'svn cleanup'
> on it. In case this is unsuccessful, we'll remove the directories.
> 
> Two of our developers ran into this and the second gclient removed their
*whole*
> checkout (including .git directories ...)!

Why are you using gclient sync --force in the first place?

[kustermann, 2013-08-14 18:41:06]

On 2013/08/14 16:50:37, M-A Ruel wrote:
> On 2013/08/14 12:12:55, kustermann wrote:
> > We encountered this issue today. It can happen in the following situation:
> > 
> > If a developer runs 'gclient sync -f' and presses <Ctrl-C> to abort it, it
> might
> > leave svn directories in an inconsistent state.
> > The next time the developer does a 'gclient sync -f' it will run 'svn
cleanup'
> > on it. In case this is unsuccessful, we'll remove the directories.
> > 
> > Two of our developers ran into this and the second gclient removed their
> *whole*
> > checkout (including .git directories ...)!
> 
> Why are you using gclient sync --force in the first place?


According to 'gclient sync --help': 
...
Examples:
...
  gclient sync --force
      update files from SCM according to current configuration, for
      all modules (useful for recovering files deleted from local copy)
...

So if there are deleted files, it will restore them.
Furthermore it will force gclient to run the hooks as well. The git workflow for
dart (see https://code.google.com/p/dart/wiki/GettingTheSource) seems to require
people to either run 'gclient sync -f' or run 'gclient sync; gclient runhooks'.

But the question whether to use '--force' or not is really orthogonal to this
issue.
The current behaviour of gclient_scm.py is just wrong:
Currently it might delete the wrong directories. So why shouldn't we fix it?

[M-A Ruel, 2013-08-15 15:01:45]

Robbie, mind reviewing? Thanks.

[iannucci, 2013-08-15 19:28:18]

lgtm % comment

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

https://codereview.chromium.org/22886004/diff/1/gclient_scm.py#newcode1233
gclient_scm.py:1233: if os.path.isabs(d[1]):
On 2013/08/14 12:12:55, kustermann wrote:
> AFAIK "d[1]" will always be relative. I added this just to double check.

Maybe since we don't expect this path to hit at all, just `assert not
os.path.isabs(d[1])` ?

Makes me nervous to support conditions which we have no way to observe/test....

[kustermann, 2013-08-19 07:29:33]

Thank you for the review.

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

https://codereview.chromium.org/22886004/diff/1/gclient_scm.py#newcode1233
gclient_scm.py:1233: if os.path.isabs(d[1]):
On 2013/08/15 19:28:18, iannucci wrote:
> On 2013/08/14 12:12:55, kustermann wrote:
> > AFAIK "d[1]" will always be relative. I added this just to double check.
> 
> Maybe since we don't expect this path to hit at all, just `assert not
> os.path.isabs(d[1])` ?
> 
> Makes me nervous to support conditions which we have no way to
observe/test....

Done.

[commit-bot: I haz the power, 2013-08-19 07:29:58]

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

[commit-bot: I haz the power, 2013-08-19 07:31:44]

Change committed as 218239