Ckan: [Bug] Out-of-order checks during Fake creation

[tofof]

Background

• Operating System: Win10
• CKAN Version: 1.26.2
• KSP Version: 1.7.2.2556

Have you made any manual changes to your GameData folder (i.e., not via CKAN)?
No

Problem

During creation of a fake instance, the name for the fake instance is checked only after CKAN puts files (including a lock) into the destination path. Changing the name after it complains still doesn't allow for the creation of the fake at that location, because now the destination isn't blank -- and the user can't delete the registry.locked file CKAN put there without exiting CKAN.

Steps to reproduce

1. Make new folders named 'one' and 'two' on desktop
2. File, manage ksp instances, new ksp instance, clone or fake new instance.
3. Fake new instance, name 'test1', path into 'one'.
4. Create and switch to instance.
5. File, manage instances, new instance, clone/fake.
6. Fake new instance, name 'test1', path into 'two', press create.
7. CKAN puts files into 'two' folder, then returns error about 'name already used' or 'name taken'.
8. Change name from 'test1' to 'test2'. press create.
9. Now, CKAN complains the folder is non-empty.
10. Attempt to delete files from folder.
11. Fail, because open handle on registry.locked file.

Expected behavior
CKAN checks requisite conditions like name uniqueness before altering path on disk, and/or cleans up after itself (rolling back its file creations) if the creation process does not successfully complete.

[DasSkelett]

With #2773 the instance faking is a wrapped in a transaction, this should solve all those issue, including cleaning up after failed attempts. It is not merged yet.

Do you want to test that branch if everything works as you expect it?
I can upload a compiled binary here if you don't want to build it yourself.

[tofof]

Yes I'm happy to test it, particularly if I don't have to build it myself this early in the morning. 💤

[DasSkelett]

Here you go:
link removed, see below
Source-code available here: https://github.com/DasSkelett/CKAN/tree/feature/fake-more-dlcs

Also, the instance managing dialog is getting a rework too right now, see #2787.

[tofof]

No, it doesn't work as expected.
In fact, it's worse, in that it throws an unhandled exception when following the steps I outlined above.

Also, it suggests 'empty for none' on the expansions but if you actually do so it complains that it's not formatted correctly, and won't let you leave them empty.

See the end of this message for details on invoking 
just-in-time (JIT) debugging instead of this dialog box.

************** Exception Text **************
System.FormatException: Index (zero based) must be greater than or equal to zero and less than the size of the argument list.
   at System.Text.StringBuilder.AppendFormatHelper(IFormatProvider provider, String format, ParamsArray args)
   at System.String.FormatHelper(IFormatProvider provider, String format, ParamsArray args)
   at System.String.Format(String format, Object[] args)
   at CKAN.Main.ErrorDialog(String text, Object[] args)
   at CKAN.GUIUser.RaiseError(String message, Object[] args)
   at CKAN.CloneFakeKspDialog.<buttonOK_Click>d__6.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()

[DasSkelett]

Huh, that's funny, it crashes when generating an error message :laughing:

Also, it suggests 'empty for none' on the expansions but if you actually do so it complains that it's not formatted correctly, and won't let you leave them empty.

Oups...

[DasSkelett]

Next try :)
newer version down below
Now it shouldn't complain about empty textboxes anymore, and the crash shouldn't happen.
And a lot of other stuff I discovered should be fixed. Gosh, transactions are complicated.

[tofof]

Closer but still no cigar.

Blank expansion versions 🆗
Fake with duplicate name 🆗
Clone with duplicate name ❌

Change step 6 in my original procedure to clone, instead:

1. Leave the 'clone instance' set to 'one', name for new instance 'one', path for new instance 'two'

You'll get the error "Clone failed: An entry with the same key already exists."

Once again, however, it will have too-eagerly placed files into 'two'. And, as before, when you dismiss the error and change the name for the new instance to 'two', you now get the nonemptiness error.

This version at least doens't do the registry.locked file handle problem, so now you are able to manually delete all the files to fix it. But, as before, it shouldn't have created those files in the first place until after these conditionals are checked.

[DasSkelett]

Okay, I think I got it all covered now.
I moved all the checks to the beginning, and also (hopefully) enhanced the transactions.
Feel free to test every edge case you can think of.
ckan.zip

[tofof]

[DasSkelett]

Thanks alot for testing!