Re: [coreutils] [PATCH] md5sum: Add option to ignore non-existant files

[Bernhard Voelker]

On 11/22/2015 11:06 PM, Pádraig Brady wrote:
> From f664e7e87b979503f263334caa02cc0525b77c43 Mon Sep 17 00:00:00 2001
> From: Luther Thompson <address@hidden>
> Date: Sun, 22 Nov 2015 21:47:59 +0000
> Subject: [PATCH] md5sum, sha*sum: add --ignore-missing for checking a subset
>  of files

This requires 'git commit --no-verify'.  I thought we already have an allowed
word for the "md5sum, sha*sum" family, but obviously we don't.
If we add such a keyword one day - which is not easy as "*sum" would also
match the 'cksum' and the 'sum' utilities, too - it'd be good to ensure that
each of the afftected utils is mentioned explicitly in the lines following
(or in the NEWS as you did here).

> * doc/coreutils.texi (md5sum invocation): Document the new option.
> * src/md5sum.c (digest_file): Return an empty digest to indicate
> a missing file.
> (digest_check): don't fail or output status given an empty checksum.

s/don't/Don't/

> (usage): Document the new option.
> (main): Process and validate the new option.
> * tests/misc/md5sum.pl: Add new test cases.
> * NEWS: Mention the new feature.

> diff --git a/src/md5sum.c b/src/md5sum.c
> index 5d4b958..8990f1a 100644
> --- a/src/md5sum.c
> +++ b/src/md5sum.c
> @@ -119,6 +119,9 @@ static bool status_only = false;
>     improperly formatted checksum line.  */
>  static bool warn = false;
>  
> +/* With --check, ignore missing files.  */
> +static bool ignore_missing = false;
> +
>  /* With --check, suppress the "OK" printed for each verified file.  */
>  static bool quiet = false;
>  
> @@ -133,7 +136,8 @@ static int bsd_reversed = -1;
>     non-character as a pseudo short option, starting with CHAR_MAX + 1.  */
>  enum
>  {
> -  STATUS_OPTION = CHAR_MAX + 1,
> +  IGNORE_MISSING_OPTION = CHAR_MAX + 1,
> +  STATUS_OPTION,
>    QUIET_OPTION,
>    STRICT_OPTION,
>    TAG_OPTION
> @@ -143,6 +147,7 @@ static struct option const long_options[] =
>  {
>    { "binary", no_argument, NULL, 'b' },
>    { "check", no_argument, NULL, 'c' },
> +  { "ignore-missing", no_argument, NULL, IGNORE_MISSING_OPTION},
>    { "quiet", no_argument, NULL, QUIET_OPTION },
>    { "status", no_argument, NULL, STATUS_OPTION },
>    { "text", no_argument, NULL, 't' },
> @@ -197,7 +202,8 @@ Print or check %s (%d-bit) checksums.\n\
>  "), stdout);
>        fputs (_("\
>  \n\
> -The following four options are useful only when verifying checksums:\n\
> +The following five options are useful only when verifying checksums:\n\
> +      --ignore-missing  don't fail or report status for missing files\n\
>        --quiet          don't print OK for each successfully verified file\n\
>        --status         don't output anything, status code shows success\n\
>        --strict         exit non-zero for improperly formatted checksum 
> lines\n\
> @@ -482,6 +488,11 @@ digest_file (const char *filename, int *binary, unsigned 
> char *bin_result)
>        fp = fopen (filename, (O_BINARY && *binary ? "rb" : "r"));
>        if (fp == NULL)
>          {
> +          if (ignore_missing && errno == ENOENT)
> +            {
> +              *bin_result = '\0';
> +              return true;
> +            }
>            error (0, errno, "%s", quotef (filename));
>            return false;
>          }
> @@ -597,6 +608,7 @@ digest_check (const char *checkfile_name)
>  
>            ++n_properly_formatted_lines;
>  
> +          *bin_buffer = '\1'; /* flag set to 0 for ignored missing files.  */
>            ok = digest_file (filename, &binary, bin_buffer);
>  
>            if (!ok)
> @@ -613,10 +625,14 @@ digest_check (const char *checkfile_name)
>            else
>              {
>                size_t digest_bin_bytes = digest_hex_bytes / 2;
> -              size_t cnt;
> +              size_t cnt = 0;
> +
> +              if (ignore_missing && ! *bin_buffer)
> +                cnt = digest_bin_bytes;
> +
>                /* Compare generated binary number with text representation
>                   in check file.  Ignore case of hex digits.  */
> -              for (cnt = 0; cnt < digest_bin_bytes; ++cnt)
> +              for (;cnt < digest_bin_bytes; ++cnt)
>                  {
>                    if (tolower (hex_digest[2 * cnt])
>                        != bin2hex[bin_buffer[cnt] >> 4]
> @@ -629,7 +645,7 @@ digest_check (const char *checkfile_name)
>  
>                if (!status_only)
>                  {
> -                  if (cnt != digest_bin_bytes || ! quiet)
> +                  if (cnt != digest_bin_bytes || (! quiet && *bin_buffer))
>                      {
>                        if (needs_escape)
>                          putchar ('\\');
> @@ -638,7 +654,7 @@ digest_check (const char *checkfile_name)
>  
>                    if (cnt != digest_bin_bytes)
>                      printf (": %s\n", _("FAILED"));
> -                  else if (!quiet)
> +                  else if (!quiet && *bin_buffer)
>                      printf (": %s\n", _("OK"));
>                  }
>              }

While the above 4 changes in the big 'else' block are correct, I think it'd
be easier and clearer to handle the case explicitly in one place, like:

@@ -610,6 +622,10 @@ digest_check (const char *checkfile_name)
                   printf (": %s\n", _("FAILED open or read"));
                 }
             }
+          else if (ignore_missing && ! *bin_buffer)
+            {
+              ;  /* Ignore actually missing file.  */
+            }
           else
             {
               size_t digest_bin_bytes = digest_hex_bytes / 2;


Otherwise +1.

Thanks & have a nice day,
Berny