-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redefine PATH_MAX to allow all Git operations to take advantage of Windows long path support. #4848
Conversation
eb71cb8
to
4445a02
Compare
…ndows long path support. Signed-off-by: Lucas Zadrozny <[email protected]>
4445a02
to
bcdaacd
Compare
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.
As mentioned below, the commit message needs improving, because it needs to convince the reader that this is a desirable change. A couple of important things are still unclear to me:
- Did you mean to mention
Computer\HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabled
in the commit message? I think it would be highly advisable to do so. - What is the
PATH_MAX
value going to be if we#undef
it? - What is the consequence of running this with a long path when
core.longPaths=true
andComputer\HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabled = 0
? - What is the consequence of running this with a long path when
core.longPaths=true
andComputer\HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabled = 1
? - What is the consequence of running this with a long path when
core.longPaths=false
andComputer\HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabled = 0
? - What is the consequence of running this with a long path when
core.longPaths=false
andComputer\HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabled = 1
?
* | ||
* Redefining this macro addresses that issue. | ||
*/ | ||
#if defined(_WIN32) || defined(__CYGWIN__) || defined(__MINGW32__) |
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.
I can understand most of this, but since you are contributing this to the Git for Windows project, I don't think it would be appropriate to include the _CYGWIN
part.
Also, the commit message is a bit too mum about rationale. Please follow the guidance in https://github.blog/2022-06-30-write-better-commits-build-better-projects/ to improve it, in particular with a strong focus on this part:
What you’re doing | Why you’re doing it | |
---|---|---|
High-level (strategic) | Intent (what does this accomplish?) | Context (why does the code do what it does now?) |
Low-level (tactical) | Implementation (what did you do to accomplish your goal?) | Justification (why is this change being made?) |
|
Right. Thank you for looking it up, I vaguely remembered something like that. However, my point was that the commit message needs improving. But talking back and forth between reviewers won't fix that. |
@lucasz93 feel free to respond to the reviews. We can reopen this PR then. |
@dscho |
No. |
PATH_MAX is inherited from mingw64/include/limits.h, with a value of 260.
The problem is that even after enabling Windows long path support and 'core.longpaths' there are still some Git operations that will fail, such as cloning a submodule (see setup_explicit_git_dir).
I've observed some CI runners that have temp clones in a deeply nested path which will break even some medium sized repository paths.
Redefining this macro addresses that issue.
This does seem very hacky, but it was a big issue for my Windows Bitbucket runner which clones into a ridiculously long path of:
C:/Windows/System32/atlassian-bitbucket-pipelines-runner/temp/b00863ba-0d50-5cf5-91e6-452e9580164b/1709559465808/build
. This caused some of my dependent submodules to fail initialisation.Examples of the same issue:
aws/aws-iot-device-sdk-cpp-v2#157
gruntwork-io/terragrunt#2343
If there's another possible way to implement this - I'm open to ideas. Would just like to see this issue go away :)