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

clone: Store stderr so it is displayed on clone failure #4060

Merged
merged 2 commits into from Jan 21, 2020

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Jan 20, 2020

When GitPython raises a GitCommandError, whether the exception's
stderr attribute actually contains the standard error depends on
whether progress reporting is enabled. With progress enabled, the
standard error is stored in RemoteProgress.error_lines instead of the
exception 0.

As of 3ecc68a (2018-09-27), we assign RemoteProgress.error_lines to
our GitPythonProgressBar._last_error_lines. In the case of clone(),
we consider _last_error_lines to some extent, but it's not included in
the errors messages that we show the clone call fails with all
candidates. In the specific cases of an unknown version, this means
that we report that cloning failed without any indication that the
unknown version was the issue.

Make the standard error visible to the caller by replacing the
exception's stderr with _last_error_lines.

Fixes #4038.


As an example, this changes

$ datalad clone ria+http://127.0.0.1:36265/#eb94c485-3bbe-11ea-b262-28c63f920482@impossible a
[...]
install(error): /tmp/ga-KyJmTKq/a (dataset) [Failed to clone from all attempted sources: ['http://127.0.0.1:36265/eb9/4c485-3bbe-11ea-b262-28c63f920482', 'http://127.0.0.1:36265/eb9/4c485-3bbe-11ea-b262-28c63f920482/.git']]

into

$ datalad clone ria+http://127.0.0.1:36265/#eb94c485-3bbe-11ea-b262-28c63f920482@impossible a
[...]
install(error): /tmp/ga-KyJmTKq/a (dataset) [Failed to clone from any candidate source URL. Encountered errors per each url were:
- http://127.0.0.1:36265/eb9/4c485-3bbe-11ea-b262-28c63f920482
  Cmd('git') failed due to: exit code(128)
  cmdline: git clone --progress -v --branch=impossible http://127.0.0.1:36265/eb9/4c485-3bbe-11ea-b262-28c63f920482 /tmp/ga-KyJmTKq/a
  stderr: fatal: Remote branch impossible not found in upstream origin
- http://127.0.0.1:36265/eb9/4c485-3bbe-11ea-b262-28c63f920482/.git
  Cmd('git') failed due to: exit code(128)
  cmdline: git clone --progress -v --branch=impossible http://127.0.0.1:36265/eb9/4c485-3bbe-11ea-b262-28c63f920482/.git /tmp/ga-KyJmTKq/a
  stderr: fatal: repository 'http://127.0.0.1:36265/eb9/4c485-3bbe-11ea-b262-28c63f920482/.git/' not found]

kyleam added 2 commits Jan 20, 2020
This is in preparation for the next commit, which will check that we
receive an informative message when a specified version doesn't exist.
That check is more pleasant to do by inspecting the
IncompleteResultsError str() than using any of the assert_* helpers
with the results list.
When GitPython raises a GitCommandError, whether the exception's
stderr attribute actually contains the standard error depends on
whether progress reporting is enabled.  With progress enabled, the
standard error is stored in RemoteProgress.error_lines instead of the
exception [0].

As of 3ecc68a (2018-09-27), we assign RemoteProgress.error_lines to
our GitPythonProgressBar._last_error_lines.  In the case of clone(),
we consider _last_error_lines to some extent, but it's not included in
the errors messages that we show the clone call fails with all
candidates.  In the specific cases of an unknown version, this means
that we report that cloning failed without any indication that the
unknown version was the issue.

Make the standard error visible to the caller by replacing the
exception's stderr with _last_error_lines.

Fixes datalad#4038.

[0]: gitpython-developers/GitPython#599
@codecov
Copy link

@codecov codecov bot commented Jan 20, 2020

Codecov Report

Merging #4060 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4060      +/-   ##
==========================================
+ Coverage   89.77%   89.77%   +<.01%     
==========================================
  Files         273      273              
  Lines       36601    36604       +3     
==========================================
+ Hits        32859    32862       +3     
  Misses       3742     3742
Impacted Files Coverage Δ
datalad/core/distributed/tests/test_clone.py 91.22% <100%> (+0.05%) ⬆️
datalad/core/distributed/clone.py 93.01% <100%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 733ba83...d5d0f53. Read the comment docs.

mih
mih approved these changes Jan 21, 2020
Copy link
Member

@mih mih left a comment

Great, thx @kyleam !

@mih mih merged commit 4b3941d into datalad:master Jan 21, 2020
17 checks passed
@kyleam kyleam deleted the clone-version-error branch Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants