If --force is specified when updating, remove unversioned directories

If both -f and -D are specified when updating, remove all untracked directories

This is required to avoid the need to clobber the bots when moving a directory
to deps/. Currently, the directory in question is likely to remain in the
working copy, despite having been removed, due to the presence of untracked
files. This causes the checkout from deps/ to fail.

With this change, when both --force and --delete_unversioned_trees are
specified, the the directory in question will be removed from the working copy,
thereby allowing the copy in deps/ to be checked out correctly.

Note that untracked directories which are explicitly ignored (ie in .gitignore
or svn:ignore) will not be removed.

BUG=112887, chromium-os:20759
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121986

[M-A Ruel, 2012-02-08 14:43:46]

http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py
File gclient_scm.py (right):

http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py#newcod...
gclient_scm.py:451: ['ls-files', '--directory', '--others',
'--exclude-standard'],
Note that this will be overzealous if .gitignore is not updated properly. See
the comment at line 958 for an explanation about what's preferable.

http://crbug.com/109763 is about filtering dependencies that are know when doing
a "status", which would be helpful here and below.

http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py#newcod...
gclient_scm.py:933: if status[0][0] != '?':
Eh, that was bad.

http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py#newcod...
gclient_scm.py:958: # If --force is specified, remove any untracked files and
directories.
You are basically doing a revert only for directories.

What I wanted if to scan file_list that was generated at line 956, look for 'D'
operations and then process these.

Otherwise, it's going to be constantly deleting directories that aren't
svn:ignored properly.

[Steve Block, 2012-02-09 17:55:04]

http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py
File gclient_scm.py (right):

http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py#newcod...
gclient_scm.py:451: ['ls-files', '--directory', '--others',
'--exclude-standard'],
> http://crbug.com/109763 is about filtering dependencies that are know when
doing
> a "status", which would be helpful here and below.
I think that in this case, we want to delete untracked directories which have
just been removed from the repository, even if they are ignored or are a
dependency. This will allow us to use a single change to move a directory to
deps/ and set svn:ignore for that directory.

http://chromiumcodereview.appspot.com/9348054/diff/1004/gclient_scm.py#newcod...
gclient_scm.py:958: # If --force is specified, remove any untracked files and
directories.
> What I wanted if to scan file_list that was generated at line 956,
> look for 'D'
I considered this approach, but it seems less robust, as if you ever end up with
untracked directories in your working copy (for example, as the result of
'gclient sync' without '--force'), then there's no way to remove them without
manual intervention. So if a later invocation of 'gclient sync' tries to put a
DEPS directory in that location, it will fail, even if you use '--force'.

I guess though that it's sufficient for the bots, which can always use
'--force'. I'll update the patch to try this approach.

> Otherwise, it's going to be constantly deleting directories that
> aren't svn:ignored properly.
Indeed, but they'll immediately be replaced from DEPS. Isn't there an
expectation that svn:ignore is set correctly?

[Steve Block, 2012-02-09 18:36:46]

Updated patch is a work in progress

[cmp, 2012-02-09 18:38:46]

+some cros people that have been impacted by the previous condition in gclient
and might be interested in this

[M-A Ruel, 2012-02-10 01:43:55]

On 2012/02/09 17:55:04, Steve Block wrote:
> > Otherwise, it's going to be constantly deleting directories that
> > aren't svn:ignored properly.
> Indeed, but they'll immediately be replaced from DEPS. Isn't there an
> expectation that svn:ignore is set correctly?

I wish!

You convinced me now, I'm fine with the idea. Sorry for the delay in this review
cycle, I had to think about it a little. Why have you removed both git support
and the test?

http://chromiumcodereview.appspot.com/9348054/diff/5001/gclient_scm.py
File gclient_scm.py (right):

http://chromiumcodereview.appspot.com/9348054/diff/5001/gclient_scm.py#newcod...
gclient_scm.py:944: if options.force:
I'd prefer to start with:
if options.force and options.self._options.delete_unversioned_trees:

[Peter Mayo, 2012-02-10 01:51:09]

Could you add ,chromium-os:20759 to the BUG line.  It looks like this will go a
long way to adressing that too.

[Steve Block, 2012-02-10 11:22:20]

Updated patch to check --delete_unversioned_trees

Note that I'm seeing failures in ManagedGitWrapperTestCase.testUpdateCheckout()
and UnmanagedGitWrapperTestCase.testUpdateCheckout() when I test locally, both
before and after my change. Is this a known issue?

[M-A Ruel, 2012-02-10 15:56:40]

On 2012/02/10 11:22:20, Steve Block wrote:
> Updated patch to check --delete_unversioned_trees
> 
> Note that I'm seeing failures in
ManagedGitWrapperTestCase.testUpdateCheckout()
> and UnmanagedGitWrapperTestCase.testUpdateCheckout() when I test locally, both
> before and after my change. Is this a known issue?

ManagedGitWrapper tests aren't stable and have race conditions. :/ As long as
they pass on the CQ, it's fine with me.

lgtm with optional nit.

http://chromiumcodereview.appspot.com/9348054/diff/10002/gclient_scm.py
File gclient_scm.py (right):

http://chromiumcodereview.appspot.com/9348054/diff/10002/gclient_scm.py#newco...
gclient_scm.py:454: if paths:
nit: Not necessary to do "if paths:", just this will work right away. You can
keep the "if" on its own line if you prefer than using a generator. I don't mind
either way.

for path in (p for p in paths.splitlines() if p.endswith('/')):

[commit-bot: I haz the power, 2012-02-11 10:25:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/9348054/13002

[commit-bot: I haz the power, 2012-02-11 10:31:50]

Presubmit check for 9348054-13002 failed and returned exit status 1.

warning: code.google.com certificate with fingerprint
ce:3e:bd:64:4d:e5:66:1c:e8:63:eb:99:3d:7e:75:36:a0:86:0e:03 not verified (check
hostfingerprints or web.cacerts config setting)
Running presubmit commit checks ...
Checking out rietveld...
Running push-basic.sh
Running save-description-on-failure.sh
Running basic.sh
Running upload-stale.sh
Running upload-local-tracking-branch.sh
Running patch.sh
Running submit-from-new-dir.sh
Running post-dcommit-hook-test.sh
Running hooks.sh
Running abandon.sh
Running upstream.sh

** Presubmit Messages **
You should install python 2.5 and run ln -s $(which python2.5) python.
A great place to put this symlink is in depot_tools.
Otherwise, you break depot_tools on python 2.5, you get to keep the pieces.

** Presubmit ERRORS **
tests/gclient_smoketest.py failed!
Command tests/gclient_smoketest.py returned non-zero exit status 1 in
/mnt/data/b/commit-queue/workdir/tools/depot_tools
............................F...........
======================================================================
FAIL: testRevertAndStatus (__main__.GClientSmokeSVN)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 568, in testRevertAndStatus
    self.assertEquals(10, len(out))
AssertionError: 10 != 4

----------------------------------------------------------------------
Ran 40 tests in 158.250s

FAILED (failures=1)


Presubmit checks took 351.3s to calculate.

[Peter Mayo, 2012-02-14 04:00:24]

lgtm

[Marc-Antoine Ruel (Google), 2012-02-14 04:03:06]

On 2012/02/11 10:31:50, I haz the power (commit-bot) wrote:
> FAIL: testRevertAndStatus (__main__.GClientSmokeSVN)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "tests/gclient_smoketest.py", line 568, in testRevertAndStatus
>     self.assertEquals(10, len(out))
> AssertionError: 10 != 4

BTW, you can
./tests/gclient_smoketest.py -v GClientSmokeSVN.testRevertAndStatus
to figure out the problem.

[commit-bot: I haz the power, 2012-02-15 00:55:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/9348054/19001

[commit-bot: I haz the power, 2012-02-15 01:02:28]

Change committed as 121986

[Peter Mayo, 2012-02-15 16:13:11]

It seems this may have negatively impacted the chrome in chromiumos builders at
least.


chromeos-chrome-19.0.1043.0_alpha-r1: 1>_____ removing unversioned directory c
chromeos-chrome-19.0.1043.0_alpha-r1: Syncing projects:  40% ( 2/ 5) src 
chromeos-chrome-19.0.1043.0_alpha-r1: Error: Called
rmtree(/var/cache/chromeos-chrome/chrome-src/src/c) in non-directory


I'll look more closely in a few minutes to try to figure out what "c" was.  If
anyone else has an idea for what "in a non-directory" means, that could be
valuable to add.

[flackr, 2012-02-15 16:24:35]

On 2012/02/15 16:13:11, Peter Mayo wrote:
> It seems this may have negatively impacted the chrome in chromiumos builders
at
> least.
> 
> 
> chromeos-chrome-19.0.1043.0_alpha-r1: 1>_____ removing unversioned directory c
> chromeos-chrome-19.0.1043.0_alpha-r1: Syncing projects:  40% ( 2/ 5) src 
> chromeos-chrome-19.0.1043.0_alpha-r1: Error: Called
> rmtree(/var/cache/chromeos-chrome/chrome-src/src/c) in non-directory
> 
> 
> I'll look more closely in a few minutes to try to figure out what "c" was.  If
> anyone else has an idea for what "in a non-directory" means, that could be
> valuable to add.

For what a non-directory means, from gclient_utils.py:
  if os.path.islink(path) or not os.path.isdir(path):
    raise Error('Called rmtree(%s) in non-directory' % path)

[Steve Block, 2012-02-15 16:26:13]

Investigating

[M-A Ruel, 2012-02-15 16:34:23]

I'll revert in the meantime.

Le 15 février 2012 08:26, Steve Block <steveblock@chromium.org> a écrit :

> Investigating
>

[Steve Block, 2012-02-15 16:36:38]

Note that it's the change to gclient that needs reverting, not the
change to the buildbot config. See
http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%...

[M-A Ruel, 2012-02-15 16:37:36]

Sure but it's faster and safer to revert this now.

Le 15 février 2012 08:36, Steve Block <steveblock@chromium.org> a écrit :

> Note that it's the change to gclient that needs reverting, not the
> change to the buildbot config. See
>
>
http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%...
>

[Peter Mayo (wrong one), 2012-02-15 17:45:21]

Which "this" is that?

gclient -> ebuild -> boom!

No buildbot config will help without neutering the affected builder
completely.

On Wed, Feb 15, 2012 at 11:37 AM, Marc-Antoine Ruel <maruel@chromium.org>wrote:

> Sure but it's faster and safer to revert this now.
>
> Le 15 février 2012 08:36, Steve Block <steveblock@chromium.org> a écrit :
>
> Note that it's the change to gclient that needs reverting, not the
>> change to the buildbot config. See
>>
>>
http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%...
>>
>
>


-- 

Peter Mayo | Waterloo | petermayo@google.com | 519-880-3439

[M-A Ruel, 2012-02-15 17:53:53]

I also reverted the gclient change. You were cc'ed on the CL.

Le 15 février 2012 09:45, Peter Mayo <petermayo@google.com> a écrit :

> Which "this" is that?
>
> gclient -> ebuild -> boom!
>
> No buildbot config will help without neutering the affected builder
> completely.
>
>
> On Wed, Feb 15, 2012 at 11:37 AM, Marc-Antoine Ruel
<maruel@chromium.org>wrote:
>
>> Sure but it's faster and safer to revert this now.
>>
>> Le 15 février 2012 08:36, Steve Block <steveblock@chromium.org> a écrit :
>>
>> Note that it's the change to gclient that needs reverting, not the
>>> change to the buildbot config. See
>>>
>>>
http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%...
>>>
>>
>>
>
>
> --
>
> Peter Mayo | Waterloo |  petermayo@google.com |  519-880-3439
>
>