Re: RFE: hash-type in sum utils

[Ondrej Oprala]

Ok, I've made all the changes(though I had to change print_filename contents 
around a bit, it wouldn't
print out the file name properly unless the name consisted of '\n' and '\' 
chars only), added a NEWS entry
and a simple test to md5sum-bsd. Syntax-check was also fine with my changes.
Cheers,
 Ondrej

----- Original Message -----
From: "Jim Meyering" <address@hidden>
To: "Ondrej Oprala" <address@hidden>
Cc: "Pádraig Brady" <address@hidden>, "D Yu Bolkhovityanov" <address@hidden>, 
address@hidden
Sent: Monday, July 30, 2012 3:43:45 PM
Subject: Re: RFE: hash-type in sum utils

Ondrej Oprala wrote:
> Alright, I've redone it so now the hashes create and expect GNU escaping
>  in both the BSD and GNU output format.
> I left the --tag format indication of text files to be a space before
> the algo. name
> so far, but I can change that if needed.
...
> +static const char *const prefixes[] =
> +{
> +  "MD5 ",
> +  "SHA1 ",
> +  "SHA256 ",
> +  "SHA224 ",
> +  "SHA512 ",
> +  "SHA384 "
> +};
> +
...
> +                  fputs (prefixes[PREFIX_INDEX], stdout);

It looks like you can remove that "prefixes" array and replace
the sole use with this, no?

                     fputs (DIGEST_TYPE_STRING, stdout);
                     putchar (' ');

> +static bool
> +filename_escape (char *s, int s_len, char **file_name)

s/int/size_t/

> +{
> +      /* Translate each '\n' string in the file name to a NEWLINE,
> +         and each '\\' string to a backslash.  */

Please put the function-describing comment before the function
definition.  It should mention the meaning of the S_LEN
and **FILE_NAME parameters and should explain the meaning of
the return value.

/* Translate each '\n' string in the file name, S, to a NEWLINE,
   and each '\\' string to a backslash. ...  */

Please change the name to e.g. filename_unescape, since it is not
escaping special characters, but performing the reverse operation.
It is unescaping them.

> +  *file_name = s;

I would set this only at the end, just before returning "true".

> +  char *dst = s;
> +  int i = 0;

"i" is always non-negative and indexes a string, so please use size_t
in place of that "int".

> +  while (i < s_len)
> +    {
> +      switch (s[i])
> +        {
> +        case '\\':
> +          if (i == s_len - 1)
> +            {
> +              /* A valid line does not end with a backslash.  */
> +              return false;
> +            }
> +          ++i;
> +          switch (s[i++])
> +            {
> +            case 'n':
> +              *dst++ = '\n';
> +              break;
> +            case '\\':
> +              *dst++ = '\\';
> +              break;
> +            default:
> +              /* Only '\' or 'n' may follow a backslash.  */
> +              return false;
> +            }
> +          break;
> +
> +        case '\0':
> +          /* The file name may not contain a NUL.  */
> +          return false;
> +          break;

The "break" after return may be classified as unreachable,
so please remove it.

> +        default:
> +          *dst++ = s[i++];
> +          break;
> +        }
> +    }
> +  *dst = '\0';
> +  return true;
> +}

> +static void
> +print_filename (char *file)

Please make that parameter const:

    print_filename (char const *file)


> +{
> +  size_t i;
> +  /* Translate each NEWLINE byte to the string, "\\n",
> +     and each backslash to "\\\\".  */

Please use the following instead.
Otherwise, the code traverses FILE twice.
first via strlen, and again via the loop.

     while (*file)
       {
         switch (*file++)

         [and s/file[i]/*file/ in the putchar]

> +  for (i = 0; i < strlen (file); ++i)
> +    {
> +      switch (file[i])
> +        {
> +        case '\n':
> +          fputs ("\\n", stdout);
> +          break;
> +
> +        case '\\':
> +          fputs ("\\\\", stdout);
> +          break;
> +
> +        default:
> +          putchar (file[i]);
> +          break;
> +        }
> +    }
> +}

Once you've done the above, consider adding tests to exercise the change
as well as a NEWS entry.

Finally, please run "make syntax-check".
Among other things, that will ensure that your change does
not add trailing blanks.

+                  if (file_is_binary)
+                    putchar ('*');
+                  else
+                    putchar (' ');

Please replace those four lines with this one:
    putchar (file_is_binary ? '*' : ' ');

you could slightly more concise, but I would not suggest this :-)
    putchar (" *"[!!file_is_binary]);

sum-tags.patch
Description: Text Data