emacs-compat / compat

COMPATibility Library for Emacs Lisp
https://elpa.gnu.org/packages/compat.html
GNU General Public License v3.0
69 stars 12 forks source link

Please don't create branches with characters that are invalid for Windows filenames #44

Closed Kartiku closed 5 months ago

Kartiku commented 5 months ago

Please have pity on us windows users. I don't know how this is even a bug in 2024

$ cd "c:/Users/usename/AppData/Roaming/.emacs.d/straight/repos/compat/"
$ "git" "fetch" "origin"

error: cannot lock ref 'refs/remotes/origin/feature/value<': Unable to create 'C:/Users/username/AppData/Roaming/.emacs.d/straight/repos/compat/.git/refs/remotes/origin/feature/value<.lock': Invalid argument
From https://github.com/emacs-compat/compat
 ! [new branch]      feature/value< -> origin/feature/value<  (unable to update local ref)

[Return code: 1]
git version 2.45.2.windows.1
Kartiku commented 5 months ago

fix is to add

[remote "origin"]
    url = https://github.com/emacs-compat/compat.git
    fetch = +refs/heads/*:refs/remotes/origin/*
    fetch = ^refs/heads/feature/value<

to .git/config

phikal commented 5 months ago

Kartik Saranathan @.***> writes:

Please have pity on us windows users. I don't know how this is even a bug in 2024

$ cd "c:/Users/usename/AppData/Roaming/.emacs.d/straight/repos/compat/"
$ "git" "fetch" "origin"

error: cannot lock ref 'refs/remotes/origin/feature/value<': Unable to
create
'C:/Users/username/AppData/Roaming/.emacs.d/straight/repos/compat/.git/refs/remotes/origin/feature/value<.lock':
Invalid argument
From https://github.com/emacs-compat/compat
 ! [new branch]      feature/value< -> origin/feature/value<  (unable to update local ref)

[Return code: 1]
git version 2.45.2.windows.1

Using slashes in branch names is an established convention, if anything this sounds like a bug with the implementation of Git on Windows.

To assess the urgency, did you manually check out the feature/value< branch, or did the issue arise just because of the branches existence?

-- Philip Kaludercic on peregrine

Kartiku commented 5 months ago

The issue is with the < not the slash. value<.lock is an invalid filename on windows

phikal commented 5 months ago

I see, I wasn't familiar with that fact. But again, did you try to check-out the feature/value< branch, or what did you do to trigger this issue?

msolglu commented 5 months ago

I got this when running doom sync -u which uses straight under the hood to git pull.

It's surprisingly hard to reproduce, but I think this will happen to anything running git pull on Windows as long as there has been any update on the value< branch since the initial clone (or maybe any update after the first time git sees the branch?), regardless of whether the branch has been checked out locally.

The underlying error is that value<.lock cannot be created as a file on Windows, however git won't attempt to create that file on the initial clone from GitHub because it stores that branch in .git/packed-refs instead. This file is a cache of the remote refs, and when a branch is updated remotely it is moved out of packed-refs and into .git/refs^1. At that point it will probably error during a pull because it will try to create value<.lock with the updated reference.

So what I did to test

  1. git clone https://github.com/emacs-compat/compat
  2. cd compat
  3. run git pull to confirm everything is working

    git pull Already up to date.

  4. open .git/packed-refs in a text editor and change

a7fb705b93e2d289e0e10656942fcfc02ff7cd15 refs/remotes/origin/feature/value<

to

43513809f12b4bf567fffc6ce50f9106f77ce430 refs/remotes/origin/feature/value<

this in theory sets it back to the previous commit.

  1. run git pull again

    git pull error: cannot lock ref 'refs/remotes/origin/feature/value<': Unable to create 'C:/Users/user/compat/.git/refs/remotes/origin/feature/value<.lock': Invalid argument From https://github.com/emacs-compat/compat ! 4351380..a7fb705 feature/value< -> origin/feature/value< (unable to update local ref)


In case you were wondering, these are invalid characters in Windows:

< (less than)
> (greater than)
: (colon)
" (double quote)
/ (forward slash)
\ (backslash)
| (vertical bar or pipe)
? (question mark)
* (asterisk)

via https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file

minad commented 5 months ago

@msolglu Thanks for the explanations. For now I've deleted the branch, given that the value< function has already been added to the emacs-30 branch. We can be more careful in the future and avoid special characters in branch names.

phikal commented 5 months ago

You should have renamed the branch, as it still had more exhaustive tests than what is currently on master. I hope I still have the code lying around, and if I do I'll push the branch back into the repository with some other name.

Also, this is fundamentally a Git issue, so it should be reported to the Git maintainers. It is not reasonable to expect everyone to conform with Windows conventiosn, especially if third-party package managers download repositories and all branches by default (which is something that one could mention to the straight maintainers as well). If one of the two windows users could do that, that would be great.

On June 14, 2024 5:19:42 PM GMT+02:00, Daniel Mendler @.***> wrote:

@msolglu Thanks for the explanations. For now I've deleted the branch, given that the value< function has already been added to the emacs-30 branch. We can be more careful in the future and avoid special characters in branch names.

minad commented 5 months ago

I don't agree that we should keep this branch around. Note that some of the other feature/* branches we have here are dysfunctional and/or outdated, or are failed experiments. Usually feature branches can be deleted after merge of the feature. Anyway I still have the code locally and we should definitely add more of the tests to compat-tests.el directly.