Re: grep-2.5.1a egrep/fgrep PATH problem

[Tony Abou-Assaleh]

Some quick remarks from just looking at the patch:

> Index: src/grep.c
> ===================================================================
> RCS file: /cvsroot/grep/grep/src/grep.c,v
> retrieving revision 1.111
> diff -p -u -r1.111 grep.c
> --- src/grep.c        22 Jun 2005 01:47:43 -0000      1.111
> +++ src/grep.c        24 Jun 2005 00:02:28 -0000
> @@ -1458,9 +1458,11 @@ if any error occurs and -q was not given
>  static void
>  setmatcher (char const *m)
>  {
> -  if (matcher && strcmp (matcher, m) != 0)

Is it necessary to change the above test? I don't see the advantage of the
change.

> +  static int matcher_is_set;

Shouldn't we initialize matcher_is_set to 0? Some compilers will do, but I
think it is not a guaranteed behaviour.

> +  if (matcher_is_set && strcmp (matcher, m) != 0)
>      error (2, 0, _("conflicting matchers specified"));

It would be cool to support some thing like:

        grep -e "abc*" -F -e "blah" -E -e "^foo.*bar$"

But this is more of a feature request with a low priority.

>    matcher = m;
> +  matcher_is_set = 1;
>  }
>
>  /* Go through the matchers vector and look for the specified matcher.
> @@ -1702,38 +1704,6 @@ main (int argc, char **argv)
>
>    initialize_main (&argc, &argv);
>    program_name = argv[0];
> -  if (program_name && strrchr (program_name, '/'))
> -    program_name = strrchr (program_name, '/') + 1;
> -
> -  if (!strcmp(program_name, "egrep"))
> -    setmatcher ("egrep");
> -  if (!strcmp(program_name, "fgrep"))
> -    setmatcher ("fgrep");
> -
> -#if defined(__MSDOS__) || defined(_WIN32)
> -  /* DOS and MS-Windows use backslashes as directory separators, and usually
> -     have an .exe suffix.  They also have case-insensitive filesystems.  */
> -  if (program_name)
> -    {
> -      char *p = program_name;
> -      char *bslash = strrchr (argv[0], '\\');
> -
> -      if (bslash && bslash >= program_name) /* for mixed forward/backslash 
> case */
> -     program_name = bslash + 1;
> -      else if (program_name == argv[0]
> -            && argv[0][0] && argv[0][1] == ':') /* "c:progname" */
> -     program_name = argv[0] + 2;
> -
> -      /* Collapse the letter-case, so `strcmp' could be used hence.  */
> -      for ( ; *p; p++)
> -     if (*p >= 'A' && *p <= 'Z')
> -       *p += 'a' - 'A';
> -
> -      /* Remove the .exe extension, if any.  */
> -      if ((p = strrchr (program_name, '.')) && strcmp (p, ".exe") == 0)
> -     *p = '\0';
> -    }
> -#endif
>
>    keys = NULL;
>    keycc = 0;
> @@ -2081,9 +2051,6 @@ main (int argc, char **argv)
>        parse_grep_colors();
>      }
>
> -  if (! matcher)
> -    matcher = "grep";
> -
>    if (show_version)
>      {
>        printf ("%s\n\n", PACKAGE_STRING);

We should not remove the code above just to add it back in a few patches
down the road. If we are to support the wrapper approach, either now or in
the near future, Maybe we should just comment out (or def out?) the above
code segments.

> Index: src/grepmat.c
> ===================================================================
> RCS file: /cvsroot/grep/grep/src/grepmat.c,v
> retrieving revision 1.3
> diff -p -u -r1.3 grepmat.c
> --- src/grepmat.c     6 Oct 1999 01:13:13 -0000       1.3
> +++ src/grepmat.c     24 Jun 2005 00:02:28 -0000
> @@ -3,4 +3,4 @@
>  #endif
>  #include "system.h"
>  #include "grep.h"
> -char const *matcher;
> +char const *matcher = "grep";

I recommend creating a define for the various matchers. I would also have
them integers, rather than strings, especially if the snippets above about
program name checkings are removed.

> --- /dev/null 2003-03-18 13:55:57 -0800
> +++ src/egrepmat.c    2005-06-23 16:54:43 -0700
> @@ -0,0 +1,6 @@
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +#include "system.h"
> +#include "grep.h"
> +char const *matcher = "egrep";
> --- /dev/null 2003-03-18 13:55:57 -0800
> +++ src/fgrepmat.c    2005-06-23 16:54:43 -0700
> @@ -0,0 +1,6 @@
> +#ifdef HAVE_CONFIG_H
> +# include <config.h>
> +#endif
> +#include "system.h"
> +#include "grep.h"
> +char const *matcher = "fgrep";

It seems that the general philosophy is that 'grep -E|F' should be
encouraged, and the 'fgrep' and 'egrep' methods should become depricated.
If that's the case, then maybe we should give the option not to create
'e|fgrep', even if it is not the default at the moment, and document the
intention in the man pages.

Cheers,

TAA

--------------------------------------------------
Tony Abou-Assaleh
Ph.D. Candidate, Faculty of Computer Science
Dalhousie University, Halifax, NS, Canada, B3H 1W5
Fax:   902-492-1517
Email: address@hidden
WWW:   http://www.cs.dal.ca/~taa/
---------------------[THE END]--------------------