Re: [PATCH 2/4] config: really keep value-internal whitespace verbatim
- From
- Dragan Simic <dsimic@manjaro.org>
- Date
- Mar 15, 2024, 19:50 UTC
- Message-ID
- <1b6cfdd62b08b7bdc817e3c106e3ef50@manjaro.org>
- In-Reply-To
- <xmqq4jd7qtg6.fsf@gitster.g>
On 2024-03-15 18:46, Junio C Hamano wrote:
Show 14 quoted lines
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Fixes: ebdaae372b46 ("config: Keep inner whitespace verbatim")
>
> No need for this line. You mentioned it in the text already, and
> more importantly, grepping for trailers is not the right thing to do
> because we may think we fixed all bugs in ebdaae372b46 did with this
> patch, which may in 6 months turn out to be false but we cannot undo
> the trailer. On the other hand, the discussion of the problem in
> the proposed log message gives readers a richer context and the
> future developers can understand that this fixed one thing in
> ebdaae372b46 but didn't even know about other bugs introduced by
> that old commit and make more intelligent decision based on that
> better understanding.Got it, and I agree that the discussions linked in the patch description provide much better ways for understanding what brought us to the current situation, and what led to the proposed bugfixes.
It's simply that I'm kind of used to providing "Fixes" tags for patches in other projects; I'll make a mental note not to do that for git.
I'll also send the v2 with no "Fixes" tag, after the v1 spends a few days on the mailing list.
Show 35 quoted lines
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> config.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/config.c b/config.c
>> index a86a20cdf5cb..5072f12e62e4 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -817,33 +817,38 @@ static int get_next_char(struct config_source
>> *cs)
>>
>> static char *parse_value(struct config_source *cs)
>> {
>> - int quote = 0, comment = 0, space = 0;
>> + int quote = 0, comment = 0;
>> + size_t trim_len = 0;
>
>
>> strbuf_reset(&cs->value);
>> for (;;) {
>> int c = get_next_char(cs);
>> if (c == '\n') {
>> if (quote) {
>> cs->linenr--;
>> return NULL;
>> }
>> + if (trim_len)
>> + strbuf_setlen(&cs->value, trim_len);
>> return cs->value.buf;
>
> So the idea is that we copy everything we read verbatim in cs->value
> but keep track of the beginning of the run of whitespace characters
> at the end of the line in trim_len so that we can rtrim it? That
> should be the most straight-forward implementation.Yes, that's it. It's as simple as it can be. A different, somewhat more complex approach of putting the encountered whitespace in a separate buffer may be warranted if we expected _a lot_ of whitespace, i.e. much more whitespace than the other characters, but that shouldn't be the case, which makes the simple approach a good choice.
Show 12 quoted lines
>> }
>> if (comment)
>> continue;
>> if (isspace(c) && !quote) {
>> + if (!trim_len)
>> + trim_len = cs->value.len;
>> if (cs->value.len)
>> + strbuf_addch(&cs->value, c);
>> continue;
>
> While we are not inside a dq-pair, we keep ignoring a whitespace
> character at the beginning (i.e. cs->value.len == 0).Yes, that "eats up" any leading whitespace.
Show 7 quoted lines
> If we have some value in cs->value, however, we add the whitespace > character to cs->value verbatim. trim_len==0 signals that the last > character we processed was not a whitespace character, and we copy > the current length of cs->value there---this is so that we can rtrim > away the run of whitespaces at the end of the input, including the > byte we are adding here, if it turns out that we are looking at the > first whitespace character of such trailing blanks.
Exactly. By the way, please know that I _really_ appreciate your highly detailed patch reviews!
Show 15 quoted lines
>> }
>> if (!quote) {
>> if (c == ';' || c == '#') {
>> comment = 1;
>> continue;
>> }
>> }
>
>> + if (trim_len)
>> + trim_len = 0;
>
> If we are outside a dq-pair, we reach here only when we are looking
> at a non-whitespace character. If we are counting a run of unquoted
> whitespaces, we can reset trim_len here to record that we do not
> have to trim.Exactly. It resets the "need to rtrim flag", so to speak.
Show 6 quoted lines
> But can we be seeing a whitespace that is inside quote here? Is > resetting trim_len to zero what we want in such a case? Let's see. > Inside dq, we'd want to accumulate bytes, possibly after unwrapping > their backslash escapes, to cs->value, and these bytes include the > whitespace characters---we want to keep them literally without > trimming. OK.
Yes, any whitespace inside a quoted option value is to be taken verbatim, so we basically reset the "need to rtrim flag" in that case, because value-internal whitespace isn't to be rtrimmed.
> Looking good. We should already demonstrate that this works well > with a new test in the same patch, can't we? If we can, we should.
Sorry, this sounds a bit confusing to me? I think there are already more than enough new tests in the 3/4 patch I sent?
Could you, please, clarify a bit?
>> if (c == '\\') {
>> c = get_next_char(cs);
>> switch (c) {