-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configuration variables can appear on the same line as the section header #4819
Conversation
src/config_parse.c
Outdated
@@ -135,7 +135,7 @@ static int parse_section_header_ext(git_config_parser *reader, const char *line, | |||
} | |||
|
|||
*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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This is an quirk of the config format, but you are allowed to do
and have
some.var
exist. 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.