Skip to content
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

Invalid characters being put in Gist filenames #1044

Closed
rcdailey opened this issue Sep 16, 2019 · 19 comments
Closed

Invalid characters being put in Gist filenames #1044

rcdailey opened this issue Sep 16, 2019 · 19 comments

Comments

@rcdailey
Copy link

🐛 Describe the bug

Certain files are written with invalid characters to the Gist which prohibits them from being cloned via Git on Windows, due to the limitations Windows places on allowed characters in filenames. In this case, the backslash (\) character is present in a few files:

  • bd3d4befe38185704bf0fc875e9deed6\configuration.json
  • snippets\cpp.json

When attempting to clone this Gist via SSH, it fails:

$ git clone [email protected]:abcdefg123456789.git settings_sync
Cloning into 'settings_sync'...
remote: Enumerating objects: 11, done.
remote: Counting objects: 100% (11/11), done.
remote: Compressing objects: 100% (10/10), done.
remote: Total 11 (delta 1), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (11/11), 6.27 KiB | 3.13 MiB/s, done.
Resolving deltas: 100% (1/1), done.
error: unable to create file bd3d4befe38185704bf0fc875e9deed6\configuration.json: No such file or directory
error: unable to create file snippets\cpp.json: No such file or directory
fatal: unable to checkout working tree
warning: Clone succeeded, but checkout failed.
You can inspect what was checked out with 'git status'
and retry with 'git restore --source=HEAD :/'

The backslash characters break on Windows.

🌴 Visual Studio Code Version : [ 1.38.1 ]
🌴 Code Settings Sync Version : [ 3.4.2 ]
🌴 Standard or Insiders : [ Standard ]
🌴 Portable or Installed : [ Installed ]
🌴 OSS or Official Build : [ Official Build ]
🌴 Operating System : [ Windows 10 x64 Professional ]
🌴 Proxy Enabled: [ No ]
🌴 Gist Id: [ N/A ]

📰 To Reproduce
Steps to reproduce the behavior:

  1. Go to your Gist on gist.github.com
  2. Copy the SSH Clone URL provided on the top right of the page
  3. Clone the gist via Git on Windows

💪 Expected behavior

The clone should succeed. To fix the issue will require removing the incompatible characters from the filenames. You could use a dash or an underscore, for example.

For context: The reason I want to clone the Gist is to revert to a previous version of my VS Code settings. There is no built-in revert command that I've seen, so I decided to do it manually. Between 2 machines I'm working on, Settings Sync is constantly overwriting settings between machines by uploading extensions that I've uninstalled already.

@rcdailey
Copy link
Author

@shanalikhan I'm sure you are busy but I would very much appreciate a time slice if you have one.

@shanalikhan
Copy link
Owner

The reason I want to clone the Gist is to revert to a previous version of my VS Code settings. There is no built-in revert command that I've seen, so I decided to do it manually.

I understand your concern. Have you check the procedure to revert older Gist.

Between 2 machines I'm working on, Settings Sync is constantly overwriting settings between machines.

In v3.4.3 version. Settings Sync upload process is considerable improved (thanks to @karl-lunarg ). If its still over writing again and again. I suggest you can open new issue with reproducing steps so i can further investigate.

@rcdailey
Copy link
Author

@shanalikhan I will try the newer version for you. In the meanwhile, I still do firmly believe that the invalid characters should be removed. I'd rather revert using a clone on my local machine rather than doing it through Gist's clunky interface. I think it's a relatively simple task to do this. I might consider a PR if you are OK with these changes:

  1. New settings filename format will use underscores instead of slashes
  2. Backward compatibility: If files with slashes are detected, load those settings and then convert the filename to use underscores. Old files (with the slashes) will be removed once the conversion occurs.

@karl-lunarg
Copy link
Contributor

@rcdailey : You might just want to try cloning or fetch/pull your gist again after doing an upload with version v3.4.3. As part of this update, I fixed some code that was using pathSep in a place where it should have been using '|'. I don't know if this will fix your problem or not. But when I clone my gist after uploading with v3.4.3, I see filenames like snippets|c.json in my local cloned directory.

You may also want to inspect your gist to make sure that the "old" files with the bad names are not there. For example, you might have snippets\c.json that got uploaded to the gist as a result of the bug and might be left in your gist after the bug was fixed. If that's the case, it is easy enough to go into your gist via the GitHub web interface and delete the offending files. That would let you clone your gist again.

@rcdailey
Copy link
Author

@karl-lunarg Thanks for the quick response. I did force-push up my settings from VS Code after updating, I do not see the filenames renamed:

$ git ls-tree origin
100644 blob 564d79ba9709c16aef7cbe7c17f85b7bf6ec3ba2    "bd3d4befe38185704bf0fc875e9deed6\\configuration.json"
100644 blob 40b286a50443bd6846f94c78d0375454b2541015    cloudSettings
100644 blob 3469cb0ebce45571b742f700e07a81376dfdf81e    extensions.json
100644 blob 36b52393c665c9486fa1501edd5941933928b7e4    keybindings.json
100644 blob 23ca3db4bfe4526aa725f78f1a3acebd63d0f787    keybindingsMac.json
100644 blob a7531d98f018d9cf2029a769065ca88a7b623ed6    settings.json
100644 blob b2060fd17ecc58d68f053ba1b93a4e86266addd3    "snippets\\cpp.json"

@karl-lunarg
Copy link
Contributor

And I have:

$ git ls-tree origin
100644 blob 4b6af4b1f680d2386b839d31540e642c96265d0e	cloudSettings
100644 blob 01d0f82746b8a273ad05981a50a883c90b8d12d5	extensions.json
100644 blob a8fe6f52918f2d3aa23b9c6a99eee946c02ae0c6	keybindings.json
100644 blob 23ca3db4bfe4526aa725f78f1a3acebd63d0f787	keybindingsMac.json
100644 blob 75b22d62b0d6133f0525d06b4365a8f109ef8df5	settings.json
100644 blob 450dafffd4c866c481f368f07127948d448c0655	snippets|c.json
100644 blob fbecafc83c807bc33f13ccbf0643cf4d9875fcd7	snippets|cpp.json

I'm on Linux, but if I were experiencing the same problem, I would expect snippets/cpp.json.

What do these files look like in your gist, as seem from the GitHub web interface? Mine looks like:

Screenshot from 2019-09-23 07-53-14

@rcdailey
Copy link
Author

image

@rcdailey
Copy link
Author

It makes sense to me that if VS Code had a generic variable for "separator", that it would be \ on Windows, and / on Linux. So you'd need to handle both forward & backward slash in your logic. Is it possible you are only converting forward slashes to pipes?

@karl-lunarg
Copy link
Contributor

VS Code does have such a generic variable and it is called path.sep and it does what you say.

The code in service/file.service.ts, function GetFile is supposed to generate a gistName that substitutes the native path separator with '|'. On Windows, path.sep is "\\" and I wonder if the split function is handling it as expected. We'd have to debug this function with a Windows-style path to see what is going on.

@rcdailey : Are you in a position to look into this?

@rcdailey
Copy link
Author

rcdailey commented Sep 23, 2019 via email

@rcdailey
Copy link
Author

@karl-lunarg unfortunately it looks like pipe characters are forbidden as well:

https://gist.github.com/doctaphred/d01d05291546186941e1b7ddc02034d3

Need to use underscores.

@karl-lunarg
Copy link
Contributor

I just don't know where to start.

I didn't know where to start either a couple of weeks ago. :-)

You might search for "building vs code extensions", etc. I think I started here and here.

Basically, you'll have to install node.js on your system. Then fork/clone this repo. Run npm install in this repo. Start vs code in this repo. Press F5 to build the extension and start another instance of vs code. You can then set a breakpoint in that function I mentioned in the first instance of vs code. Finally start an upload in the debug instance.

@karl-lunarg
Copy link
Contributor

Need to use underscores.

I see. Even if we fix the extension to properly put '|' in the gistName, cloning the gist to Windows won't work because of the '|' in the name. Correct?

Oddly, Linux allows this and still lets me deal with files with the pipe embedded in the name:

$ ls
 cloudSettings     keybindings.json      settings.json     'snippets|cpp.json'
 extensions.json   keybindingsMac.json  'snippets|c.json'

I suppose that I can keep the shell from interpreting pipe as a pipe as long as I keep the filenames enclosed.

$ cat 'snippets|cpp.json'

works.

If a git clone of the gist fails on Windows because of the '|', then we're in the position of not being friendly about cloning the gist (hopefully an unusual case) or coming up with another way to represent path separators. The gist seems to be a flat directory and so it seems hard to try to do real directories there.

Why do you think that underscore might be good for this? Couldn't any settings file have an underscore in its name, which would probably cause the extension to download it into the wrong place?

I don't know right now how to fix this. I didn't come up with the '|' thing and I sort of thought that was an established GitHub/Gist convention. And if the gist can't be cloned on Windows, then it looks like cloning a gist containing snippets wasn't tested on Windows. And this isn't hard to imagine since it seems like it shouldn't be common to clone a gist. (But I totally get why you would want to)

p.s. I just tried cloning my gist (with filenames containing '|') to a Windows machine and it failed to checkout the files with the '|'.

@rcdailey
Copy link
Author

When looking at snippets/cpp.json, why is the snippets/ part required? Why couldn't you use snippets_cpp.json? What does snippets/ mean here? Or maybe just use cpp.json?

@karl-lunarg
Copy link
Contributor

What does snippets/ mean here?

I think that it is because snippets is a directory in the User settings area.

I also think that the choice of storing snippets in a snippets directory in the User area was a choice made in VS Code itself.

You'd have to loop in @shanalikhan on this, but I'd guess that there's some sort of attempt here to have a generalized facility to store more things from the User settings area in the gist and this would include things in directories as well as files like settings.json. And that's why there is the path.sep substitution with '|' in the gistName.

It would be a little dangerous to assume that any gistName that has an underscore in it implies that it goes into a subdir in the User area. If that assumption is made and a future version of VS Code came out that started using a file called other_settings.json, then this assumption would cause the extension to put a file called settings.json in a directory called other.

I suppose that your approach would work if this extension code sort of singled out the snippets files and supported storing them in the gist specifically. That is, when the extension uploads a snippet file, it would form the gistName by snippets_ + filename. ("snippets_cpp.json"). And then when downloading, if the gistName started with "snippets_", it would then form the local path to the User's settings file as snippets + path.sep + filename, where filename is whatever follows "snippets_" in the gistName.

In other words, this would be sort of "tagging" snippet files with some sort of prefix to form the gistName and then undoing the tag to come up with a local directory name. The tag could be a GUID or anything else like "banana", but it would obviously be better to be more mnemonic.

The downside is that explicit support would have to be added if VS Code ever added a new feature that stored state in a new dir in the User area like "stickyNotes". But I don't think that would happen often and isn't necessarily a bad thing.

I poked around a bit and found that being unable to clone (checkout, actually) files with illegal filenames is not that uncommon on Windows. There are a few projects out there that ran into this and had to take steps to avoid having things like ':' in their filenames. So it seems like some effort to fix this would be worthwhile.

@rcdailey
Copy link
Author

Probably graduating from gists would be the best solution. What you really want is the ability to represent directories in the gist. But since gists don't allow directories, you'd have to use an actual git repo. I think adding support for arbitrary repo hosts (like Gitlab, Bitbucket) which lets you host private repositories, would be the answer. Gitlab specifically would be a great choice. Instead of using a gist, you'd use a git repo to store the settings. You can then use actual directories which will be nicer and you avoid these hacky workarounds.

@karl-lunarg
Copy link
Contributor

Probably graduating from gists would be the best solution.

Yeah, I agree

(sorry for the long previous reply)

But for now, this could be fixed pretty simply if this extension tries to store a selected list of User files and directories in the gist. That way, you could guarantee that there are no underscore surprises.

In file.service.ts GetFile(), change the '|' to '_'. when forming the gistName.

In file.service.ts CreateDirTree(), change the code to split on '_.' And probably split on '|', if present, to handle old gists.

Need to think about snippet files that could have '_' in their names.

@rcdailey
Copy link
Author

rcdailey commented Sep 23, 2019

Don't forget that this isn't just an issue with snippet files, there's this monstrosity too:

"bd3d4befe38185704bf0fc875e9deed6\\configuration.json"

I'd much rather contribute support for fully fledged git repositories if I were to spend any of my time on this. I know that's a much bigger effort, but I think it's worth it in the long haul. The worst thing about hacks is not the hack itself, but becoming complacent in the solution.

@shanalikhan
Copy link
Owner

shanalikhan commented Oct 18, 2019

I'd much rather contribute support for fully-fledged git repositories if I were to spend any of my time on this. I know that's a much bigger effort, but I think it's worth it in the long haul.

@rcdailey Yes there is already issue opened to support Git Repo. in Settings Sync. People have placed bounties about 200-300$ in #413. Feel free to send PR and get the bounty price, it would really help Settings Sync!

I think it's better to close this issue and instead innovate on supporting Git in Settings Sync to allow more flexibility and features to users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants