hediet / vscode-hediet-power-tools

This extension provides various helper commands.
https://marketplace.visualstudio.com/items?itemName=hediet.hediet-power-tools
GNU General Public License v3.0
34 stars 4 forks source link

Extension clobbers global gitconfig setting #10

Open AjayKMehta opened 2 years ago

AjayKMehta commented 2 years ago

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.

hediet commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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
```js $ 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 commented 2 years ago

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

AjayKMehta commented 2 years ago

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 commented 2 years ago

A PR to fix this would be great.

There is one!

AjayKMehta commented 2 years ago

Awesome 👍

On Mon, Aug 15, 2022, 9:15 AM Arseny Boykov @.***> wrote:

A PR to fix this would be great.

There is one!

— Reply to this email directly, view it on GitHub https://github.com/hediet/vscode-hediet-power-tools/issues/10#issuecomment-1215139957, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACVJQJ7LXXFNKQJNTCA7KV3VZJNHPANCNFSM5OBGMNQQ . You are receiving this because you were mentioned.Message ID: @.***>