Hi Patrick,
On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> On Tue, Mar 25, 2025 at 11:32:11PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/wildmatch.c b/wildmatch.c
> > index 8ea29141bd7..ce8108a6d57 100644
> > --- a/wildmatch.c
> > +++ b/wildmatch.c
> > @@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
> > p_ch = 0; /* This makes "prev_ch" get set to 0. */
> > } else if (t_ch == p_ch)
> > matched = 1;
> > - } while (prev_ch = p_ch, (p_ch = *++p) != ']');
> > + } while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');
> > if (matched == negated ||
> > ((flags & WM_PATHNAME) && t_ch == '/'))
> > return WM_NOMATCH;
>
> In this case I agree that it makes sense to not introduce curly braces
> for brevity.
I should probably have mentioned that this patch took the longest to write
of the entire patch series, by far. Not because of the changed code, that
was easy. No, when I wrote the commit message and spotted the `continue`,
I did not want to leave it at that because the code around that `continue`
looks... well, let's just say that it could be rewritten for clarity.
In fact, when I looked at the following part, I was immediately
_convinced_ that it is incorrect, and had to work very hard to understand
why it works correctly, even going so far as to single-step through a
couple of examples, e.g. `test-tool wildmatch wildmatch 'b' '[[:a-z]'`:
} else if (p_ch == '[' && p[1] == ':') {
const uchar *s;
int i;
for (s = p += 2; (p_ch = *p) && p_ch != ']'; p++) {} /*SHARED ITERATOR*/
if (!p_ch)
return WM_ABORT_ALL;
i = p - s - 1;
if (i < 0 || p[-1] != ':') {
/* Didn't find ":]", so treat like a normal set. */
p = s - 2;
p_ch = '[';
if (t_ch == p_ch)
matched = 1;
continue;
}
For context, here is a link:
https://gitlab.com/gitlab-org/git/-/blob/v2.49.0/wildmatch.c?ref_type=tags#L213-227.
At this point, `t_ch` is the current character in the text to match;
`p_ch` (and `*p`) is the current character in the _pattern_, and it is
_inside_ a `[...]` character range, and it wants to match a character
class of the form `[:alnum:]` but also allow for a regular character range
that starts by including the colon. And that latter scenario, where it is
_not_ a special character class, is what this `continue` is all about.
What threw me was that `t_ch == p_ch` condition _directly_ after assigning
`p_ch = '['`. It is still a pattern I would always immediately suspect to
be a bug: Why not compare `t_ch == '['` instead, which would be much more
obvious?
You will also note that the value of `i` is only used in the condition,
and it is basically used to determine whether the the colon was
immediately followed by the closing bracket or not, which could be
rewritten to be a lot more obvious.
So what does the `continue` do here? It skips back to the outer loop,
continuing with the `:` as next pattern character in the character range,
and for that it is crucial that the `p_ch` be set to the opening bracket
and `p` is rewound _just_ so that the assignments in the loop condition
can set things up for the next loop iteration (still within that `case
'['`) not to be thrown by that `[:`.
I probably did a terrible job explaining why the code works as intended,
but I'll just chalk it up to the contortions my brain had to exercise to
understand that code.
But all that's safely outside the scope of the question whether to use a
comma operator or not ;-)
Ciao,
Johannes
