8000 Support `core.longpaths` on Windows by ianhattendorf · Pull Request #5347 · libgit2/libgit2 · GitHub
[go: up one dir, main page]
Skip to content

Support core.longpaths on Windows #5347

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 10 commits into from
Closed

Support core.longpaths on Windows #5347

wants to merge 10 commits into from

Conversation

ianhattendorf
Copy link
Contributor
@ianhattendorf ianhattendorf commented Dec 31, 2019 •
edited
Loading

Initial test run at supporting paths > 260 on Windows (#3053). Works as far as I can tell (clone, commit, checkout, etc.) but definitely needs more testing.

TODO:

  • Do we need to respect core.longpaths, or can we simply always support long paths? I'm assuming we need to respect it (https://xkcd.com/1172/), but AFAIK this exists because CLI Git for Windows doesn't support long paths across all commands/internal scripts, and is seen as experimental.
  • Potentially keep static buffers at 260, dynamically allocate past that. Is an increase in path static buffer size from 532/778 bytes to 8204/12286 bytes too much for Win32? Static is nice here since there is no allocation performance penalty regardless of whether short or long paths are used. Do we support variants of Windows with small stack sizes (e.g. Windows CE)?
  • Tests tests tests tests tests
  • Test cleanup fails (DeleteFileW(buffer))

@ianhattendorf ianhattendorf force-pushed the feature/longpaths branch 3 times, most recently from 25c1a29 to c87570a Compare December 31, 2019 19:51
@ethomson
Copy link
Member

Awesome!

Do we need to respect core.longpaths, or can we simply always support long paths?

I think we should honor it. There are just too many tools on windows that don’t understand long paths that this should be opt in so that people who shoot themselves in the foot had to explicitly load the gun.

@ianhattendorf ianhattendorf force-pushed the feature/longpaths branch 3 times, most recently from dcb8bef to 557fd6f Compare January 10, 2020 20:06
@ianhattendorf ianhattendorf force-pushed the feature/longpaths branch 2 times, most recently from ec9cd39 to 99edaed Compare January 13, 2020 18:27
@ianhattendorf ianhattendorf changed the title WIP Support core.longpaths on Windows Support core.longpaths on Windows Jan 13, 2020
@ianhattendorf ianhattendorf marked this pull request as ready for review January 13, 2020 21:09
@ianhattendorf
Copy link
Contributor Author
ianhattendorf commented Jan 13, 2020 •
edited
Loading

A few updates:

  • Instead of supporting core.longpaths, we respect the option GIT_OPT_SET_WINDOWS_LONGPATHS. This allows some configurability, without threading core.longpaths through every function call.
  • I added a new devops job to test Windows 64 bit Visual Studio with longpaths enabled. I didn't want to modify any existing Windows jobs since longpaths off is still the default, however wanted to verify nothing else breaks when they're enabled. Let me know if this is too much additional CI time or if there's a better option.

I'm sure this still needs more tests for the longpaths case, however I believe it's ready to go functionally.

@ianhattendorf ianhattendorf mentioned this pull request Jan 13, 2020
Merged
ianhattendorf added a commit to ianhattendorf/nodegit that referenced this pull request Jan 13, 2020
@ianhattendorf
Add longpath options to NodeGit.Libgit2.OPT
e4e66f4 
This should be removed once libgit2/libgit2#5347 is merged
@implausible implausible mentioned this pull request Jan 31, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Feb 23, 2020
Open
implausible added a commit to implausible/nodegit that referenced this pull request Feb 25, 2020
@implausible
Bump Libgit2 to fork of v0.99
5afc67e 
Includes unmerged PRs:
- libgit2/libgit2#5384
- libgit2/libgit2#5347
- libgit2/libgit2#4205
implausible added a commit to implausible/nodegit that referenced this pull request Feb 26, 2020
@implausible
Bump Libgit2 to fork of v0.99
bfbe2d3 
Includes unmerged PRs:
- libgit2/libgit2#5384
- libgit2/libgit2#5347
- libgit2/libgit2#4205
@implausible implausible mentioned this pull request Feb 26, 2020
Closed
@ianhattendorf
Copy link
Contributor Author

/rebuild

@libgit2-azure-pipelines
Copy link

Sorry @ianhattendorf, an error occurred while trying to requeue the build.

@DynConcepts DynConcepts mentioned this pull request Mar 8, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request Mar 12, 2020
Merged
@snyk-bot snyk-bot mentioned this pull request Apr 22, 2020
Open
@snyk-bot snyk-bot mentioned this pull request May 5, 2020
Open
@ianhattendorf ianhattendorf force-pushed the feature/longpaths branch 3 times, most recently from 33f6144 to 40a3c13 Compare May 28, 2020 22:58
@badrelmers badrelmers mentioned this pull request Jun 1, 2020
Open
@ianhattendorf
Copy link
8000
Contributor Author
ianhattendorf commented Dec 16, 2020 •
edited
Loading

@ethomson rebased onto latest master. I'm sure you're busy with the holidays coming up, when you have the time it would be great if you could leave any feedback you have.

Base automatically changed from master to main January 7, 2021 10:09
@libgit2 libgit2 deleted a comment from pde-cds Apr 14, 2021
@libgit2 libgit2 deleted a comment from Rossbro2 Apr 14, 2021
@libgit2 libgit2 deleted a comment from navono Apr 14, 2021
@libgit2 libgit2 deleted a comment from cjmanca Apr 14, 2021
@libgit2 libgit2 deleted a comment from rupreck Apr 14, 2021
@libgit2 libgit2 deleted a comment from pde-cds Apr 14, 2021
@libgit2 libgit2 deleted a comment from gigi81 Apr 14, 2021
@libgit2 libgit2 deleted a comment from pde-cds Apr 14, 2021
@libgit2 libgit2 deleted a comment from japj Apr 14, 2021
@ethomson
Copy link
Member

This depends on #5823.

I'm deleting "me too!" or "why isn't this merged yet!" comments. Please don't post those. (If you want this feature as-is, include this PR in your own build of libgit2.)

This was referenced May 2, 2021
Merged
Merged
@ethomson
Copy link
Member
ethomson commented May 5, 2021

I opened #5857 with this, and a few minor changes. Of note:

  1. Our constants are GIT_something, so WIN_GIT_PATH_MAX should be GIT_WIN_PATH_MAX or something similar.
  2. clar/fs.h shouldn't depend on libgit2 directly, if possible - it should stay standalone so that we can pull in upstream changes whenever we need to.
  3. Since we've added support for enforcing core.longpaths in Working directory path validation #5823, we can remove the GIT_OPT_SET_WINDOWS_LONGPATHS option and enforce core.longpaths on a repository-by-repository basis.

@ethomson
Copy link
Member
ethomson 88A4 commented May 6, 2021

Thanks for this @ianhattendorf! I'm going to bring it in over at #5857! 🎉

@ethomson ethomson closed this May 6, 2021
@pde-cds pde-cds mentioned this pull request Jul 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ianhattendorf @ethomson
0