FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Support `core.longpaths` on Windows by ianhattendorf · Pull Request #5347 · libgit2/libgit2 · GitHub

/ libgit2 Public

Support core.longpaths on Windows - #5347

Closed
ianhattendorf wants to merge 10 commits into
libgit2:mainfrom
ianhattendorf:feature/longpaths
ConversationCommits (10)ChecksFiles changed
Closed

Support core.longpaths on Windows#5347
ianhattendorf wants to merge 10 commits into
libgit2:mainfrom
ianhattendorf:feature/longpaths

Conversation

ianhattendorf commented Dec 31, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 force-pushed the feature/longpaths branch 3 times, most recently from 25c1a29 to c87570a Compare December 31, 2019 19:51

ethomson commented Dec 31, 2019

Copy link
Copy Markdown
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 force-pushed the feature/longpaths branch 3 times, most recently from dcb8bef to 557fd6f Compare January 10, 2020 20:06
ianhattendorf force-pushed the feature/longpaths branch 2 times, most recently from ec9cd39 to 99edaed Compare January 13, 2020 18:27
ianhattendorf changed the title WIP Support core.longpaths on Windows Support core.longpaths on Windows Jan 13, 2020
ianhattendorf marked this pull request as ready for review January 13, 2020 21:09

ianhattendorf commented Jan 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 mentioned this pull request Jan 13, 2020
Merged
ianhattendorf added a commit to ianhattendorf/nodegit that referenced this pull request Jan 13, 2020
Add longpath options to NodeGit.Libgit2.OPT
e4e66f4 
This should be removed once libgit2/libgit2#5347 is merged
implausible mentioned this pull request Jan 31, 2020
Open
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
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
Bump Libgit2 to fork of v0.99
bfbe2d3 
Includes unmerged PRs:
- libgit2/libgit2#5384
- libgit2/libgit2#5347
- libgit2/libgit2#4205
implausible mentioned this pull request Feb 26, 2020
Closed

ianhattendorf commented Feb 26, 2020

Copy link
Copy Markdown
Contributor Author

/rebuild

libgit2-azure-pipelines Bot commented Feb 26, 2020

Copy link
Copy Markdown

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

DynConcepts mentioned this pull request Mar 8, 2020
Closed
snyk-bot mentioned this pull request Mar 12, 2020
Merged
snyk-bot mentioned this pull request Apr 22, 2020
Open
snyk-bot mentioned this pull request May 5, 2020
Open
ianhattendorf force-pushed the feature/longpaths branch 3 times, most recently from 33f6144 to 40a3c13 Compare May 28, 2020 22:58
badrelmers mentioned this pull request Jun 1, 2020
Open

ianhattendorf commented Dec 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 deleted a comment from pde-cds Apr 14, 2021
libgit2 deleted a comment from Rossbro2 Apr 14, 2021
libgit2 deleted a comment from navono Apr 14, 2021
libgit2 deleted a comment from cjmanca Apr 14, 2021
libgit2 deleted a comment from SHAZAM-Analytics Apr 14, 2021
libgit2 deleted a comment from pde-cds Apr 14, 2021
libgit2 deleted a comment from gigi81 Apr 14, 2021
libgit2 deleted a comment from pde-cds Apr 14, 2021
libgit2 deleted a comment from japj Apr 14, 2021

ethomson commented Apr 14, 2021

Copy link
Copy Markdown
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 commented May 5, 2021

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

ethomson closed this May 6, 2021
pde-cds mentioned this pull request Jul 20, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL