Don't delete directories that have been superseded by a broader checkout.

Don't delete directories that have been superseded by a broader checkout.

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

[M-A Ruel, 2013-05-06 17:07:05]

It may work

https://chromiumcodereview.appspot.com/14583004/diff/1/gclient.py
File gclient.py (right):

https://chromiumcodereview.appspot.com/14583004/diff/1/gclient.py#newcode1138
gclient.py:1138: for e in entries]
alignment

https://chromiumcodereview.appspot.com/14583004/diff/1/gclient.py#newcode1161
gclient.py:1161: if scm.GetCheckoutRoot() in full_entries:
Add a logging.info() here.

Also, I'm not sure it works well. In theory it should work well with entries
that have been removed since GetCheckoutRoot() should behave properly, but that
is only if the SCM matches. In case of git/svn mismatch, anything can happen.

GetCheckoutRoot() will return a primary entry even if
os.path.join(self.root_dir, entry_fixed) is not a versioned directory anymore.
It's even worse in mismatch, since the function will search for a parent up to
'/', especially in git case. For svn, I think the returned value will differ if
>=1.7 or not.

[Mike Stip (use stip instead), 2013-05-06 18:55:49]

On 2013/05/06 17:07:05, Marc-Antoine Ruel wrote:
> It may work
> 
> https://chromiumcodereview.appspot.com/14583004/diff/1/gclient.py
> File gclient.py (right):
> 
> https://chromiumcodereview.appspot.com/14583004/diff/1/gclient.py#newcode1138
> gclient.py:1138: for e in entries]
> alignment
> 
> https://chromiumcodereview.appspot.com/14583004/diff/1/gclient.py#newcode1161
> gclient.py:1161: if scm.GetCheckoutRoot() in full_entries:
> Add a logging.info() here.
> 
> Also, I'm not sure it works well. In theory it should work well with entries
> that have been removed since GetCheckoutRoot() should behave properly, but
that
> is only if the SCM matches. In case of git/svn mismatch, anything can happen.

Why? It returns the local directory, not the repo URL.

> 
> GetCheckoutRoot() will return a primary entry even if
> os.path.join(self.root_dir, entry_fixed) is not a versioned directory anymore.
> It's even worse in mismatch, since the function will search for a parent up to
> '/', especially in git case. For svn, I think the returned value will differ
if
> >=1.7 or not.

The only way to tell if something is in a checkout is to recursively bubble up?
Can we do the opposite; for each repo ask if the directory is in it?

[Dirk Pranke, 2013-05-06 19:44:35]

https://chromiumcodereview.appspot.com/14583004/diff/1/gclient.py
File gclient.py (right):

https://chromiumcodereview.appspot.com/14583004/diff/1/gclient.py#newcode1161
gclient.py:1161: if scm.GetCheckoutRoot() in full_entries:
On 2013/05/06 17:07:05, Marc-Antoine Ruel wrote:
> Add a logging.info() here.
> 
> Also, I'm not sure it works well. In theory it should work well with entries
> that have been removed since GetCheckoutRoot() should behave properly, but
that
> is only if the SCM matches. In case of git/svn mismatch, anything can happen.
> 
> GetCheckoutRoot() will return a primary entry even if
> os.path.join(self.root_dir, entry_fixed) is not a versioned directory anymore.
> It's even worse in mismatch, since the function will search for a parent up to
> '/', especially in git case. For svn, I think the returned value will differ
if
> >=1.7 or not.

I don't follow this comment. Can you give a specific example?

[M-A Ruel, 2013-05-06 19:50:25]

On 2013/05/06 18:55:49, Mike Stipicevic wrote:
> Why? It returns the local directory, not the repo URL.

As a real-world example;
svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
cd depot_tools
git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git
cd depot_tools
svn info . -> doesn't work on 1.6 but I'm not sure about 1.7

Now the reverse:
git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git
cd depot_tools
svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
cd depot_tools
git rev-parse --show-cdup -> It prints out ../. This is not what you want.


> > GetCheckoutRoot() will return a primary entry even if
> > os.path.join(self.root_dir, entry_fixed) is not a versioned directory
anymore.
> > It's even worse in mismatch, since the function will search for a parent up
to
> > '/', especially in git case. For svn, I think the returned value will differ
> if
> > >=1.7 or not.
> 
> The only way to tell if something is in a checkout is to recursively bubble
up?
> Can we do the opposite; for each repo ask if the directory is in it?

You want to know if the directory you are looking at has files in it for the
root directory of the checkout. This is because git doesn't track directories.

[Dirk Pranke, 2013-05-06 22:46:28]

On 2013/05/06 19:50:25, Marc-Antoine Ruel wrote:
> On 2013/05/06 18:55:49, Mike Stipicevic wrote:
> > Why? It returns the local directory, not the repo URL.
> 
> As a real-world example;
> svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
> cd depot_tools
> git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git
> cd depot_tools
> svn info . -> doesn't work on 1.6 but I'm not sure about 1.7
> 
> Now the reverse:
> git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git
> cd depot_tools
> svn co svn://svn.chromium.org/chrome/trunk/tools/depot_tools
> cd depot_tools
> git rev-parse --show-cdup -> It prints out ../. This is not what you want.
> 

Both of these are in the other direction (subdir hides parent). I think if you
were to delete the subdirs, then check out the outer dir, then the right thing
would happen, which is the case that triggered this change (it may be that we
need to care about both, of course).

[M-A Ruel, 2013-05-16 13:34:46]

FTR, I don't mind that much so lgtm but fix alignment.

[Dirk Pranke, 2013-05-24 20:02:27]

clean up time .. this patch has landed in a different form, right. Should you
close/abandon this?

[iannucci, 2013-05-24 20:29:10]

On 2013/05/24 20:02:27, Dirk Pranke wrote:
> clean up time .. this patch has landed in a different form, right. Should you
> close/abandon this?

Yeah this was the gclient safe revert step?

[Dirk Pranke, 2013-05-24 20:54:45]

On 2013/05/24 20:29:10, iannucci wrote:
> On 2013/05/24 20:02:27, Dirk Pranke wrote:
> > clean up time .. this patch has landed in a different form, right. Should
you
> > close/abandon this?
> 
> Yeah this was the gclient safe revert step?

think so.

[Mike Stip (use stip instead), 2013-05-31 09:49:52]

On 2013/05/24 20:54:45, Dirk Pranke wrote:
> On 2013/05/24 20:29:10, iannucci wrote:
> > On 2013/05/24 20:02:27, Dirk Pranke wrote:
> > > clean up time .. this patch has landed in a different form, right. Should
> you
> > > close/abandon this?
> > 
> > Yeah this was the gclient safe revert step?
> 
> think so.

Nope, this is still needed. See https://chromiumcodereview.appspot.com/16212004
for example. Committing now...

[commit-bot: I haz the power, 2013-05-31 09:53:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xusydoc@chromium.org/14583004/15001

[commit-bot: I haz the power, 2013-05-31 09:54:42]

Change committed as 203384