-
Notifications
You must be signed in to change notification settings - Fork 11
Fix pull failure when deleted items have no internal or external name #849
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good fix, but I think you can and should take this a step further.
do ##class(SourceControl.Git.Utils).RemoveRoutineTSH(item) | ||
if (filename = "") || '##class(%File).Exists(filename) { | ||
if (item '= "") { | ||
do ##class(SourceControl.Git.Utils).RemoveRoutineTSH(item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of completeness, you should also update RemoveRoutineTSH()
to validate the input is not null because that is called from a few different places:
ClassMethod RemoveRoutineTSH(InternalName As %String) As %Status
{
Quit:(InternalName = "") $$$OK
Set tInternalName = ##class(%Studio.SourceControl.Interface).normalizeName(InternalName)
Quit:(tInternalName = "") $$$OK
Kill @..#Storage@("TSH", tInternalName)
Quit $$$OK
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it's a good idea and I have implemented it.
The reason I've kept the filename = ""
check is because the behaviour of %File.Exists()
did not seem to be explicitly defined in the documentation or in the code for a null string input. It seems sensible to me to not bother checking for the existence of a file using a filename when we may not actually have one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call.
…ble null subscript error Co-authored-by: Cameron M <[email protected]>
1442ce0
to
9165ff9
Compare
I just tested to verify this PR fixes the issue I reported in #844. I made a branch starting at a point with the bad file name, pushed a commit fixing my config file + renaming the schema file, then running "pull" from the sc menu. My "Broken" Schema config: "HL7":{
"*":{
"directory":"schemas/hl7"
}
} My fixed schema config: "HL7": {
"*": {
"directory": "schemas/hl7/",
"noFolders": true
}
} my "Pull" output:
Not sure where the "Page does not exist" error is coming from, but that seemed to not matter because after pulling the rename commit, my custom schema had been loaded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified this works and also fixes a bug I had reported: #844
The "Page does not exist" error is due to |
Fixes #848
Gracefully handle
filename
oritem
being empty strings inside##class(SourceControl.Git.PullEventHandler.IncrementalLoad).DeleteFile()
Commit 7d52f33 causes
filename
to be an empty string more readily. Which results in the first part of the if statement being executed, ifitem
is an empty string too then##class(SourceControl.Git.Utils).RemoveRoutineTSH
will throw an exception and cause the pull to fail.As described in fixed issue, was observed with an .xsd file that was present in a Git repo and then deleted. Pull would fail when handling the deletion of this file that had no external or internal name.