Patrick Steinhardt <ps@xxxxxx> writes:
>> static const char *unquote_quoted_string(struct strbuf *outbuf, const char *in)
>> {
>> - int c;
>> int take_next_literally = 0;
>>
>> - while ((c = *in++) != 0) {
>> + while (*in) {
>> + int c = *in++;
>> if (take_next_literally == 1) {
>> take_next_literally = 0;
>> } else {
>
> I was wondering whether we want to convert `unquote_quoted_pair()` in
> the same way. It's not subject to the same issue because it doesn't
> return `in` to its callers. But it would lower the cognitive overhead a
> bit by keeping the coding style consistent. But please feel free to
> ignore this remark.
Yeah, I was wondering about the value of establishing a pattern that
can be followed safely and with clarity, too. I also briefly
wondered how bad if we picked the "compensate for the increment done
by the last iteration before leaving the loop by returning (in-1)"
idiom (which Peff called "a hacky way") to be that universal
pattern, but this bug was a clear enough evidence that it does not
work very well in developers' minds.
I actually had trouble with the proposed update, and wondered if
- while ((c = *in++) != 0) {
+ while ((c = *in)) {
+ in++;
is easier to follow, but then was hit by the possibility that the
same "we have incremented 'in' a bit too early" may exist if such
a loop wants to use 'in' in its body. Wouldn't it mean that
- while ((c = *in++) != 0) {
+ for (; c = *in; in++) {
would be even a better rewrite?
Enough bikeshedding...
> Another thing I was wondering about is the recursive nature of these
> functions, and whether it can lead to similar issues like we recently
> had when parsing revisions with stack exhaustion. I _think_ this should
> not be much of a problem in this case though, as we're talking about
> mail headers here. While the length of header values isn't restricted
> per se (the line length is restricted to 1000, but with Comment Folding
> Whitespace values can span multiple lines), but mail provdiers will sure
> clamp down on mails with a "From:" header that is megabytes in size.
Good thinking, and I think Peff's iterative rewrite would be a good
way to deal with it if it ever becomes an issue.
> So taken together, this looks good to me.
Thanks, both, for writing and reviewing.
