Configuration variables can appear on the same line as the section header#4819
Conversation
|
|
||
| *section_name = git_buf_detach(&buf); | ||
| return 0; | ||
| return base_name_len + 1 /* SP */ + 1 /* " */ + rpos + 1 /* " */ + 1 /* ] */; |
There was a problem hiding this comment.
Can we really assume to have "static" length here? What about e.g. [foo "bar"] or [foo "bar" ]? Isn't that accepted by the parser? I don't think it currently is, but that leaves me wondering if we're stricter than git.git itself. A quick check shows that [foo "bar"] is accepted, while [foo "bar" ] is not. I don't think we accept the former right now.
If we were to update our logic, would the above calculation still be correct? If possible, I'd instead prefer to do some pointer arithmetic between the start of the line and our end position.
There was a problem hiding this comment.
It is invalid to have a space between the closing quotes and the closing bracket. Both git and libgit2 refuse to parse such a file, so we should be fine.
I do agree that a more direct calculation would be nicer to have. The different sub-strings we use here made it a bit awkward so I did this to get the number. But again I do think the math will end up working out.
| git_config *cfg; | ||
|
|
||
| cl_set_cleanup(&clean_test_config, NULL); | ||
| cl_git_mkfile("./testconfig", "[some] var = value\n[some \"OtheR\"] var = value"); |
There was a problem hiding this comment.
You might want to have one of these drop the space between the header and the value (e.g., [some]var = value) for variety. From my reading of the code, that should be handled just fine, but it's yet another funny corner case.
While rare and a machine would typically not generate such a configuration file,
it is nevertheless valid to write
[foo "bar"] baz = true
and we need to deal with that instead of assuming everything is on its own line.
carlosmn
force-pushed
the
cmn/config-nonewline
branch
from
e28d03f to
1d718fa
Compare
|
Rebased and fixed up to add a no-whitespace test as well as a hopefully more straightforward way of figuring out the section header length. |
| cl_assert_equal_s(buf.ptr, "value"); | ||
|
|
||
| git_config_free(cfg); | ||
| cl_git_mkfile("./testconfig", "[some] var = value\n[some \"OtheR\"]var = value"); |
There was a problem hiding this comment.
Remark: now that we have the memory config backend, we could just use that one to do the tests instead of writing files. This PR predates that, though
3 participants
This is an quirk of the config format, but you are allowed to do
and have
some.varexist. Our parser currently assumes everything is line-based and completely ignores anything past the closing bracket.So this changes the section parser functions to return how far we should advance and then we restart. For the "normal" case, we'll detect the rest is an empty line and immediately jump to the next line and if we do find this situation we'll run the loop as though this were a brand new line.