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

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