pack-refs --all fails when temporary .lock file is longer than MAX_PATH but ref name is shorter than MAX_PATH (core.longPaths enabled)

[kevin-david]

Setup

• Which version of Git for Windows are you using? Is it 32-bit or 64-bit?

> git --version --build-options
git version 2.13.2.windows.1
built from commit: 914b84c9d949b14a135ab50c8b881cdee9be116c
sizeof-long: 4
machine: x86_64

• Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?

> cmd.exe /c ver
Microsoft Windows [Version 10.0.16193.1001]

• What options did you set as part of the installation? Or did you choose the
  defaults? Defaults, I think:

Path Option: Cmd
SSH Option: OpenSSH
CURL Option: WinSSL
CRLF Option: CRLFCommitAsIs
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Enabled
Enable Symlinks: Enabled

• Any other interesting things about your environment that might be related
  to the issue you're seeing?

> git config core.longPaths
true

If the full ref path is longer than 260 characters, everything works as intended. This only seems to repro when the full ref path is under 260 without .lock but over 260 with .lock

Details

• What commands did you run to trigger this issue?

From a git bash prompt, assuming you can use /d/ as a temporary drive path for testing...

cd /
mkdir -p /d/FileCache/Proxy/file/
cd /d/FileCache/Proxy/file/
git clone https://github.com/kevin-david/test-shallow.git 449f5d8c23864896ba60549eaf335291
cd 449f5d8c23864896ba60549eaf335291/.git/
mv * .. 
git config core.bare true
cd .. 
echo "# pack-refs with: peeled fully-peeled" > packed-refs
git fetch
git pack-refs --all

• What did you expect to occur after running these commands?

pack-refs --all succeeds

• What actually happened instead?

$ git pack-refs --all
error: cannot lock ref 'refs/remotes/origin/user/aschoorl/cherrypick-change-play-event-to-take-audioplaybackevents-as-parameter-on-official-rs_apps_comms-on-user-stevek/audioplayer_new-revert-from-user-stevek/audioplayer_new': Unable to create 'D:/FileCache/Proxy/file/449f5d8c23864896ba60549eaf335291/./refs/remotes/origin/user/aschoorl/cherrypick-change-play-event-to-take-audioplaybackevents-as-parameter-on-official-rs_apps_comms-on-user-stevek/audioplayer_new-revert-from-user-stevek/audioplayer_new.lock': Filename too long

[dscho]

I took a good look at this, and it would appear that git pack-refs simply ignores the core.longPaths setting because it never bothers to parse that part of the config.

A quick workaround would be to call git_config(git_default_config, NULL); somewhere early in cmd_pack_refs().

However, I find that too fragile, as it only affects the pack-refs command, and there might be other commands that neglect to parse the Windows-specific core.* settings similarly. So I started reworking the entire way those settings are read. Hopefully I can get this mini-project finished by end of tomorrow.

[dscho]

Phew. This took much longer than I expected.

The real problem with my original idea "to fix this for good" was that there is a Catch-22: the first time the config is read, mingw_open() wants to know whether long paths are supported, and therefore would want to read the config. This recursive config reading would devolve into a stack overflow very, very quickly.

That is not the only problem, though: the first time we read a file is typically done before any -c <var>=<value> is parsed, which would then require re-reading the config once the command-line parameters are parsed, for which there is no good point to intercept (and detecting whether GIT_CONFIG_PARAMETERS has changed every time we want to know whether core.longPaths is set would be prohibitively expensive).

Therefore, I decided in the end that the best course of action is the pragmatic (if slightly unsatisfying) one to run the entire test suite after instrumenting the code to detect when mingw_core_config() was not called to parse the config before the process ends. And then fix all of the offending commands.

It is slightly unsatisfying because it leaves the door wide open for future breakages, as the instrumentation is expensive and cannot be used in production, and is too ugly to even merge into master. But it shows what needs to be fixed, for the moment.

This took a while longer than I intended, as I mentioned above, and I pushed the current version (including the instrumentation patch that will be removed before merging). I will try to run a couple more tests on this branch, just to make sure that there is nothing obvious wrong with it, maybe after the weekend, though.