Do not blindly assume that a .gclient file in a parent directory belongs to t...

Do not blindly assume that a .gclient file in a parent directory belongs to the current directory.

BUG=54238
TEST=gclient_smoketest.py

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

[jochen (gone - plz use gerrit), 2010-09-07 10:09:21]

please review.

Added another test case. I'm relying on .gclient_entries this time. Doing the
change in gclient_utils should also fix trychange.py

[M-A Ruel, 2010-09-07 13:00:58]

http://codereview.chromium.org/3342020/diff/1/2
File depot_tools/gclient_utils.py (right):

http://codereview.chromium.org/3342020/diff/1/2#newcode352
depot_tools/gclient_utils.py:352: return None
?

.gclient_entries is not created when gclient sync is aborted in the middle of an
initial sync, forcing the user to redo a complete sync. Better to return path.

Or better, put the rest of the code in "if os.path.exists(entries_filename):"

http://codereview.chromium.org/3342020/diff/1/2#newcode358
depot_tools/gclient_utils.py:358: all_directories = [d for d in
scope['entries'].keys()]
you mean:
all_directories = scope['entries'].keys()
?

http://codereview.chromium.org/3342020/diff/1/2#newcode359
depot_tools/gclient_utils.py:359: real_from_dir = real_from_dir[len(path)+1:]
I'd prefer you to use another variable since it's confusing to mutate it this
way.

http://codereview.chromium.org/3342020/diff/1/2#newcode360
depot_tools/gclient_utils.py:360: while len(real_from_dir):
while real_from_dir:

http://codereview.chromium.org/3342020/diff/1/3
File depot_tools/tests/gclient_smoketest.py (right):

http://codereview.chromium.org/3342020/diff/1/3#newcode152
depot_tools/tests/gclient_smoketest.py:152: write(join(self.root_dir,
'.gclient_entries'),
don't create this file manually. It removes the purpose of the test.

[jochen (gone - plz use gerrit), 2010-09-07 13:58:27]

http://codereview.chromium.org/3342020/diff/1/2
File depot_tools/gclient_utils.py (right):

http://codereview.chromium.org/3342020/diff/1/2#newcode352
depot_tools/gclient_utils.py:352: return None
An initial sync is always done in the directory where the .gclient file is. If
the initial sync hasn't happened yet, there's no way to tell for gclient which
directory belongs to the checkout and which doesn't

http://codereview.chromium.org/3342020/diff/1/2#newcode358
depot_tools/gclient_utils.py:358: all_directories = [d for d in
scope['entries'].keys()]
On 2010/09/07 13:00:58, Marc-Antoine Ruel wrote:
> you mean:
> all_directories = scope['entries'].keys()
> ?

Done.

http://codereview.chromium.org/3342020/diff/1/2#newcode359
depot_tools/gclient_utils.py:359: real_from_dir = real_from_dir[len(path)+1:]
On 2010/09/07 13:00:58, Marc-Antoine Ruel wrote:
> I'd prefer you to use another variable since it's confusing to mutate it this
> way.

Done.

http://codereview.chromium.org/3342020/diff/1/2#newcode360
depot_tools/gclient_utils.py:360: while len(real_from_dir):
On 2010/09/07 13:00:58, Marc-Antoine Ruel wrote:
> while real_from_dir:

Done.

http://codereview.chromium.org/3342020/diff/1/3
File depot_tools/tests/gclient_smoketest.py (right):

http://codereview.chromium.org/3342020/diff/1/3#newcode152
depot_tools/tests/gclient_smoketest.py:152: write(join(self.root_dir,
'.gclient_entries'),
On 2010/09/07 13:00:58, Marc-Antoine Ruel wrote:
> don't create this file manually. It removes the purpose of the test.

the purpose of this test is to check that this is the correct config for an
existing checkout. An initial checkout should always happen from the directory
where the file is.

[M-A Ruel, 2010-09-07 14:05:06]

http://codereview.chromium.org/3342020/diff/1/2
File depot_tools/gclient_utils.py (right):

http://codereview.chromium.org/3342020/diff/1/2#newcode352
depot_tools/gclient_utils.py:352: return None
On 2010/09/07 13:58:27, jochen wrote:
> An initial sync is always done in the directory where the .gclient file is. If
> the initial sync hasn't happened yet, there's no way to tell for gclient which
> directory belongs to the checkout and which doesn't

Initial gclient sync aborts all the time for different reasons; http 502,
invalid credentials, etc. Right now your code requires the user to be in a
specific directory to do the checkout when the user has an incomplete checkout.

http://codereview.chromium.org/3342020/diff/6001/7002
File depot_tools/tests/gclient_smoketest.py (right):

http://codereview.chromium.org/3342020/diff/6001/7002#newcode152
depot_tools/tests/gclient_smoketest.py:152: write(join(self.root_dir,
'.gclient_entries'),
I'd prefer the test to sync than making the file manually.

http://codereview.chromium.org/3342020/diff/6001/7002#newcode163
depot_tools/tests/gclient_smoketest.py:163: write(join(self.root_dir,
'.gclient_entries'),
same. This test should simply make sure that running gclient status from a
subdirectory works.

[jochen (gone - plz use gerrit), 2010-09-07 16:50:19]

please review

[M-A Ruel, 2010-09-07 18:56:28]

lgtm with one small nit

http://codereview.chromium.org/3342020/diff/11001/5002
File depot_tools/gclient_utils.py (right):

http://codereview.chromium.org/3342020/diff/11001/5002#newcode356
depot_tools/gclient_utils.py:356: logging.warning("%s file in parent directory
%s might not be the file"
I think it's better to print >> sys.stderr