Re: [bug #66365] find -exec treats + as special when it shouldn't

I have installed a modified version of the patch
(1dcdf3de8e27cc130968891ee5a529a461a248da), updated in the ways you
suggested.

On Sat, Nov 2, 2024 at 9:24 AM Bernhard Voelker
<[email protected]> wrote:
>
> Hi James,
>
> thanks, technically this patch fixes it, yet I think it deserves some more love before pushing.
>
> First of all, this patch does not cleanly apply.
>
>    $ git am /tmp/x.patch
>    Applying: Fix Savannah bug 66365.
>    error: git diff header lacks filename information when removing 1 leading pathname component (line 82)
>    Patch failed at 0001 Fix Savannah bug 66365.
>    ...
>
> Didn't you use 'git format-patch -1' and 'git send-email ...'?
>
> On 10/31/24 17:28, James Youngman wrote:
> >  From e7641882e3ef373b6c58bcf27fbc49269e2541f3 Mon Sep 17 00:00:00 2001
> > From: James Youngman <[email protected]>
> > Date: Thu, 31 Oct 2024 15:06:02 +0000
> > Subject: [PATCH] [find] Fix Savannah bug 66365.
>
> the bug should be mentioned in the commit message, but the Subject line
> should better tell about the change, e.g.
>
>    find: only treat '+' special after pure '{}' in -exec option family
>
> > A "+" only terminates -exec when it immediately follows an argument
> > which is exactly "{}".
>
> Let's also explain the change and reasoning for it in a NEWS entry.
>
> > ---
> >   find/parser.c                                 | 21 ++++++++++++-------
> [...]
> >   create mode 100644 find/testsuite/find.posix/sv-bug-66365-exec.exp
> >   create mode 100644 find/testsuite/find.posix/sv-bug-66365-exec.xo
>
> I'd prefer going with the newer style for the tests like
> 'tests/find/exec-plus-last-file.sh'.
>
> > diff --git a/find/parser.c b/find/parser.c
> > index ad3b9904..d7b07c40 100644
> > --- a/find/parser.c
> > +++ b/find/parser.c
> > @@ -2770,7 +2770,7 @@ insert_exec_ok (const char *action,
> >   {
> >     int start, end;               /* Indexes in ARGV of start & end of cmd. */
> >     int i;                        /* Index into cmd args */
> > -  int saw_braces;               /* True if previous arg was '{}'. */
> > +  bool plus_is_terminator;      /* True if previous arg was '{}'. */
>
> I like the change to bool, but I'm not sure whether the new name is clearer here.
> Actually it should say "the previous argument were pure braces only".
> Maybe 'saw_pure_braces'?
>
> >     bool allow_plus;              /* True if + is a valid terminator */
> >     int brace_count;              /* Number of instances of {}. */
> >     const char *brace_arg;        /* Which arg did {} appear in? */
> > @@ -2827,24 +2827,31 @@ insert_exec_ok (const char *action,
> >      * Also figure out if the command is terminated by ";" or by "+".
> >      */
> >     start = *arg_ptr;
> > -  for (end = start, saw_braces=0, brace_count=0, brace_arg=NULL;
> > +  for (end = start, plus_is_terminator=false, brace_count=0, brace_arg=NULL;
> >          (argv[end] != NULL)
> >          && ((argv[end][0] != ';') || (argv[end][1] != '\0'));
> >          end++)
> >       {
> >         /* For -exec and -execdir, "{} +" can terminate the command. */
> > -      if ( allow_plus
> > -           && argv[end][0] == '+' && argv[end][1] == 0
> > -           && saw_braces)
> > +      if (allow_plus && plus_is_terminator
> > +           && argv[end][0] == '+' && argv[end][1] == 0)
> >           {
> >             our_pred->args.exec_vec.multiple = 1;
> >             break;
> >           }
> >
> > -      saw_braces = 0;
> > +      plus_is_terminator = false;
> >         if (mbsstr (argv[end], "{}"))
> >           {
> > -          saw_braces = 1;
> > +   if (0 == strcmp(argv[end], "{}"))
> > +     {
> > +       /* Savannah bug 66365: + only terminates the predicate
> > +        * immediately after an argument which is exactly, "{}".
> > +        * However, the "{}" in "x{}" should get expanded for
> > +        * the ";" case.
> > +        */
> > +       plus_is_terminator = true;
> > +     }
>
> The indentation is odd.
>
> >             brace_arg = argv[end];
> >             ++brace_count;
> >
>
> BTW: the guard to prevent '-exec {} ;' seems to be ineffective for a while as well.
> I'll check.
>
> Have a nice day,
> Berny