worktreeConfig capitalization problem

[wshanks]

dulwich only tests for extensions.worktreeconfig but in the git documentation it is written as extensions.worktreeConfig. Recently, we started seeing dulwich error in GitHub Actions with dulwich.repo.UnsupportedExtension: b'worktreeConfig'. I tested locally and confirmed that extensions.worktreeconfig does not lead to an error from dulwich but extensions.worktreeConfig does.

I am not sure what is going on with GitHub Actions. I see that someone was affected by this in #1262 but then closed it as "fixed by git". Also, some other users have been having the opposite problem with GitHub Actions it seems: GitTools/actions#1115. It seems like this setting is not consistent across GitHub Actions runs recently.

[0x2b3bfa0]

The variable names are case-insensitive, allow only alphanumeric characters and -, and must start with an alphabetic character.

[wshanks]

After some more testing, I found that git itself is insensitive to the capitalization of config entries. So I think the issue is that dulwich is treating the config entry as it is in the file rather without lower casing it first and that #1262 would be the right solution.

[jelmer]

Could you create a new PR, ideally with a unit test that reproduces the issue?

[shcheklein]

I think the easiest, less intrusive fix is to change this:

        for extension, _value in config.items((b"extensions",)):
            if extension not in (b"worktreeconfig",):
                raise UnsupportedExtension(extension)

to something like:

        for extension, _value in config.items((b"extensions",)):
            if extension.lower() not in (b"worktreeconfig",):
                raise UnsupportedExtension(extension)

dulwich is actually using CaseInsensitiveOrderedMultiDict internally, but items is iterating on an actual (real) keys.

It would be great to see if items() can be changed to return a nomalized list, but most likely there was a good reason for this.

[0x2b3bfa0]

I think the easiest, less intrusive fix is to change this [...] to something like [...]

• See fix: 🐛 📝 worktreeconfig extension typo #1262 linked above.

[shcheklein]

@jelmer would a simple fix like in #1285 (comment) work? can we merge it (add test and merge)?

[jelmer]

Yeah, just calling .lower() seems fine

[wshanks]

Here is my attempt: #1286

[jelmer]

It would be great to see if items() can be changed to return a nomalized list, but most likely there was a good reason for this.

ConfigFile doesn't know about the fields and their supported values. We can't just lowercase all values returned from ConfigFile since that would also impact fields where case matters such as user.name

[0x2b3bfa0]

We can't just lowercase all values returned from ConfigFile since that would also impact fields where case matters such as user.name

It's not about converting values to lowercase, but keys, which as per the Git documentation should be case-insensitive; see #1285 (comment).

[0x2b3bfa0]

• Fix attempt at Lowercase CaseInsensitiveOrderedMultiDict.items() keys #1288

[jelmer]

We can't just lowercase all values returned from ConfigFile since that would also impact fields where case matters such as user.name

It's not about converting values to lowercase, but keys, which as per the Git documentation should be case-insensitive; see #1285 (comment).

Case insensitive is not the same as has no case, it just means comparisons should ignore case.

[0x2b3bfa0]

@jelmer, some of the methods of CaseInsensitiveOrderedMultiDict already normalize keys to lowercase; see #1288 (comment) for more information.

Please take a look to these two alternative solutions, depending of whether you'd prefer to normalize keys to lowercase everywhere or just allow case-insensitive comparison:

• Lowercase CaseInsensitiveOrderedMultiDict.items() keys #1288
• Make CaseInsensitiveOrderedMultiDict.items() keys case insensitive #1289

If you deem #1286 enough, feel free to close both of my pull requests. Still, please keep in mind that this issue might arise again in the future with other configuration keys if we don't write an universal solution.

[jelmer]

@jelmer, some of the methods of CaseInsensitiveOrderedMultiDict already normalize keys to lowercase; see #1288 (comment) for more information.

Please take a look to these two alternative solutions, depending of whether you'd prefer to normalize keys to lowercase everywhere or just allow case-insensitive comparison:

• Lowercase CaseInsensitiveOrderedMultiDict.items() keys #1288
• Make CaseInsensitiveOrderedMultiDict.items() keys case insensitive #1289

If you deem #1286 enough, feel free to close both of my pull requests. Still, please keep in mind that this issue might arise again in the future with other configuration keys if we don't write an universal solution.

So I think you're right that consistently returning lowercase section and variable names is probably the best approach here long-term. I've merged the lowercasing (#1286) since it addresses the immediate issue.

CaseInsensitiveOrderedMultiDict tries to specifically preserve case today though - it keeps two copies, which it wouldn't need to do if all it cared about was lowercase keys.

[wshanks]

Besides consistency between items(), __iter__(), and keys(), I think there is the potential for a bug with how items() works now:

dulwich/dulwich/config.py

Lines 91 to 102 in ed1a145

	def items(self):
	return iter(self._real)
	def __iter__(self):
	return self._keyed.__iter__()
	def values(self):
	return self._keyed.values()
	def __setitem__(self, key, value) -> None:
	self._real.append((key, value))
	self._keyed[lower_key(key)] = value

CaseInsensitiveOrderedMultiDict.items() iterates on self._real and CaseInsensitiveOrderedMultiDict.__setitem__() appends to self._real. So if an item gets set twice, it will remain twice in self._real and be yielded twice by items() while only appearing once in keys() and __iter__(). Since configuration does not change usually, maybe this does not come up, but maybe it does with loading the repo config on top of the global config? (And also maybe worktreeConfig which I hadn't known about before running into this error 🙂 ).