Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 26, 2025, 07:46 UTC
Message-ID: <721cfcca-b702-cb44-c847-f438b2495bda@gmx.de>
In-Reply-To: <Z-OWpEDwHtaq3r2J@pks.im>

Hi Patrick,

On Wed, 26 Mar 2025, Patrick Steinhardt wrote:

Show 17 quoted lines

> 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