Re: [PATCH] wildmatch: change behavior of "foo**bar" in WM_PATHNAME mode

From: Duy Nguyen <pclouds@gmail.com>
Date: Oct 28, 2018, 06:35 UTC
Message-ID: <CACsJy8BoWmWaVpXnSL8Stm_2XDnKfXSaj2i0UwZg7aEgYF9i4w@mail.gmail.com>
In-Reply-To: <20181028062533.GA15061@tor.lan>

On Sun, Oct 28, 2018 at 7:25 AM Torsten Bögershausen <tboegi@web.de> wrote:

Show 24 quoted lines

>
> On Sat, Oct 27, 2018 at 10:48:23AM +0200, Nguyễn Thái Ngọc Duy wrote:
> > In WM_PATHNAME mode (or FNM_PATHNAME), '*' does not match '/' and '**'
> > can but only in three patterns:
> >
> > - '**/' matches zero or more leading directories
> > - '/**/' matches zero or more directories in between
> > - '/**' matches zero or more trailing directories/files
> >
> > When '**' is present but not in one of these patterns, the current
> > behavior is consider the pattern invalid and stop matching. In other
> > words, 'foo**bar' never matches anything, whatever you throw at it.
> >
> > This behavior is arguably a bit confusing partly because we can't
> > really tell the user their pattern is invalid so that they can fix
> > it. So instead, tolerate it and make '**' act like two regular '*'s
> > (which is essentially the same as a single asterisk). This behavior
> > seems more predictable.
>
> Nice analyzes.
> I have one question here:
> If the user specifies '**' and nothing is found,
> would it be better to die() with a useful message
> instead of silently correcting it ?

Consider the main use case of wildmatch, .gitignore patterns, dying would be really bad because it can affect a lot of commands. It would be much better if wildmatch api allows the caller to handle the error so they can either die() or propagate the error upwards. But even then the current API is not suited for that, ideally we should have a compile phase where you can validate the pattern first. Without it, you encounter the error every time you try to match something and handling errors becomes much uglier.

And it goes against the way pattern errors are handled by fnmatch/wildmatch anyway. If you write '[ab' instead of '[ab]', '[' will just be considered literal '[' instead of erroring out.

Show 14 quoted lines

> See the the patch below:
> > -                             } else
> > -                                     return WM_ABORT_MALFORMED;
>
> Would it be possible to put in the die() here?
> As it is outlined so nicely above, a '**' must have either a '/'
> before, or behind, or both, to make sense.
> When there is no '/' then the user specified something wrong.
> Either a '/' has been forgotten, or the '*' key may be bouncing.
> I don't think that Git should assume anything here.
> (but I didn't follow the previous discussions, so I may have missed
> some arguments.)
>
> []

-- 
Duy