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

Extension clobbers global gitconfig setting #10

Open
AjayKMehta opened this issue Feb 10, 2022 · 9 comments · May be fixed by #11
Open

Extension clobbers global gitconfig setting #10

AjayKMehta opened this issue Feb 10, 2022 · 9 comments · May be fixed by #11

Comments

@AjayKMehta
Copy link

I was wondering why the core.excludesFile setting in my global .gitconfig file was getting clobbered and I tracked it down to this.

I had the value in the file set to C:\\users\\ajay\\.gitconfig.

git config --get core.excludesFile returns c:\users\ajay\.gitconfig.

The issue I believe is that existsSync does not handle Window-style paths correctly (not verified). I also saw that it does not handle ~ in file path.

Can you please fix this so it handles these cases correctly? For now, I switched to Unix style path to be safe.

Thanks.

@AjayKMehta AjayKMehta changed the title Extension clobbers global git config setting Extension clobbers global gitconfig setting Feb 10, 2022
@hediet
Copy link
Owner

hediet commented Feb 10, 2022

Thanks for reporting this!

What is the expected value of core.excludesFile and what is the actual value?

What is your suggestion to fix this?

@AjayKMehta
Copy link
Author

So, the value in the .gitconfig file has \\ as path separator but as stated in my previous comment, git config --get core.excludesFile returns c:\users\ajay.gitconfig with \ as separator which existsSync does not like.

Screenshot below illustrates this as well as issue with ~:

existsSync

My suggestion is to replace single \ with \\ before calling this method, replace ~ using advice from accepted answer here or use a different library.

I'm not really familiar with JavaScript or TypeScript or else, I would have happily submitted a PR to fix this 😀

Thank you for making such a great extension that I use daily!

@hediet
Copy link
Owner

hediet commented Feb 11, 2022

with \ as separator which existsSync does not like.

This cannot be the issue.

When you provide a string literal in JS, you just have to double enquote \.
\u starts a unicode sequence, but digits have to follow.

I.e. "C:\\users" literally represents the string C:\users.

@AjayKMehta
Copy link
Author

Yes, you are right. I should have been more specific saying that single \ is a problem when followed by certain characters such that it forms an escape sequence.

For example, \n is also a problem:

fs.existsSync("C:\program files\notepad++") returns false.
fs.existsSync("C:\\program files\\notepad++") returns true.

Solution is to replace single \ with \\ or / in return value of git config --get core.ExcludesFile.

@Bobronium
Copy link

Bobronium commented Aug 15, 2022

I have the same issue. Even when I'm editing .git-global-excludes-file it's still gets replaced every time. I'm on macOS.

@hediet, I think the issue comes down to

  1. not stripping stdout, so fs.existsSync("~/.gitignore\n") is called
  2. as @AjayKMehta mentioned, fs.existsSync doesn't expand ~/ in path

So solution would be:

  1. call stdout.trim()
  2. expand ~/ if found in path
$ node
Welcome to Node.js v18.2.0.
Type ".help" for more information.
> const { promisify } = require('util');
undefined
> const { exec } = require('child_process');
undefined
> const { stdout } = await promisify(exec)(
... 			"git config --get core.excludesfile"
... 		)
undefined
> stdout
'~/.gitignore\n'
>
@AjayKMehta, you seem to misunderstand how escaping works.

I'm familiar with python, so will write an example using it, however same applies to JS, AFAIK.

When you hardcode strings, you have to escape certain characters because they would be replaced by interpreter otherwise:

var = "C:\\program files\\notepad++"
print(var)  # C:\program files\notepad++

But when you're dealing with input, you don't have to worry about, say, \n sequences, because it would literally represent a \ followed by n, same as you would get by hardcoding "\\n":

var = input()
print(var)  # C:\program files\notepad++

@hediet
Copy link
Owner

hediet commented Aug 15, 2022

@Bobronium thanks for your analysis, can you do a PR? :)

@Bobronium Bobronium linked a pull request Aug 15, 2022 that will close this issue
@AjayKMehta
Copy link
Author

Thanks for the primer on escaping, @Bobronium.

I understand how escaping works but not how it is done in JS/TS 😃

A PR to fix this would be great.

@Bobronium
Copy link

A PR to fix this would be great.

There is one!

@AjayKMehta
Copy link
Author

AjayKMehta commented Aug 15, 2022 via email

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

Successfully merging a pull request may close this issue.

3 participants