Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suppress git stderr when checking for existence of repo #7891

Merged
merged 3 commits into from Jul 16, 2019

Conversation

@gshuflin
Copy link
Contributor

commented Jun 18, 2019

When we shell out to git rev-parse in detect_worktree to check for
the existence of a git repository, we should be suppressing the stderr
output to avoid printing the confusing git error message fatal: not a git repository (or any of the parent directories): .git in the case
where pants happens not to be invoked inside a git repo.

@benjyw benjyw requested review from benjyw, Eric-Arellano and stuhood Jun 18, 2019

@@ -73,7 +73,7 @@ def clone(cls, repo_url, dest, binary='git'):
return cls(binary=binary, worktree=dest)

@classmethod
def _invoke(cls, cmd):
def _invoke(cls, cmd, stderr=None):

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 18, 2019

Contributor

I think this should be subprocess.DEVNULL. Is there a particular reason you want wirh None?

This comment has been minimized.

Copy link
@gshuflin

gshuflin Jun 18, 2019

Author Contributor

I don't think we always want to suppress stderr when calling _invoke, just in the specific case where we call it in detect_worktree to run git rev-parse. So the default parameter in _invoke should do the same thing as calling Popen without stderr at all, unless we tell it otherwise.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 18, 2019

Contributor

I see, that makes sense.

Could you please add a docstring entry explaining that if left as None, subprocess will keep its default of printing to stderr? That was my main confusion, being unsure what None does in this context.

This comment has been minimized.

Copy link
@gshuflin

gshuflin Jun 18, 2019

Author Contributor

Updated the docstring to clarify this

@gshuflin gshuflin force-pushed the gshuflin:fix_git branch from 27ef961 to 31ee2a1 Jun 18, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Blocked by dropping Python 2. Not critical to land this now, so Greg is planning on waiting to pick this up later in the week.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

This can be rebased against master to get around the Python 2 issues.

@gshuflin gshuflin force-pushed the gshuflin:fix_git branch from 31ee2a1 to da6bb20 Jul 15, 2019

gshuflin added some commits Jun 18, 2019

Suppress git stderr when checking for existence of repo
When we shell out to `git rev-parse` in `detect_worktree` to check for
the existence of a git repository, we should be suppressing the stderr
output to avoid printing the confusing git error message `fatal: not a
git repository (or any of the parent directories): .git` in the case
where pants happens not to be invoked inside a git repo.
Clarify _invoke behavior
Clarify the _invoke docstring to explain why stderr=None is defined
as a parameter with a default value.
@@ -47,9 +47,9 @@ def detect_worktree(cls, binary='git', subdir=None):
try:
if subdir:
with pushd(subdir):
process, out = cls._invoke(cmd)
process, out = cls._invoke(cmd, stderr=subproces.DEVNULL)

This comment has been minimized.

Copy link
@hrfuller

hrfuller Jul 15, 2019

Contributor

Ci failed because of this typo.

This comment has been minimized.

Copy link
@gshuflin

gshuflin Jul 15, 2019

Author Contributor

thanks for catching that!

Correct typo
subproces -> subprocess

@jsirois jsirois merged commit bfb6073 into pantsbuild:master Jul 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gshuflin gshuflin deleted the gshuflin:fix_git branch Jul 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.