Revert #86 and #111 #114
Revert #86 and #111 #114
Conversation
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
@vangdfang if you don't mind, I'd like to start again, from this pull request. @kblees could you have a look, in particular at the test case so that we all agree what this pull request is supposed to fix eventually? |
|
MSysGit - the development behind Git for Windows » git #162 SUCCESS |
| @@ -0,0 +1,101 @@ | |||
| #!/bin/sh | |||
| # | |||
| # Copyright (c) 2013 Doug Kelly | |||
There was a problem hiding this comment.
Hehe... You probably wrote it in 2013.
There was a problem hiding this comment.
Actually, no, I just came up with the testcase yesterday. :( I just haven't admitted to myself it's the new year yet.
|
I'd prefer not going this route, but understand if you're trying to get a new 1.8.5 release out the door. I've been tossing other ideas around in my head all night, but I simply can't come up with anything much better, so the solution would still remain the same. |
|
@vangdfang the idea of a restart would be to be able to address the fundamental issues with #86 rather than patching up after it... (keeping in mind that we actually want to come up with a clean topic branch in https://github.com/msysgit/git that we can contribute upstream). |
| done | ||
| # Pick a substring maximum of 90 characters | ||
| # This should be good, since we'll add on a lot for temp directories | ||
| export longpath=${longpath:0:90} |
There was a problem hiding this comment.
Also just noticed in CodingGuidelines this substring syntax is not preferred.
There was a problem hiding this comment.
Easily fixed by export longpath="$(echo "$longpath" | cut -c 1-90)". The problem is that only bash understands this syntax.
In related news, I think that the whole block might gain readability by replacing with
string45=0123456789abcdefghijklmnoprstuvwxyz0123456789
longpath=$longpath$longpathThere was a problem hiding this comment.
Also: why 90? That is too arbitrary a number without any further explanation.
There was a problem hiding this comment.
I'm a fan of the second you proposed. The number 90 came out because you have to stay under 260 with total path length, and after c:\msysgit\git\t\trash directory.t7410-submodule-long-path\ -- you've already got a lot of length used. 90 (just from testing) happened to be a pretty good number to compensate for this. I suppose this test could be marked for msysgit only (and then the length of pwd -W could be subtracted to find available length), but there's also some value on other platforms to the test (just maybe not as much in its current form).
I'm seeing more layers to this problem as I look beyond the mingw porting layer, since large portions of Git use PATH_MAX as the size of a stack variable to store paths (and sometimes, there's additional space reserved for other characters -- in path.c, there's even an extra 100 character padding reserved with submodules, which could easily explain why this so quickly hits the path limit -- you get bad_path arbitrarily early). Other places use strbufs to hold the path, and just assign PATH_MAX for a default size. Ultimately, I think the "correct" fix is to use strbuf more liberally, as this could increase the path length limit arbitrarily, even on other platforms (such as GNU/Hurd, which doesn't have a maximum path length) -- basically, http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html seems to be rather relevant in this scenario. Also, I think this may be more palatable to the upstream maintainers, since it's not just a one-off Windows fix, but an enhancement for all platforms. I know you and the other commenters are also actively involved in upstream development, so certainly, your opinions on this matter are welcome. But, in terms of working towards the ultimate goal of how to achieve long file support in Windows, this seems like the best step forward.
This maintainer merged the pull request in a bit of a rash rush. This pull request is meant to allow us to restart the process of fixing the problem #86 tried to fix: overly long submodule paths.