The stat() method should not be dependent on the core.symlinks config entry

[lchiocca]

The contact for the stat() and lstat() function is:

stat() stats the file pointed to by path and fills in buf.
lstat() is identical to stat(), except that if path is a symbolic link, then the link itself is stat-ed, not the file that it refers to.

stat() should always return the statistics of the file or directory a symbolic link is pointing to. The lstat() function is used to get the stats for the symlink. Hence the check should not be there.

[kblees]

If symlink support is disabled, stat() should behave the same way as it did in previous versions. That's why we need this extra check (and use lstat() instead).

[lchiocca]

@kblees, that was exactly the issue. In 2.4.0.1 it worked perfectly, while now it doesn't. That's what I call a break in functionality. If you are so quick in dissmissing this pull-request, could you at least have the dignity to read and comment then the issue #186.

Thanks
Loris

[lchiocca]

In addition to the above: Look at the code of the 2.4.0.1 tag:

mingw_lstat() { return do_stat_internal(0, file_name, buf); } // 0 = don't follow symlink
mingw_stat() { return do_stat_internal(1, file_name, buf); } // 1 = follow symlink

In 2.4.0.1: The contract for stat() and lstat() was absolutely correct! In master this is no longer the case!

I know that making git symlink work was a pain in the butt, but that doesn't mean there aren't any issues. The problem is that mingw functions shouldn't be affected by the fact that git is configured to use symlinks or not. Now, stat() behaves differently if core.symlinks is set or not.

[dscho]

If symlink support is disabled, stat() should behave the same way as it did in previous versions. That's why we need this extra check (and use lstat() instead).

@kblees When I read this, at first it made complete sense. But then I checked the documentation:

core.symlinks::
If false, symbolic links are checked out as small plain files that
contain the link text. linkgit:git-update-index[1] and
linkgit:git-add[1] will not change the recorded type to regular
file. Useful on filesystems like FAT that do not support
symbolic links.

In other words, Git's documentation promises to change behavior based on core.symlinks only when checking out files. All other file system operations are unchanged.

And indeed, on Linux the behavior of stat() is unaffected by the core.symlinks setting.

Based on these observations, I tend to agree that it would be a good idea to change stat() in the way proposed by @lchiocca, i.e. always follow symbolic links, in line with the behavior on Linux.

@kblees do you agree with this line of reasoning?

[kblees]

If you are so quick in dissmissing this pull-request, could you at least have the dignity to read and comment then the issue #186.

I might have found this issue if it had been referenced by this PR.

mingw_stat() { return do_stat_internal(1, file_name, buf); } // 1 = follow symlink

do_stat_internal never followed symlinks, even with follow==1.

That stat() now fails with ELOOP if it needs to follow a symlink was introduced in b908441. If this causes problems, we should fix it, but in a way that still ensures backward compatibility.

Following the symlink is implemented by opening the file with FILE_FLAG_BACKUP_SEMANTICS, which may not work for end users (requires the SeBackupPrivilege).

[kblees]

If false, symbolic links are checked out as small plain files that contain the link text.

This couldn't have worked before because readlink() wasn't implemented at all.

If core.symlinks should behave this way, perhaps it was a mistake to "reuse" it to enable sysmlinks in the mingw.[ch] backend.

[lchiocca]

@kblees, sorry for being harsh on this one, my apologies. For me, do_stat_internal has always worked. That might be because I'm always working with admin-rights. I never checked if it worked without them.

[dscho]

@lchiocca could you check without admin rights? That would be truly helpful.

As to v2.4.0.1: as pointed out by @kblees, readlink() is not implemented, but then I fail to see how it could have worked to resolve symbolic links because the code uses readlink() and never uses FILE_FLAG_BACKUP_SEMANTICS...

[lchiocca]

@dscho, @kblees: Double-checked. It works without admin rights in my case. But just as a reminder, I'm not using symlinks in the workspace, but within the .git folder. I've got multiple branches (i.e. folders) of the same repo that share the same git repo (using the git-new-workdir). That was the original issue. stat() wasn't working because of symlinks inside the .git folder.

[dscho]

@kblees how about using FILE_FLAG_BACKUP_SEMANTICS by default, detecting the error code that we do not have permission and falling back to the current code in that case? That should be a nice compromise, methinks.

[kblees]

but then I fail to see how it could have worked to resolve symbolic links

It didn't...it just returned symlinks as regular files or directories, with st_size == -1 (return value of unimplemented readlink()). The other stat fields (file times etc.) were those of the link rather than the target.

[kblees]

It works without admin rights in my case.

MSDN docs for CreateFile say this: "Appropriate security checks still apply when this flag is used without SE_BACKUP_NAME and SE_RESTORE_NAME privileges."

This could mean that it works without the privileges if the user's permissions match the requested permissions (the CreateFile call here uses dwDesiredAccess == 0, to require absolute minimum permissions).

However, I'm not really sure if my interpretation of the docs is correct (thus the extra code that tries to mimic the old behaviour).

If you can confirm that opening the file works with end user permissions and on all platforms (including XP), and the test-suite passes with this, your change should be fine.

...and sorry for closing the issue so rashly, I didn't realize that this was in response to an existing issue.

[lchiocca]

@kblees, no problems with closing the issue; I was kind of impolite to assume too much - "assumptions are the mothers of all f**kups" ;)

I'll try my best to test it in different conditions, but I honestly don't think I'll be able to test winXP... I'm not even sure if I kept those iso. Maybe I can find some floppy images with the installation disks ;)

[dscho]

@lchiocca no worries, I have VMs for XP 32-bit and 64-bit. I can test. We should make those tests part of the test suite anyway.

[lchiocca]

@dscho & @kblees : Let me do some initial testing in that case, but might be mid-week until I can get it done.

[dscho]

@kblees are you referring to https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858%28v=vs.85%29.aspx ? If so, maybe you can help me understand it. The only mention of SE_BACKUP_NAME I see is:

To open a directory using CreateFile, specify the FILE_FLAG_BACKUP_SEMANTICS flag as part of dwFlagsAndAttributes. Appropriate security checks still apply when this flag is used without SE_BACKUP_NAME and SE_RESTORE_NAME privileges.

I do not quite understand what that means, as I am usually not working with SE_* flags. Is SE_BACKUP_NAME or SE_RESTORE_NAME somehow limited to certain privileged users by default?

[lchiocca]

@dscho, that flag is really weird to understand. The best explanation I found is:

The way I read it, if you have those privileges, then the system will override the file security checks for you. So if you don't have those privileges, then the program will simply continue to be bound by whatever file security checks would ordinarily be in effect

(source: http://stackoverflow.com/questions/2640601/does-using-readdirectorychangesw-require-administrator-rights)

[dscho]

@lchiocca that's really useful, I think even I get it now. Thanks.

[dscho]

@kblees with the successful testing of this PR by @rburgstaler, do you have any remaining objections against merging this? I would like to include it in Git for Windows 2.4.6.

[kblees]

do you have any remaining objections against merging this

No, not at all.

As I said, the extra code was just an (obviously unsuccessful) attempt to mimic old behaviour, in case the new variant causes problems.

If we can fix bugs by removing code, so much the better.

[dscho]

Perfect. Thanks, all!