Skip to content
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

Support core.longpaths on Windows #5347

Closed
wants to merge 10 commits into from

Conversation

ianhattendorf
Copy link
Contributor

@ianhattendorf ianhattendorf commented Dec 31, 2019

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

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
Copy link
Contributor Author

/rebuild

@libgit2-azure-pipelines
Copy link

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

@ianhattendorf
Copy link
Contributor Author

ianhattendorf commented Dec 16, 2020

@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.)

@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 commented May 6, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants