issue-6910 clearer message with actionable hint for repo sync#7110
issue-6910 clearer message with actionable hint for repo sync#7110
Conversation
4f8629b to
c10c679
Compare
vilmibm
left a comment
There was a problem hiding this comment.
Reading through the linked issue, it sounds like local changes was meant to be changed to untracked files also?
pkg/cmd/repo/sync/sync.go
Outdated
| if currentBranch == branch { | ||
| if isDirty, err := git.IsDirty(); err == nil && isDirty { | ||
| return fmt.Errorf("can't sync because there are local changes; please stash them before trying again") | ||
| return fmt.Errorf("can't sync because there are local changes; please stash them before trying again. Use git stash --all for offending untracked files.") |
There was a problem hiding this comment.
I'm not a fan of this error message since it can be potentially confusing to the user.
First, the IsDirty() method doesn't tell us what was "dirty" in the first place: was it a tracked file with uncommitted changes, an untracked file, or both? To answer that, we could in theory extend our git client with a more informative method based on UncommittedChangeCount, but that might require more work on this PR that you've bargained for, so for now let's maybe avoid that.
If an error message gives actionable instructions, we want those instructions to be as generic as possible and ideally work for everyone. The above error message essentially says to use stash or stash --all depending on the circumstances, but we have to keep in mind that the user doesn't necessarily understand the circumstances. The message should also ideally not be misleading, e.g. by suggesting that there might have been “offending untracked” files when there were none.
To address all that, I propose something like:
| return fmt.Errorf("can't sync because there are local changes; please stash them before trying again. Use git stash --all for offending untracked files.") | |
| return fmt.Errorf("refusing to sync due to uncommitted local changes.\nUse `git stash --all` before retrying sync and `git stash pop` afterwards") |
In Go in general, this much sentence formatting in an error message is frowned upon, but since this error will be printed pretty much unchanged to stderr, multiline messages with sentences are fine when returned from the main command function itself.
There was a problem hiding this comment.
That should help. I added some extra phrasing around it. Should make it clearer. Please let me know if you prefer the more compact phrasing.
0e902f6 to
31110e7
Compare
mislav
left a comment
There was a problem hiding this comment.
Thanks! I've pushed a tweak to further simplify the error message.
Co-authored-by: Mislav Marohnić <mislav@github.com>
issue-6910 clearer message with actionable hint
Add hint to the gh repo sync error message for when there are untracked files.
Using git stash --all (or git stash --include-untracked) should help when people try to sync their local copy and
the --force option fails due to untracked files.
--
Fixes #6910