Do not remove deletes from the uncommitted queue when checking item status #812
Conversation
| @@ -529,7 +529,8 @@ Method GetStatus(ByRef InternalName As %String, ByRef IsInSourceControl As %Bool | |||
| } else { | |||
| // If it doesn't show up in git status, there are no uncommitted changes so it should not be locked or checked out by any user | |||
| set Editable=1, IsCheckedOut=0, UserCheckedOut="" | |||
| if ##class(SourceControl.Git.Change).IsUncommitted(filename){ | |||
| if ##class(SourceControl.Git.Change).IsUncommitted(filename) | |||
| && '('$data(files(InternalName)) && $data($$$TrackedItems(##class(%Studio.SourceControl.Interface).normalizeName(InternalName)))) { | |||
There was a problem hiding this comment.
Are these single quotes here by accident? && '('$data( ...
I think this should just be && ($data( ...
There was a problem hiding this comment.
They are not by accident. I think that I have the condition right here though it is confusing and there may be something that I'm missing.
When I delete something and trigger this logic I get:
- A:
##class(SourceControl.Git.Change).IsUncommitted(filename) = 1 - B:
$data(files(InternalName)) = 0 - C:
$data($$$TrackedItems(##class(%Studio.SourceControl.Interface).normalizeName(InternalName))) = 1
So in order to not proceed onto RemoveUncommitted which seems to cause the 'undefined user' issue. I want:
A && '('B && C)
I believe this could be simplified to A && (B || 'C). At the time it seemed reasonable to have the same condition as what is used in #758 and to just negate the whole thing.
There was a problem hiding this comment.
OH dear. I had been jumping between languages and erroneously read those as single string quotes instead of logical negations. My mistake. I think your logic is correct.
That said, one pattern I like to follow is that when conditional statements grow to a point where they get hard to mentally parse, I will extract one or more of the conditions into a well named ClassMethod that makes the conditional much easier to read. (so you'd end up with something like if .IsUncommitted(filename) && .IsUntracked(InternalName, .files).) Side benefit: I find class methods like this are relatively easy to unit test when you pass in everything it needs.
I see this more as a nice to have refactoring at this point. 100% up to you if you want to leave this as-is or make further changes.
This is another case of what was fixed in #758
The
GetStatusmethod ofSourceControl.Git.Extensioncan remove deleted items from the uncommitted queue. This can make the deleted items show up as being owned by an 'undefined' user in the workspace view.This has occurred for me in both Studio and VS Code.