Fix for MSYS2 (Cygwin)

[zdm]

This patch apply fixes to make it work with Cygwin based Git.

The main problem is that under Cygwin all path in git command should be absolute in the unix style (/c/path instead of c:/path) or be relative to the git root dir.
It is hard to convert paths to the unix style for Cygwin only, better if just use relative paths everywhere.

Also this patch fixes problem in Windowd Terminal, then strings returned with the trailing \n.

[lewis6991]

Stdout probably just has a single trailing \n that should be stripped.

[lewis6991]

Suggested change

	if string.sub(path, 1, string.len(toplevel)) == toplevel then
	path = string.sub(path, string.len(toplevel) + 1)
	end
	if path:sub(1, #toplevel) == toplevel then
	path = path:sub(#toplevel + 1)
	end

[zdm]

Suggestions applied

[zdm]

So generally it is ready.
Main idea is to use paths, relative to the toplevel dir everywhere.

Some tests aren't passed. This is because debug log changed, because file path changed to relative.
I will review it later, very unhandy to do this on windows.

[lewis6991]

Aren't you able to pass self.file through normalize_path so it goes through cygwin path?

[zdm]

Aren't you able to pass self.file through normalize_path so it goes through cygwin path?

Yes, this is possible, but this will make things more complex.
In this case we should first check, is used git is cygwin-based.
Then if true - encode paths to cygwin format, using system call.

Much easier is to convert path to relative, and this will work under all conditions.

[lewis6991]

We already do this in some places. We just also need to do this when we create the git_obj for the file field in Obj.new. Done once for each buffer.

[zdm]

git_obj has file and ``relpath` properties.

• file - absolute path;
• relpath - relative to top-level;

Just use relpath instead of file.
You can see in changes, that I already do this in several places.

[lewis6991]

Yes I have already reviewed the code. I am suggesting it can be simplified by just fixing self.file once, which will allow you to revert most of the changes.

All you need to do is:

• strip the trailing \n from stdout of the cygwin call.
• pass self.file through normalize_path().

This will ensure there are 0 functional changes for non-windows users.

That's unless you know of a situation that using relative paths fixes/improves for non-windows?

[zdm]

Yes, you are right. Better to store flie as repative from the beginning. I will try this.

[zdm]

I am not sure about tests, seems they were broken before?
I see that log to match is significantly differ, this is not due to the changes I made.

Code is ready to merge (except tests).

[lewis6991]

Tests need fixing.

[lewis6991]

Is there a reason you couldn't just do:

• strip the trailing \n from stdout of the cygwin call.
• pass self.file through normalize_path().

What you have done looks more complicated.

[lewis6991]

I don't agree with this change. You are special casing when file starts with a drive AND is on Windows, and haven't explained it.

Suggested change

	-- windows
	if vim.fn.has('win32') then
	if not string.match(file, '^[A-Za-z]:[/\\]') then
	file = toplevel .. util.path_sep .. file
	end
	-- unix
	elseif not vim.startswith(file, '/') then
	file = toplevel .. util.path_sep .. file
	end
	if not vim.startswith(file, '/') and toplevel then
	file = toplevel .. util.path_sep .. file
	end
	file = util.normalize_path(file)

And move normalize_path from repo.lua to util.lua.

[lewis6991]

I'm not sure this change will be necessary if we change the commands to use the relative path.

This will also need the helper function for better absolute path detection I added in the other PR.

[lewis6991]

Yes, this is potential problem, but it was introduced by the previous contributor.

If it wasn't compatible you would know as --git-dir is passed to every git command. You can easily verify by running one of these commands in your shell. If that works, then I'd expect self.file = normalize_path(file) to also work.

[zdm]

I don't agree with this change. You are special casing when file starts with a drive AND is on Windows, and haven't explained it.

Path, started with a drive letter under Windows - is absolute path.
You previous code prepended it with a "/" - this is a bug.
c:/aaa become /c:/aaa - this totally incorrect. This is not valid path for cygwin or windowd and will never works.
Your previous contributor just added this code, but not tested it, or this code was initially designed for unix only.

With my improvements it become portable and works correctly.

What is wrong?

[lewis6991]

What is wrong?

You did not explain this.

It's taken a long discussion to get to you to explain this.

You previous code prepended it with a "/" - this is a bug.

I do not think it did. It prepended it with toplevel.

[lewis6991]

Can you check if #1254 works?

[zdm]

Can you check if #1254 works?

Yes, will check.

I don't understand, why you want to use normalize_path )))
I am not native speaker and unable to describe full picture correctly, so we have miscommunication.

Here is the summary of changes:

1. We are storing paths internally in the standard absolute unix or windows formats, depends on current OS. If git is cygwin - we convert work-tree and git-dir, returned by git from cygwin to windows format using normalize_path. This is only one place, where normalize_path is used. We don;t operate with cygwin paths, we just convert them to normal path.
2. So, on buffer attach we have:

• file path - absolute file path in native format. NOT cygwin.
• git-path, work-tree - absolute paths in native format for current OS. NOT cygwin.
• take into account - all paths stored in the same format.

3. When we perform some operation - we make file path relative to the current work-tree.
4. This will also correctly work with links, or show please an example when this will fail. If you do not agree - lets deeply analyze this case. Link - is also represented as absolute path with prefix, identical to the work-tree, so we can make it relative to work tree without ant problems. If link is not relative to work-tree - git will not accept it. Hope, you understand me.

Please, tell me, what is wrong or confusing at the list above?

[zdm]

Maybe this is confusing for you.
Cygwin git can accept path in both formats - native windows (c:/aaa) and cygwin (/c/aaa).
Not cygwin programs can work with native paths only.

We are using native windows paths only (not cygwin), because this is portable, and will work for any git.

[lewis6991]

You keep explaining what your changes are, but you are not explaining what bug you are fixing. The current code works well for unix, so I find most of these changes unnecessary.

One issue I can find is the vim.startwith(path, '/') check is wrong for Windows which I've fixed in #1254.

[zdm]

The bug:

git --git-dir D:/downloads/111/.git --work-tree D:/downloads/111 ls-files D:/downloads/111/js.js


fatal: /d/downloads/111/js.js: '/d/downloads/111/js.js' is outside repository at '/d/downloads/111'

The fix - use relative paths:

git --git-dir D:/downloads/111/.git --work-tree D:/downloads/111 ls-files js.js

100644 f9fcfe856d408d1d7cb86530a70544518911a98c 0       i/lf    w/lf    attr/                   js.js

[zdm]

Hi.
So, what is your decision?
Please, reject if, if you don't need this changes.

[lewis6991]

I've looked into it and I agree now that we should use relative paths for most/all git commands. Absolute paths seem to work on Unix because git has better path logic then on Windows, but git itself only reasons about relative paths.

I still need to review this and will likely want to combine it with parts from the other PR. Additionally I will want both Windows and Unix to use relative paths, similar to your first version of this PR.

Thanks for bearing with me. I know it was difficult, but I wanted to make sure I understood this properly as this code has gone back and forth a few times.

[lewis6991]

Can you explain this, I don't understand it. Don't we need to use cygpath in some cases?

[zdm]

Soon we will forget what we are talking about here.
Chances to approve this changes rapidly grows down. )))

[zdm]

Hey, how are you?
Do you alive? ))
Please, let's finish work on this patch for Windows.

[zdm]

Closed because the Dunning-Kruger effect.

[Dioarya]

Are there any temporary fixes for this? I'm running MSYS2 and still doesn't work