Fix attribute lookup without common dir#4995
Conversation
The code in the `attr_setup` function is not really matching our current coding style. Besides alignment issues, it's also hard to see what functions calls depend on one another because they're split up over multiple conditional statements. Fix these issues by grouping together dependent function calls and adjusting the alignment.
If creating a repository without a common directory (e.g. by using `git_repository_new`), then `git_repository_item_path` will return `GIT_ENOTFOUND` for every file that's usually located in this directory. While we do not care for this case when looking up the "info/attributes" file, we fail to properly ignore these errors when setting up or collecting attributes files. Thus, the gitattributes lookup is broken and will only ever return `GIT_ENOTFOUND`. Fix this issue by properly ignoring `GIT_ENOTFOUND` returned by `git_repository_item_path`.
|
Just to state the (maybe not-so) obvious thing: the PR will currently fail due to the non-working tests. The fix itself is supposed to work just fine, though |
Can you explain a little bit more about this to me? You need an index and / or working directory for attributes to be honored in git itself. (git itself will not apply attributes in a bare repository.) Are you suggested that we should? Or are you suggesting that it should be possible to piece together a nonbare repository starting with Sorry if I'm being dense, it's possible that I just haven't dug deep enough to understand here. |
Well, git doesn't, that's true. But the thing is that git doesn't have any notion of in-memory repos after all. So I'd argue that it should be possible to look up attributes for certain files in such an in-memory repo, too, without requiring a working tree. But maybe I'm simply construing fanciful usecases that don't exist out there. But what really confuses me is why we have code for inmemory attributes that is seemingly only half finished. You can only barely create one if you find the correct magic incantation, they get wiped from the attributes cache, aren't being found when looking up attribute sources (index/worktree/in-memory) and loading such inmemory sources simply says "/* in-memory attribute file doesn't need data */", making it impossible to have them work in any meaningful way. So what are these things supposed to do in the end? Is it only to support our gitignore code? As that is the only callsite I found, and I can imagine that there's more elegant ways in that case. My initial motivation was to have "efficient" tests for attribute macros without having to rewrite a repo's .gitattributes file repeatedly. But yeah, that quickly led down a road of confusion and frustration while trying to figure out what this in-memory attributes stuff actually is. |
Right, I don't know either. And while there have been some requests for something like this from end-users, I'm very afraid to start trying to "correct" anything without a plan or use-case. When we talk about "inmemory" repos, I think that what we're actually talking about is "bare repositories with custom databases". And my gut reaction here is that they should, generally, behave like bare repos that are opened the normal way. 🤔 |
|
OTOH, I do want to be able to look up attributes out of a bare repository. (To answer the question: how would I filter this file if I were going to?) I don't even know if that's something that I can do today? Gah, you're right, this is confusing. |
It's not, right now we only support looking up gitattributes files from the index and the working directory. Hasn't there been a discussion on the git mailing list a few years ago to introduce lookup from the HEAD commit, too? Maybe I misremember, though, at least gitattributes(5) only mentions working directory and the index. So to support this, we could add a new flag But anyway. It's certainly out-of-scope for this PR, I'll just change it to repeatedly re-write the repo's .gitattributes file. |
So this is the fix I came up while I didn't yet see that there's #4967 already. The fixes are mostly the same, even though I did some refactoring up front to make the code a bit more readable (at least in my opinion).
What's been itching me is to write tests for this. While it's quite easy to write one with
git_attr_get, I noticed that we didn't have any tests for our attribute macros at all. As that code path implicitly also verified that we're able to look up attributes without commondir, I thought I'd just write one like that.But the crux is: with
git_repository_new, I don't see any way of adding attributes files to agit_repository. While we supposedly have in-memory support for gitattributes files, we have no way of adding them to a repo. The same holds for adding real files: there is no way of adding a gitattributes file that lives at a non-standard path to a repo. So maybe I'm dumb and not seeing the obvious way to do so, but I was not able to make gitattributes work with in-memory repos.So this PR is rather a plea for help: is there anything that makes this work already? Otherwise I think we should definitely fix this API shortcoming and introduce proper in-memory gitattributes support.