jacogr / atom-git-control

Not maintained :(
MIT License
227 stars 70 forks source link

Managed names with spaces, fixes #28 #74

Closed Emily-Steel closed 8 years ago

Emily-Steel commented 9 years ago

I've tested this thoroughly and dealt with all cases I could think of. I initially tried to espace spaces with backslashes but firstly, the code was ugly, and secondly, it didn't work x).

So I used JSON.stringify to add double quotes around each file name and backslash any double quotes that the file name may contain (though Atom doesn't allow creating files with double quotes in the name). That fixed commiting.

I replaced the split that was done when parsing git status to account for spaces in the file name. Previously it would only read the file name up to the first space

MarcelMue commented 9 years ago

I tried this out and your fix works well for the most part, unfortunately there is still a bug: If you delete a file with a space in its name then the filename is still lurking around with double quotes (from the stringify) and can't be selected. Same thing happens when you alter a file after it is known to git.

I am not entirely sure how to fix this issue, maybe you can think of something. Also github doesn't seem to recognize your github username as the one commiting. You might need to look into your local git setup to fix this.

Thanks for the contribution so far :+1:

Emily-Steel commented 9 years ago

According to the git-scm documentation for git-status, git status -z activates --porcelain if no other format is specified. The interesting difference is that --porcelain quotes and escapes paths containing special characters or spaces whereas -z does not. Also -z uses \0 as a separator for different status lines instead of \n.

I changed the git status to add -z but kept the --porcelain for clarity. I'm honestly a bit worried that this might cause problems elsewhere so I'll test it out a bit and see if any issues arise.

After that I noticed that I forgot to stringify paths in git rm so once I had the possibility to select the missing or renamed files, git rm still failed.

The remaining problem I see is that git status doesn't display nicely in the console anymore : OLD

> git status --porcelain --untracked-files=all
 D file with spaces
 D file with more spaces
?? new file with spaces
?? new_file

NEW

> git status --porcelain -z --untracked-files=all
 D file with spaces D file with more spaces ?? new file with spaces ?? new_file

I don't have a sufficient understanding of the console to fix this yet.

I'll check through the rest of the git class to check that no more stringify calls are needed and push to my fork so you can have a look.

UPDATE: I added stringify to git diff and git reset.

Also, I understand now that callGit passes the output of the git command to the console before passing it through the parser so it comes with no \n and I can't cleanly add them without further thought.

And by the way, you're most welcome for the contribution, I work with git a lot and lately with atom; I use your package daily in a Windows environment. It's natural that if I need a fix and can make it myself, I should do my best and share with the community.

UPDATE 2: Sorry for the messy commit history, I tried to rebase my history to change my email address on my first commit and get GitHub to recognize me as the author but it didn't go as planned :/... I you want me to clean it up before you accept the pull request I'll fully understand.

MarcelMue commented 9 years ago

I just had a quick glimpse and it looks more solid now, I will test again later today. Concerning the console: It pretty much works the way you mentioned we just pass the output from git-promise to an output area. You mentioned that -z uses \0 as a seperator instead of \n, so I think you could replace all instances of \0 with \n at line 93. But I am not sure if this has an impact on any other messages (even though I don't think it should).

Don't worry about the commits being a bit messy. I only really look at the changes and if you look through the 300 commits in this repo you will notice that the battle for clean commits is long lost :D

Thanks again for contributing!

Emily-Steel commented 9 years ago

You were right, replacing \0 with \n before logging seems to work

Emily-Steel commented 9 years ago

Don't merge yet, I found a potentially big error (no idea yet if I caused it).

I tried to do a big commit for a work related project and the editor crashed upon commiting. The commit didn't work and now I have problems in the git status display :

http://puu.sh/hO0eU/e4e18c1539.png http://puu.sh/hO0iZ/5648167851.png

I'll keep the repo in this state so that I can fiddle with it later and try to isolate the problem. I'll be in touch.

MarcelMue commented 9 years ago

Alright, thanks for your efforts! It looks to me like the console output might be too long and something goes wrong because of it. Not sure though :/

Emily-Steel commented 8 years ago

Outdated, I might give it another try, not today however