Update top_dir with git repo root just when cwd locate in a git repo.

Update top_dir with git repo root just when cwd locate in a git repo.

if when execute command "git rev-parse --show-toplevel" in a directory
that is not a subdirectory of a git repo, CheckCallAndFilter will not
throw exception, so the top_dir may be updated with a invalid path.

BUG=633971
Signed-off-by: hanpfei <hanpfei@gmail.com>;
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/13f9c37185750d4212a125204d22302376c77f8e

[hanpfei, 2016-08-03 12:59:14]

Description was changed from

==========
Update top_dir with git repo root just when cwd locate in a git repo.

if when execute command "git rev-parse --show-toplevel" in a directory
that is not a subdirectory of a git repo, CheckCallAndFilter will not
throw exception, so the top_dir may be updated with a invalid path.

BUG=633971

Signed-off-by: hanpfei <hanpfei@gmail.com>
==========

to

==========
Update top_dir with git repo root just when cwd locate in a git repo.

if when execute command "git rev-parse --show-toplevel" in a directory
that is not a subdirectory of a git repo, CheckCallAndFilter will not
throw exception, so the top_dir may be updated with a invalid path.

BUG=633971

Signed-off-by: hanpfei <hanpfei@gmail.com>
==========

[hanpfei, 2016-08-03 12:59:14]

hanpfei@gmail.com changed reviewers:
+ gab@chromium.org, hinoka@google.com, sbc@chromium.org

[gab, 2016-08-03 13:37:43]

gab@chromium.org changed reviewers:
- gab@chromium.org

[gab, 2016-08-03 13:37:43]

Why 3 reviewers for one file? Only one of which is OWNERS here AFAICT..? I'm at
least not qualified for this review -- removing self.

[hanpfei, 2016-08-03 14:06:09]

hanpfei@gmail.com changed reviewers:
+ agable@chromium.org, dpranke@chromium.org, hinoka@chromium.org,
iannucci@chromium.org, sergeyberezin@chromium.org

[agable, 2016-08-03 16:39:50]

LGTM because this does in fact fix an error, but I would argue strongly that
this code shouldn't exist at all.

Also, please don't add ~7 reviewers to a CL; try to find just one or two of the
most appropriate reviewers (by looking at OWNERS files, or `git blame` on the
lines you're editing) and just add them.

https://codereview.chromium.org/2200193003/diff/1/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/2200193003/diff/1/gclient_utils.py#newcode665
gclient_utils.py:665: def GetPrimarySolutionPath():
The only things which call this function are GetBuildtoolsPath and
dart_format.py
The only things which call GetBuildtoolsPath are GetBuildtoolsPlatformBinaryPath
and clang_format.py
The only things which call GetBuildtoolsPlatformBinaryPath are clang_format.py
and gn.py

This logic should not exist in gclient; it is very specific to the fact that
chromium/src.git contains a directory named 'buildtools'. Those tools
(dart_format, clang_format, and gn) should determine that they are being invoked
from chromium/src.git and locate buildtools themselves, not rely on heuristics
built in to a generic repository synchronization tool.

https://codereview.chromium.org/2200193003/diff/1/gclient_utils.py#newcode669
gclient_utils.py:669: if not gclient_root:
Under what circumstances do we actually enter this branch? Why are you calling
gclient without a .gclient file at all?

[hanpfei, 2016-08-04 01:28:32]

hanpfei@gmail.com changed reviewers:
+ erg@chromium.org, jochen@chromium.org
- dpranke@chromium.org, hinoka@chromium.org, hinoka@google.com,
iannucci@chromium.org, sbc@chromium.org, sergeyberezin@chromium.org

[hanpfei, 2016-08-04 01:30:15]

On 2016/08/03 16:39:50, agable wrote:
> LGTM because this does in fact fix an error, but I would argue strongly that
> this code shouldn't exist at all.
> 
> Also, please don't add ~7 reviewers to a CL; try to find just one or two of
the
> most appropriate reviewers (by looking at OWNERS files, or `git blame` on the
> lines you're editing) and just add them.
> 
> https://codereview.chromium.org/2200193003/diff/1/gclient_utils.py
> File gclient_utils.py (right):
> 
> https://codereview.chromium.org/2200193003/diff/1/gclient_utils.py#newcode665
> gclient_utils.py:665: def GetPrimarySolutionPath():
> The only things which call this function are GetBuildtoolsPath and
> dart_format.py
> The only things which call GetBuildtoolsPath are
GetBuildtoolsPlatformBinaryPath
> and clang_format.py
> The only things which call GetBuildtoolsPlatformBinaryPath are clang_format.py
> and gn.py
> 
> This logic should not exist in gclient; it is very specific to the fact that
> chromium/src.git contains a directory named 'buildtools'. Those tools
> (dart_format, clang_format, and gn) should determine that they are being
invoked
> from chromium/src.git and locate buildtools themselves, not rely on heuristics
> built in to a generic repository synchronization tool.
> 
> https://codereview.chromium.org/2200193003/diff/1/gclient_utils.py#newcode669
> gclient_utils.py:669: if not gclient_root:
> Under what circumstances do we actually enter this branch? Why are you calling
> gclient without a .gclient file at all?

Sorry for adding so many reviewers.

[hanpfei, 2016-08-04 01:35:57]

https://codereview.chromium.org/2200193003/diff/1/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/2200193003/diff/1/gclient_utils.py#newcode669
gclient_utils.py:669: if not gclient_root:
On 2016/08/03 16:39:50, agable wrote:
> Under what circumstances do we actually enter this branch? Why are you calling
> gclient without a .gclient file at all?

I didn't call gclient. I just called gn.
I want to use net module of chromium and gn + ninja flow in our projects. So I
create an environment that just include net module and the components it depends
on. Then I found this issue.

[Elliot Glaysher, 2016-08-04 17:07:30]

Removing self.

[jochen (gone - plz use gerrit), 2016-08-05 11:39:28]

Hey,

we don't have a contributor license agreement from you, so we can't just yet
accept this patch. Can you please follow the steps here:
http://dev.chromium.org/developers/contributing-code#TOC-Legal-stuff

thanks!

[hanpfei, 2016-08-05 13:42:31]

On 2016/08/05 11:39:28, jochen (OOO until Aug 22) wrote:
> Hey,
> 
> we don't have a contributor license agreement from you, so we can't just yet
> accept this patch. Can you please follow the steps here:
> http://dev.chromium.org/developers/contributing-code#TOC-Legal-stuff
> 
> thanks!

I have submitted my CLA.

Thanks a lot!

[jochen (gone - plz use gerrit), 2016-08-05 13:43:46]

On 2016/08/05 at 13:42:31, hanpfei wrote:
> On 2016/08/05 11:39:28, jochen (OOO until Aug 22) wrote:
> > Hey,
> > 
> > we don't have a contributor license agreement from you, so we can't just yet
> > accept this patch. Can you please follow the steps here:
> > http://dev.chromium.org/developers/contributing-code#TOC-Legal-stuff
> > 
> > thanks!
> 
> I have submitted my CLA.
> 
> Thanks a lot!

Indeed, lgtm

[hanpfei, 2016-08-05 23:47:31]

On 2016/08/05 13:43:46, jochen (OOO until Aug 22) wrote:
> On 2016/08/05 at 13:42:31, hanpfei wrote:
> > On 2016/08/05 11:39:28, jochen (OOO until Aug 22) wrote:
> > > Hey,
> > > 
> > > we don't have a contributor license agreement from you, so we can't just
yet
> > > accept this patch. Can you please follow the steps here:
> > > http://dev.chromium.org/developers/contributing-code#TOC-Legal-stuff
> > > 
> > > thanks!
> > 
> > I have submitted my CLA.
> > 
> > Thanks a lot!
> 
> Indeed, lgtm

Hey, 
Could you help to submit the patch?

Thanks a lot!

[agable, 2016-08-09 01:16:24]

On 2016/08/05 at 23:47:31, hanpfei wrote:
> On 2016/08/05 13:43:46, jochen (OOO until Aug 22) wrote:
> > On 2016/08/05 at 13:42:31, hanpfei wrote:
> > > On 2016/08/05 11:39:28, jochen (OOO until Aug 22) wrote:
> > > > Hey,
> > > > 
> > > > we don't have a contributor license agreement from you, so we can't just
yet
> > > > accept this patch. Can you please follow the steps here:
> > > > http://dev.chromium.org/developers/contributing-code#TOC-Legal-stuff
> > > > 
> > > > thanks!
> > > 
> > > I have submitted my CLA.
> > > 
> > > Thanks a lot!
> > 
> > Indeed, lgtm
> 
> Hey, 
> Could you help to submit the patch?
> 
> Thanks a lot!

You should be able to submit it by hitting the "Commit" button at the top of the
page (in the Polymer UI), or by checking the "Commit" checkbox (in the older
UI). If those options aren't available to you, one of us can do it instead, but
try first.

[hanpfei, 2016-08-09 05:01:19]

The CQ bit was checked by hanpfei@gmail.com

[hanpfei, 2016-08-09 05:01:20]

The CQ bit was unchecked by hanpfei@gmail.com

[hanpfei, 2016-08-09 05:02:40]

The CQ bit was checked by hanpfei@gmail.com

[commit-bot: I haz the power, 2016-08-09 05:02:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 05:06:02]

Description was changed from

==========
Update top_dir with git repo root just when cwd locate in a git repo.

if when execute command "git rev-parse --show-toplevel" in a directory
that is not a subdirectory of a git repo, CheckCallAndFilter will not
throw exception, so the top_dir may be updated with a invalid path.

BUG=633971

Signed-off-by: hanpfei <hanpfei@gmail.com>
==========

to

==========
Update top_dir with git repo root just when cwd locate in a git repo.

if when execute command "git rev-parse --show-toplevel" in a directory
that is not a subdirectory of a git repo, CheckCallAndFilter will not
throw exception, so the top_dir may be updated with a invalid path.

BUG=633971

Signed-off-by: hanpfei <hanpfei@gmail.com>
Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/13f9c37185750d...
==========

[commit-bot: I haz the power, 2016-08-09 05:06:04]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/13f9c37185750d...