Unclear parsing of skip worktree #11011
Conversation
#10993 is merged now 🎉 |
Parse of 'S' was done by a git-status parser not related to skip-worktree or assume-unchanged status.
gerhardol
force-pushed
the
feature/parse-skip-worktree
branch
from
6671c0a
to
18b2267
Compare
UnitTests/GitCommands.Tests/Git/GetAllChangedFilesOutputParserTest.cs
Outdated
Show resolved
Hide resolved
UnitTests/GitCommands.Tests/Git/GetAllChangedFilesOutputParserTest.cs
Outdated
Show resolved
Hide resolved
RussKie
added
the
📭 needs: author feedback
ghost
removed
the
📭 needs: author feedback
| if (gitItemStatus.IsSkipWorktree) | ||
| const char SkippedStatus = 'S'; | ||
| const char SkippedStatusAssumeUnchanged = 's'; | ||
| if (statusCharacter is not SkippedStatus or SkippedStatusAssumeUnchanged) |
There was a problem hiding this comment.
Hi, sorry for commenting on a closed PR but I just noticed that this condition does not look correct.
I assume the intention is to continue if statusCharacter is not one of 'S' or 's'? If so, then the condition should be:
statusCharacter is not SkippedStatus and not SkippedStatusAssumeUnchanged
As it is now, the loop will continue if statusCharacter is 's'.
There was a problem hiding this comment.
Parens missing?
| if (statusCharacter is not SkippedStatus or SkippedStatusAssumeUnchanged) | |
| if (statusCharacter is not (SkippedStatus or SkippedStatusAssumeUnchanged)) |
There was a problem hiding this comment.
Yes, parens should be added
But it only affects files that are both assumed unchanged and skip worktree (is that possible?)
Should be changed
There was a problem hiding this comment.
Actually, it is intentional. AssumeUnchanged are already added to the filelist.
There should be a comment though
There was a problem hiding this comment.
Interesting, I didn't know you could use or in that way.
Proposed changes
Parse of 'S' was done by a git-status parser not
related to skip-worktree or assume-unchanged status.
No functionality changed.
(Well a file can be both assume-unchanged and skip-worktree simultaneously now, I do not think that is possible in Git.)
Test methodology
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.