Better reporting in adverse conditions #276
Conversation
…any errors that occur will be rendered to the user for troubleshooting. Use --quiet on 'git rm' to suppress non-error output. Fixes #275 until the error happens again.
| @@ -1108,14 +1108,14 @@ def print(*a, sep=" "): | |||
| git_add_files = [] | |||
| def flush_git_add_files(): | |||
| if git_add_files: | |||
| subprocess.run(["git", "add", "-f", *git_add_files], stdout=subprocess.PIPE, stderr=subprocess.PIPE).check_returncode() | |||
There was a problem hiding this comment.
How about introducing a flag (--verbose CLI option or config) for controlling this? Dumping output out would probably confuse users, who are not familiar with Git.
There was a problem hiding this comment.
That wouldn’t have helped me. Since the issue seemed to happen just the once, and then not on subsequent runs, I had no opportunity to retry with verbose. The intention here is to avoid swallowing errors.
The behavior on git add is not to output anything so I don’t expect novice users to see anything different in the nominal case.
| git_add_files.clear() | ||
|
|
||
| git_rm_files = [] | ||
| def flush_git_rm_files(): | ||
| if git_rm_files: | ||
| try: | ||
| subprocess.run(["git", "rm", "-f", *git_rm_files], stdout=subprocess.PIPE, stderr=subprocess.PIPE).check_returncode() | ||
| subprocess.run(["git", "rm", "--quiet", "--force", *git_rm_files]).check_returncode() | ||
| except subprocess.CalledProcessError: | ||
| pass | ||
|
|
There was a problem hiding this comment.
The code below could've run git clean -f ... to wipe out untracked files.
There was a problem hiding this comment.
This looks great. Thanks for the PR and the subsequent improvement!
Just to be clear to other people, for posterity: this patch preserves my intent but allows for better error reporting. I didn't want you to see chatter from "git" when it was adding and removing files. But, as jaraco points out, "git add" doesn't print anything under normal circumstances, and "git rm --quiet" suppresses its output when everything works fine. So now in both cases it's a good idea to let git's output get through to the user, in case there is an error.
|
I don't know how the permissions work here--do I need to merge this, or are you able to? (Would you like to hit the magic green button? Would you prefer I did? Do you not care?) |
|
@larryhastings you should have write permissions to this repo (directly or via organization) to see "Merge" button |
|
I don't care. Happy to proceed. |
In #275, I described an issue where a CalledProcessError occurs but no indication is reported, a condition that likely would have been reported if the output hadn't been suppressed.
Although a simple retry worked around the issue, it may happen again in the future, and it would be nice to know what is the cause. This change enables that.
In the third commit, this change also uses the long-form option to git, making it easier for someone troubleshooting an issue like this one unfamiliar with the meaning of
-fto understand what's happening.