git_index_add_bypath fails on junction on Windows

[csware]

libgit2 returned: Failed to make directory 'd:/Tortoisegit': Eine Datei kann nicht erstellt werden, wenn sie bereits vorhanden ist. (i.e. file cannot be created, because it already exists)

D:/TortoiseGit is a junction (and repo root). Cannot tell/test what happens if working tree root is a symlink.

stack-trace:

libgit2.dll!p_mkdir(const char * path=0x00000000062ef7e0, unsigned short mode=511) Zeile 26 C
libgit2.dll!git_futils_mkdir(const char * path=0x00000000062ef730, const char * base=0x0000000000000000, unsigned short mode=511, unsigned int flags=82) Zeile 347  C
libgit2.dll!git_futils_mkpath2file(const char * file_path=0x00000000062ef730, const unsigned short mode=511) Zeile 19   C
libgit2.dll!loose_backend__stream_fwrite(git_oid * oid=0x000000000629f318, git_odb_stream * _stream=0x00000000062f0e50) Zeile 773   C
libgit2.dll!write_file_stream(git_oid * oid=0x000000000629f318, git_odb * odb=0x00000000062ed1e0, const char * path=0x00000000062ed0b0, __int64 file_size=965) Zeile 100    C
libgit2.dll!blob_create_internal(git_oid * oid=0x000000000629f318, git_repository * repo=0x000000000048b7d0, const char * content_path=0x00000000062ed0b0, const char * hint_path=0x00000000062ed0bf, unsigned char try_load_filters='\x1') Zeile 187   C
libgit2.dll!git_blob_create_fromworkdir(git_oid * oid=0x000000000629f318, git_repository * repo=0x000000000048b7d0, const char * path=0x00000000062ecfd8) Zeile 231 C
libgit2.dll!index_entry_init(git_index_entry * * entry_out=0x000000000629f3c8, git_index * index=0x000000000048b910, const char * rel_path=0x00000000062ecfd8) Zeile 623    C
libgit2.dll!git_index_add_bypath(git_index * index=0x000000000048b910, const char * path=0x00000000062ecfd8) Zeile 779  C

It worked on revision 5b3d52ce37100c1d63229d195041fac3e6f89d28, but not on revision 824cf80f061ab31f45c94576f9e75533201a4578 any more.

/cc @vmg @jamill

[arrbee]

If you have the time to narrow down the range of commits so I can confirm what exactly broke this, that would be helpful. When I changed the code that dealt with finding the root path on Windows filesystems, I would have thought that it would help this type of problem rather than increase it.

However, even without that, I know a fix that will definitely work and I'll prepare it. Unfortunately, this seems to be one of those cases where I can't write a test in the test suite because I can't make a junction from within the tests. The world of Windows filesystem trickery seems to be extremely wide and yet mostly only accessible to system administrators.

[csware]

999d4405a69e8d4d3d3bcd4a5d4bf45d8283b172 is the first bad commit

[arrbee]

Thank you!

[arrbee]

So, the problem here is that this was really only working previously because of a bug in Windows path handling and now it's failing because stat() must either be returning an error code for the junction (which seems unlikely to me) or returning a result such that S_ISDIR is false for the junction point (which also seems crazy to me, but is probably the case).

There are two possible fixes and I've gone with the one that doesn't require me to make a junction. The possible fixes are:

1. Teach git_futils_mkdir what the mode is for a junction so when it finds an existing junction it can treat it the same as an existing directory, or
2. Modify the loose backend so that it uses a base directory when making the new object directory and doesn't try to walk all the way up to the root.

I've gone with 2. See pull request #1669 [edit: fix PR number - linked to the wrong one somehow...]

If you can confirm that that fixes the problem, that would be wonderful. Thank you!

[arrbee]

Sorry, I made a typo with the PR number initially - it should be #1669

[arrbee]

The branch with the changes is at git fetch https://github.com/arrbee/libgit2.git fix-index-add-bypath

[jamill]

What about creating a new repository beneath a junction? Is that currently broken as well? and if so, wouldn't that still be broken after #1669?

[arrbee]

Can we just change Windows so that stat'ing at junction indicates a mode value for which S_ISDIR returns true, at least on Windows? :trollface:

[ben]

We can change S_ISDIR so that's the case. :grinning:

[arrbee]

So, I was joking when I said we should change Windows stat behavior, but going over it more, I think that is actually the correct thing to do.

The problem is that currently in src/win32/posix_w32.c:do_lstat() we treat all FILE_ATTRIBUTE_REPARSE_POINT results as if they were symlinks, but actually that constant will be returned for a variety of Windows file types, including symlinks, hardlinks, and mount points. Reading over Determining Whether a Directory Is a Mounted Folder it appears there are other calls that can be made to delve further into the actual meaning of the reparse point. Instead of always translating to S_IFLNK, if a reparse point is the root of a mounted filesystem, there is no reason for us not to treat it as a directory.

May I state for the record that I find this facet of Windows absurd. The level of complexity and number of variations on these Windows-only filesystem "features" that are presumably used by a fairly small portion of the overall Windows community imposes an undue burden on supporting Windows well. Perhaps someone who knows the ins and outs of this part of Windows and has deeper connections to the Windows community can pick up this issue and run with it, because somehow I can't even find the mklink command on my Win7 VM to start playing around with this shit.

[arrbee]

Even the Apache portability layer contains the comment:

NT5 (W2K) only supports symlinks in the same manner as mount points. This code should eventually take that into account, for now treat every reparse point as a symlink...

[arrbee]

So, do draw out explicitly what I propose for a fix: currently if git_futils_mkdir is asked to make a path, it walks down the path looking to see if the items already exist. If it finds an existing item, it calls p_stat on that item and if it finds a directory then it continues; if it finds something other than a directory, it fails.

I believe (but cannot confirm personally) that p_stat is returning S_IFLNK on Windows when you have a junction. If that is the case, then it is a bug in our implementation of p_stat because stat should follow links and a regular p_stat should never return a link. The ideal solution would be for someone with deeper knowledge of Windows to write a correct version of stat that could differentiate between the various types of reparse points. This also will affect the p_readlink implementation, I think, but you might be able to get away without it.

I'm willing to do this work myself, but for some reason the mklink command doesn't seem to be available on my Windows VM so I'm not exactly sure how to set up the test environments that I'd need to confirm my suspicions. Fortunately, I believe we know a couple of people at Microsoft who might be willing to pursue this.

Please note that the issue with git_index_add_bypath and the issue with git_repository_init at junction points are largely separate, or at least the fix I wrote for git_index_add_bypath is a useful one to make even if we fix the p_stat behavior.

[ethomson]

Is there actually a use case for this or is this a completeness bug? I have never heard of anybody asking for junction point support in their version control system - and that includes tools produced by Microsoft - in the last decade. A quick search shows that there is a connect bug for junction point support in TFS but it only has a single vote.

One would think that we (Microsoft) would be the people to best implement things like this, but that data should suggest its relative priority on our backlog. (This is not even a little bit on my radar right now.)

Does core git actually support this awfulness?

[csware]

Does core git actually support this awfulness?

Git core (aka msysgit) works on a junction (closing, initializing a repo, working on a repo on a junction) - suppose it sees it as a simple directory. However, msysgit has no support for symlinks and fails immediately - that might be the reason why ppl. use junctions.

Junction:

D:\>mkdir d
D:\>mklink /j e d
Verbindung erstellt für e <<===>> d
D:\>cd e
D:\e>git init
Initialized empty Git repository in D:/e/.git/

Symlink:

D:\>mkdir d
D:\>mklink /d e d
symbolische Verknüpfung erstellt für e <<===>> d
D:\>cd e
D:\e>git init
fatal: Invalid symlink 'D:/e': Function not implemented

This is what msysgit does: https://github.com/TortoiseGit/tgit/blob/tgit/compat/mingw.h#L72 https://github.com/TortoiseGit/tgit/blob/tgit/compat/mingw.c#L304

[linquize]

In short, can anybody tell the support statuses ?

1. msysgit+junction
2. msysgit+symlink
3. libgit2+junction
4. libgit2+symlink

[arrbee]

@linquize It's complicated. Some operations in libgit2 are ok and some are not. It's difficult for us because there is no great way to incorporate this stuff into the test suite since you have to be root to set these types of file system structures up. Also, so many variations with completely unique semantics. Do Windows devs not find this a huge pain? Or maybe most devs just never have to deal with the file system like libgit2 does...

[arrbee]

By the way, I'm sorry I didn't mention this earlier but there is a trivial workaround to init a repo under a junction - use git_repository_init_ext and don't pass the flag to MKPATH. That flag is an unfortunate default in the non-ext version (you can actually see my comment to that effect at https://github.com/libgit2/libgit2/blob/development/src/repository.c#L1350 ) but when I wrote init_ext, I did not want to change the old behavior of plain init.

[csware]

msysgit doesn't handle symlinks at all.

[csware]

Btw. this also affect cloning into a junction (d:\e is the junction and clone into d:\e\repo).

libgit2.dll!p_mkdir(const char * path=0x0000000003851c70, unsigned short mode=511) Zeile 26 C
libgit2.dll!git_futils_mkdir(const char * path=0x0000000003851bc0, const char * base=0x0000000000000000, unsigned short mode=511, unsigned int flags=82) Zeile 347  C
libgit2.dll!git_futils_mkpath2file(const char * file_path=0x0000000003851bc0, const unsigned short mode=511) Zeile 19   C
libgit2.dll!git_futils_creat_locked_withpath(const char * path=0x0000000003851bc0, const unsigned short dirmode=511, const unsigned short mode=420) Zeile 79    C
libgit2.dll!lock_file(git_filebuf * file=0x000000000620e760, int flags=8) Zeile 63  C
libgit2.dll!git_filebuf_open(git_filebuf * file=0x000000000620e760, const char * path=0x0000000003870890, int flags=8) Zeile 285    C
libgit2.dll!loose_write(refdb_fs_backend * backend=0x000000000386e6a0, const git_reference * ref=0x0000000003852530) Zeile 803  C
libgit2.dll!refdb_fs_backend__write(git_refdb_backend * _backend=0x000000000386e6a0, const git_reference * ref=0x0000000003852530, int force=1) Zeile 1071  C
libgit2.dll!git_refdb_write(git_refdb * db=0x0000000003115ef0, git_reference * ref=0x0000000003852530, int force=1) Zeile 177   C
libgit2.dll!reference__create(git_reference * * ref_out=0x000000000620ef18, git_repository * repo=0x00000000003b86e0, const char * name=0x00000000038721f0, const git_oid * oid=0x000000000386e5ac, const char * symbolic=0x0000000000000000, int force=1) Zeile 331    C
libgit2.dll!git_reference_create(git_reference * * ref_out=0x000000000620ef18, git_repository * repo=0x00000000003b86e0, const char * name=0x00000000038721f0, const git_oid * oid=0x000000000386e5ac, int force=1) Zeile 367   C
libgit2.dll!update_tips_for_spec(git_remote * remote=0x00000000003ccad0, git_refspec * spec=0x00000000003b8820, git_vector * refs=0x000000000620f048) Zeile 910 C
libgit2.dll!git_remote_update_tips(git_remote * remote=0x00000000003ccad0) Zeile 965    C
libgit2.dll!setup_remotes_and_fetch(git_repository * repo=0x00000000003b86e0, const char * url=0x00000000003c32c8, const git_clone_options * options=0x000000000620f150) Zeile 388  C
libgit2.dll!git_clone(git_repository * * out=0x000000000620f2b8, const char * url=0x00000000003c32c8, const char * local_path=0x00000000003c34d8, const git_clone_options * options=0x000000000620f360) Zeile 459   C

[carlosmn]

Is there actually a use case for this or is this a completeness bug?

Apparently people try to use tools like repo on Windows, which assumes you have a symlink-capable filesystem to even work (and only works on Windows under cygwin to begin with). So people go and replace the repo symlinks with junctions and whatnot.

It doesn't seem like a stretch that TFS users don't deal with as many tools that assume a symlink-capable filesystem. Often there are ways of telling git to go look elsewhere instead of symlinking, but making things portable is hard.

[csware]

initialization and cloning are some kind of completeness bugs, I can't test fetching/pulling/resettung atm - these are real use cases. Since msysgit doesn't support symlinks I suppose some ppl. use junctions as I do.

[arrbee]

I've added a possible workaround into #1669. I'm making it so that as we construct a directory path in git_futils_mkdir if we encounter an existing entry, we will accept either S_ISDIR or S_ISLNK elements as valid intermediate entries. I didn't change the second S_ISDIR test that is executed if the GIT_MKDIR_VERIFY_DIR flag is given because that is significantly riskier (i.e. if you have a link to a file, not a directory, then the function would no longer be working correctly), but changing this verification of intermediate existing path components should help with most of the reparse-point cases.

It is still my long-term preference that we fix p_stat so that it returns "correct" results - i.e. examines the reparse point and correctly expands it to either be a regular file or a directory. A conforming p_stat should never return S_ISLNK - that is reserved for p_lstat.

I'm not 100% certain that this fixes the problem because, as I've mentioned, my Windows VM seems to be oddly misconfigured, but I believe that the change I'm proposing will fix the most common cases. I suspect you still will not be able to create a repository as a direct child of a reparse point because of the final S_ISDIR check in the function, but having a reparse point in the path to a repository should no longer be a problem for any other functions.

[csware]

I debugged a bit futher. The problem seems to be "!(S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode))". If I run p_stat on a junction fMode (in do_lstat) is 57728 (S_IREAD | S_IFDIR | S_IWRITE | S_IFLNK). However S_ISDIR(st.st_mode) and S_ISLNK(st.st_mode)) are both 0.

Changing to "!(S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode) || (st.st_mode & S_IFDIR) != 0))" seems to solve all my issues reported here.

Other possible (cleaner) options could be to change "if (fdata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)" to "if (fdata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT && (fMode & S_IFDIR) == 0)" or adding

        if ((fMode & (S_IFDIR | S_IFLNK)) == (S_IFDIR | S_IFLNK)) // junction
            fMode ^= S_IFLNK;

to do_lstat in posix_w32.c.

Decide what you like and I can create a pull request.

[arrbee]

Interesting. Thanks for the debugging! So, we should definitely fix the p_stat code. It should never set both the LNK and DIR bits for the same file. DIR should take precedence, I think, and then we can revert the check for S_ISLNK in mkdir.

[csware]

See PR #1680

[arrbee]

Thank you @csware for coming up with a fix for p_stat so it will not return both S_ISDIR and S_ISLNK for junctions! That should make the worst of the problems go away across many different functions.

I think I'll close this issue (since git_index_add_bypath should work correctly now) and if we find particular APIs that don't work correctly with junctions, let's open new issues to document the specific problems we find.