-
Notifications
You must be signed in to change notification settings - Fork 315
Conversation
@frogonwheels GitHub really makes collaborative reviewing if not fun, then at least less tedious... So what's with these !TODO and !attention commits? Do you want this branch to be merged in the current form, or rather have some general feedback? |
@@ -148,6 +144,12 @@ static inline int mingw_SSL_set_wfd(SSL *ssl, int fd) | |||
#define SSL_set_wfd mingw_SSL_set_wfd | |||
#endif | |||
|
|||
#ifdef symlink_ex | |||
#undef symlink_ex | |||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIR such #ifdef guards are unnecessary. If symlink_ex
is not defined, #undef symlink_ex
will not trigger an error nor warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy fixed. Will of course test it.
I had a quick look over the accumulated diff. Please keep in mind that my main goal is, in a manner of speaking, to let you have your cake and at the same time avoid eating it myself. In other words, I would love the symlink support to work for you, but be switched off by default -- and have Git work exactly the same as without having symlink support on Windows in that case. And I think that we can reach that goal, but we're not quite there yet. |
MSysGit - the development behind Git for Windows » git #199 SUCCESS |
I included this in my email - but just to be complete: I do agree, but can I just clarify that the ability for git to understand symlinks that are For example this should work regardless of the setting: mkidr myrepo && cd myrepo && git init
mkdir test
cd ..
rem now we are outside the repo
rem make a link into the repo.
mklink /d test myrepo\test
rem CD to the symlink
cd test
echo.hello>file
rem This part is that bit that only works with my changes present.
git add file
git commit -m "my message" This does NOT involve symlinks in the actual working dir/ git repo, and imho should Do you have certain broken behaviours that you would like to remain broken? |
I agree that reparse points generated outside Git for Windows should ideally work with Git for Windows. I just need to make sure that nothing in this branch affects my (reparse point free) system. Git for Windows should work exactly as before on my system. |
Ok, that's fair enough. Even with I've been using this patch series in development for a couple of years now. (On my machine only) |
Okay, just so you know about my idea of the upcoming Git for Windows 1.9.3: I just pushed a merging-rebased If you could address the issues we discussed so far (basically: keep as many changes out of non-Windows-specific files as humanly possible, structure commits less by file and more by purpose) and force-push, that would be grand! |
MSysGit - the development behind Git for Windows » git #205 SUCCESS |
MSysGit - the development behind Git for Windows » git #208 FAILURE |
MSysGit - the development behind Git for Windows » git #209 SUCCESS |
Hmpf. I have too little time these days. Maybe I'll manage a full review tonight, because I really want to get 1.9.3 out by tomorrow night. If I won't manage, I am afraid that it will have to wait for 1.9.4... |
I really appreciate the time you've put into this, and if postponing it is what you need to do, then do so. I've thought more about fa9215f and I might rewrite as you've suggested. I'm also thinking I can pull the definition of |
@frogonwheels thank you, also for your patience with me. I am unfortunately very busy, and my poor judgement let me spend my Git time budget on the Git mailing list yesterday. I have added a few comments here and there, it would be very nice if you could address them. Apart from those comments, I only need to run the test suite here (after the current run from the current state of our |
Oh, that's too sad. I merged into |
@frogonwheels could you have a look at the I ran out of Git time again (I actually went seriously on overdrawn), but I figured out that after running
I really fear that the problematic line might be (Quick last update for today: now |
While I was working on things I really need to work on, the test suite finished: |
Are you doing this on a system with or without symlinks? I've not got time today to work on this, so no rush. Also How do you do a |
@frogonwheels yes, I am doing this without symlinks. The whole point of my side of the review was to ensure that nothing gets broken on systems which do not enable the "symlink" feature (which would better be called the "NTFS reparse point" feature, because Microsoft just had to make a mess by inventing something just incompatible enough so as to cause more problems than are being solved). Exhibit no. 1: the test failures (caused by the requirement to work around reparse point limitations, unfortunately affecting my system that does not even have a single reparse point).
Well, I actually wanted to release 1.9.3 on Monday, then on Wednesday, and now it is Saturday. I'm unsure whether we should wait for this topic branch to be included... It would be nice to have such a big feature, but we really must avoid any regression: I don't have enough time to maintain this project according to Boyd's law of iteration...
I typically test with |
@dscho - OK. It's a bit strange these failures without symlinks are happening, there's obviously something I'm missing. I guess I'm going to have enable UAC and find it out. My point about there being no rush was that maybe it's time to release 1.9.3 without this branch. If there are these failures happening with symlinks disabled, then obviously I need to run through some tests with symlinks disabled since something funky is going on somewhere. I wonder what your core.symlinks says? Does forcing core.symlinks to 'false' fix the issues? If so, that might help work out what is happening with these failures. Ahh.. I was using |
I think the reason might be the way we override
Git's test suite runs with |
So I tried using |
@frogonwheels while it is sad that there are failures, it is good that you can reproduce my issues ;-) Please keep me posted. I am currently under serious time constraints, but I care enough about this issue to postpone some pressing projects, if you need help. |
@frogonwheels do you have an idea whether more output by BuildHive would help you? I could, for example, change the job such that it runs with certain environment variables set... Or you could temporarily insert a |
MSysGit - the development behind Git for Windows » git #230 FAILURE |
This patch implements git support for NTFS symbolic link type reparse points. * There is a specific privelege required to create symbolic links that is not generally associated with a standard user. This part is up to the user to worry about. * NTFS reparse points differentiate between file and directory links. This patch assumes file links are meant. (A separate patch will develop this further). * This patch is not intended to implement symbolic links in the shell utilities. This means that as of when this was written, bash and gnu utilities do not handle them. * Windows chdir behaves differently to *nix, and we need to unravel symbolic links for various operations to work as expected. * For efficiency, as much as possible of the calls are done with wchar_t, before being converted to utf-8. This is as much about avoiding dealing with windows default encoding as anything else. * resolve_symlink needed to be replaced in lockfile.c since there are some issues with recognising absolute paths, as well as for efficiency with wchar_t. This work was based on a combination of patches developed by the following people: original-by: Johannes Schindelin <[email protected]> original-by: Thorvald Natvig <[email protected]> Signed-off-by: Michael Geddes <[email protected]>
Required for msysgit which needs to know whether the target of a symbolic link is a directory or file. Signed-off-by: Michael Geddes <[email protected]>
Windows NTFS symbolic links require specifying whether the target is a file or directory at creation time. This can partially be done by interrogating the filesystem, however when creating symbolic links that are within a repository, there can be no guarantee of the order of creation. This patch enables the code to allow interrogation of the git cache to determine whether a target will be a file or a directory. Signed-off-by: Michael Geddes <[email protected]>
The current msys does not have support for NTFS symlinks. This includes in 'ln', 'test', 'rm'. Here the commands are overridden to use windows cmd.exe instead of msys. The implementation of test is quite expensive but works for the test. 'rm' needs to handle many scenarios. Symlinks are not supported at all by the msys version. * 'rm -rf' equivalent needs to first delete the files, and then the directories. * Directory symbolic links need to use cmd.exe 'rmdir' * file symbolic links need to use 'del'. * A couple of places use 'rm -rf' on a mix of files and directories, so needs to be split up. Signed-off-by: Michael Geddes <[email protected]>
There are 2 reasons for this: * Under windows, ln() must determine whether the target is a directory or a file before creating the link. * msys doesn't handle dangling NTFS symlinks well (for example 'ls' will abort at that point, claiming the file doesn't exist). So I have reordered commands to make sure our symlinks don't dangle from the time of their creation. Signed-off-by: Michael Geddes <[email protected]>
Signed-off-by: Michael Geddes <[email protected]>
needed to translate /c/ -> c: for comparison. Signed-off-by: Michael Geddes <[email protected]>
Required by msysgit while the msys core does not handle reading NTFS Symbolic-Link Reparse Points. Signed-off-by: Michael Geddes <[email protected]>
Signed-off-by: Michael Geddes <[email protected]>
Required for tests to work. Signed-off-by: Michael Geddes <[email protected]>
t7800 failed under symlink enabled msysgit without symlink enabled perl/msys Signed-off-by: Michael Geddes <[email protected]>
Failure of test_when_finished caused test to fail required for overridden rm () to work without error Signed-off-by: Michael Geddes <[email protected]>
MSysGit - the development behind Git for Windows » git #231 FAILURE |
MSysGit - the development behind Git for Windows » git #232 FAILURE |
MSysGit - the development behind Git for Windows » git #233 SUCCESS |
As mentioned on the mailing list, I would like to skip FWIW my VM is still running the tests, after more than four hours still not done... :-( On a positive note, all of the tests so far are |
In msysgit there were some crashes caused by the line conversions when comparing binary files. I've also used the patch originally by Stepan Kasal to extend coverage of binary compares. Signed-off-by: Michael Geddes <[email protected]>
The test was using the wrong testgit variable helper to cause the failure. I still don't know why this test wasn't failing on all machines. Signed-off-by: Michael Geddes <[email protected]>
MSysGit - the development behind Git for Windows » git #234 SUCCESS |
The build seems to be much happier now. The speed for tests is worrying. I started trying to find a way to make it quicker, but hit some bumps. I'm assuming that the time overhead of using an internal CMD.exe command vs the time overhead of using rm.exe is our problem. (Just did a quick measure, cmd.exe takes 4* the time). Bleh. The ideal way to do it is to use a version of msys with symlink support. |
I should have mentioned that a full virus scan was running at the time... ;-) And it is a VM that gets only one core and only 2GB RAM. I was promised a much better solution in the near future, looking forward to that. It ran through, for the first time in my life with no unfinished jobs... ;-)
I agree, of course (except I still would call it "reparse point" support or "symlink emulation" because symlinks are a well-established Unix paradigm that is only partially implemented by the reparse points). MSYS2 seems to support that, so once we switch to the mingwGitDevEnv (or Git for Windows SDK, or Git for Windows for real men, or whatever you want to call it), I have a hunch that we might be able to offer MSYS2 as an option. But I'd still like to keep the default with MSYS1 just because it is so extensively tested. Anyway, after this long sermon: Thank you so much for your hard work , your continued patience with me, and your incredible achievement! |
Hello |
As an end-user, after reading through a ton of StackOverflow questions and various issues in this repo I'm super-confused about how to get a repo with symlinks to clone correctly on Windows (that is, replicating the symlinks). I run git as the Admin user and I've just installed the 2015-03-19 build. |
@dandv please use the mailing lists for help with using Git; we like to keep the bug tracker for discussing bugs, and those discussions are also much easier to juggle if they stay on topic. |
No description provided.