Re: [patch 1/2] auto-load safe-path: Permit shell wildcards

[Eli Zaretskii <eliz at gnu dot org>]

> Date: Wed, 20 Jun 2012 20:40:32 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: gdb-patches@sourceware.org
> 
> > > -filename_is_in_dir (const char *filename, const char *dir)
> > > +filename_is_in_pattern (const char *filename_orig, const char *pattern_orig)
> > 
> > The arguments are named differently from what the commentary says.
> > (Do you really need the "_orig" suffix here?)
> 
> I have split it now into filename_is_in_pattern and filename_is_in_pattern_1.
> 
> As an explanation of the previous state:
> 
> The problem is that I want to use bare "filename" in the code.  Using
> "filename_local" (for example) may lead to mistakes as it is easier to
> accidentally write "filename" than to accidentally write "filename_orig".
> 
> Using just "filename" everywhere would mean to make the parameter non-const
> ('char *filename') which may lead callers into thinking the string contents
> may be modified by this function.  The code both uses and modifies the content
> of "filename".

So you wanted to use 'const', which dominoed into a whole bunch of
variable renaming and helper function.  Looks like a net loss to me.
But I won't argue about style, since I'm usually in the minority on
that.

> > > +  /* Trim trailing slashes ("/") from PATTERN.  */
> > > +  while (pattern_len && IS_DIR_SEPARATOR (pattern[pattern_len - 1]))
> > > +    pattern_len--;
> > > +  pattern[pattern_len] = '\0';
> > 
> > Wouldn't this will do the wrong thing with a pattern such as "d:/"?
> 
> It will trim it to "d:" and then it will try to remove /+[^/]+ each time from
> the filename reducing it also down to "d:".  Therefore "d:/" will match any
> file on drive d:.

I'd say that warrants a comment, since the code looks plain wrong.

> > >  @value{GDBN} provides the @samp{set auto-load safe-path} setting to list
> > >  directories trusted for loading files not explicitly requested by user.
> > > +Each directory can be also shell wildcard pattern.
> >                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > "... can also be a shell wildcard."
> 
> I believe there should still be "shell wildcard pattern":
> 
> set auto-load safe-path patternX:patternY
> set auto-load safe-path /directoryX1/directoryX2:/directoryY1/directoryY2
> 
> I can say:
> set auto-load safe-path /usr/src*/.gdbinit
> 
> So even a part of the directory ("src*") can be shell wildcard ("*").
> "src*" is IMO 'shell wildcard pattern' and not 'shell wildcard'.
> "*" is 'shell wildcard'.

I believe you are talking about "wildcard character" as opposed to a
"wildcard".

> Do you still insist on your wording?

I don't insist, but I still think "pattern" is redundant.

> > > +                                                  '*' matches only single
> > > +component, it does not match across directory separator.
> > 
> > "... @samp{*} matches a single component ..."
> > 
> > Should we describe all of the wildcard meta-characters, not just '*'?
> 
> The goal was to express FNM_FILE_NAME is in use.

I understand, but I'm not sure the readers will.  It sounds strange to
mention only one wildcard character, unless you already know that
FNM_FILE_NAME is the issue, and you also know what it does.