rmdir: align behavior on Windows when called on symlinks

[thombet]

Fixes #2967

When performing a rebase, rmdir() is called on the folder .git/logs. On Linux rmdir() exits without deleting anything in case .git/logs is a symbolic link but the Windows equivalent (_rmdir) does not behave the same and removes the folder if it is symlink which generates issues especially when Repo is used.
This commit updates mingw_rmdir() so that its behavior is the same as Linux rmdir() in case of symbolic links.

Some remarks:

• I asked confirmation to Microsoft about the actual behavior of _rmdir() with symlinks but I didn't get an answer yet so my conclusions are only based on what I saw while debugging on Windows 10 with Git Bash.
• I created a "rebase" test case because this problem was found initially when performing "git rebase" but in the end the commit applies to mingw_rmdir() in general. Moreover I think it is in line with this part of the contribution guidelines: The Git community prefers functional tests using the full git executable, so try to exercise your new code using git commands before creating a test helper. But if you think a test case dedicated to mingw_rmdir() should be created, please let me know.
• Kudos to @rimrul for helping me choose the correct value for errno :)

[dscho]

Could you move this to before the loop? The loop retries to delete a directory in case it is still in use, and it does not really make sense to ask over and over again whether it is a symbolic link or not.

Also, regarding the commit message:

issues especially when Repo is used.

The repo tool is not shipped with Git for Windows, and is actually not all that known outside of a small circle of Git users. It would make sense to elaborate a bit on what it is, and link to its homepage (if it has one).

Speaking of the commit message, I would love to see a paragraph added that talks about RemoveDirectoryW() having the same problem as _wrmdir(), and it is even documented:

RemoveDirectory removes a directory junction, even if the contents of the target are not empty; the function removes directory junctions regardless of the state of the target object. For more information on junctions, see Hard Links and Junctions.

Well, technically it does not talk about reparse points in general but only specifically about junctions. But the point is that we cannot expect that function to treat reparse points in the way we want, in general, and therefore we must take manual precautions (and there is no RemoveDirectoryExW() that would hypothetically let the caller specify whether it is okay to delete reparse points).

Since that documentation also talks about hard links, I briefly investigated whether _wrmdir() could potentially remove hardlinks by mistake. But hardlinks can only link to files, not directories, and _wrmdir() correctly errors out upon trying to remove hardlinks.

[thombet]

I moved the code and I updated the commit message based on your recommendations.
Please let me know if it is what you had in mind.

[dscho]

I like the new commit message!

[dscho]

As you pointed out in the comment above, ln -s won't work by default in an MSYS setup. However, that is exactly what all of the CI/PR builds run.

As a consequence, I fear that this test script will only ever run on Linux/macOS/FreeBSD, where we know that things work.

A better idea might be to add a new test case to an existing t34* script, guarded by the MINGW prereq, that first verifies that it can create a symbolic link, and that does not use ln -s for it, but mklink /s to do so, and if it cannot create the symlink, simply renames the logs directory back, otherwise runs through the simple rebase and verifies that the symbolic link still exists.

[thombet]

I decided to create the SYMLINKS_WINDOWS prerequisite based on the SYMLINKS prerequisite which uses mklink instead of ln in case it is useful to someone else in the future.

[thombet]

Thank you for the review and the very interesting suggestions @dscho !
I will take them into account and publish an update.

What is the policy in this repository for updates of PR which are not independent commits? Should I amend and force push my update? Or should I push another commit and you or I will squash them before merging the PR?

[dscho]

Please amend and force-push. The cleaner the commit history, the easier it will be for me to upstream those changes.

[dscho]

Apart from the code comment in t3400, I think this is ready to go!

[dscho]

I like the new commit message!

[dscho]

IMHO we do not need this code comment (it would be the first such code comment in t3400, I believe).

[thombet]

I think comments are most of the time useful but you're the boss here so I removed them 😉

[thombet]

Comments removed, commit amended, branch rebased and PR updated.
Thanks for your guidance 👍

[dscho]

Very nice!

[thombet]

@dscho some tests failed in the CI: is it expected or should I analyze the failure?

[rimrul]

The failing test is t5702.57 on macOS (gcc only). I don't really see how the changes in this PR could have caused that.

[dscho]

The failing test is t5702.57 on macOS (gcc only). I don't really see how the changes in this PR could have caused that.

It looks like a segmentation fault of one sort or other. It is admittedly not yet easy enough to get to the full log of the actual failure. You have to expand the "ci/print-test-failures.sh" task's log and then search for "not ok". Here it is: https://github.com/git-for-windows/git/runs/3191969335#step:5:20818

++ echo 'test_must_fail: died by signal 13: git -c protocol.version=2 -c transfer.fsckobjects=1 -c fetch.uriprotocols=http,https clone http://127.0.0.1:5702/smart/http_parent http_child'
test_must_fail: died by signal 13: git -c protocol.version=2 -c transfer.fsckobjects=1 -c fetch.uriprotocols=http,https clone http://127.0.0.1:5702/smart/http_parent http_child
++ return 1
error: last command exited with $?=1
not ok 57 - packfile-uri with transfer.fsckobjects fails on bad object

I think I saw this before, but if that is the case then it is a pretty rare flake. I just re-ran the build.

[dscho]

I just re-ran the build.

... and it figured out that there was a successful build already with the same tree, so it skipped it with success: https://github.com/git-for-windows/git/runs/3199628097

Which means that indeed, this is a flake, and definitely not the fault of this PR. @thombet thank you for being so diligent and to offer to investigate further!

[thombet]

@thombet thank you for being so diligent and to offer to investigate further!

You're welcome, I don't want to be the one who breaks Git 😅