Re: copy_file_preserving variant that doesn't exit on error?

[Bruno Haible]

Hi Reuben,

Thanks for working on this refactoring.

Reuben Thomas wrote:
> Here's a revised version of my patch.

A couple of comments. Yes I heard that it's not so much fun to hear picky
comments, but quality...

- The function _copy_file_preserving is not declared nor used outside
  that file. Hence it ought to be marked 'static'.

- But actually the function copy_file_preserving_error is equivalent to
  _copy_file_preserving. So why not just rename _copy_file_preserving
  to copy_file_preserving_error and ditch the one-line wrapper?

- About the naming of the function. We already have two conventions for
  a new function that does not signal an error:
    - In set-mode-acl.c the prefix 'q' (for "quiet").
    - In gl_xoset.h the prefix 'nx_' (as an antagonism to the prefix 'x').
  It would be good if you could adhere to one of these conventions instead
  of inventing a third one.

- In the 'case GL_COPY_ERR_ACL' the function copy_acl() has already
  emitted an error message. What you want, I think, is to call qcopy_acl()
  instead of copy_acl. But qcopy_acl is not yet public. Let me address this
  in a separate patch.

> Is there no way to deal with error messages normally, i.e. via
> gnulib's strerror? Then the error-returning copy_file_preserving could
> replace the aborting version, and users who want to could check the
> return code and issue errors in the usual way.

This would be a regression on the quality of the messages. There is no
errno code for "cannot open backup file for writing".

<errno.h> contains shorthands for the most commonly encountered error
cases. It is absolutely normal that more specialized functions have to
invent their own error codes.

Finally, in the unit test,

  - Please use ASSERT (from macros.h) instead of assert (from <assert.h>).
    (Remember that assert() is eliminated if NDEBUG is not defined.)

  - For structuring the test code, better move all the code that tests
    copy_file_preserving() into a function of its own, and the new code
    that tests copy_file_preserving_error() also into a function of its own.
    Like it is done in test-regex-quote.c or test-xvasprintf.c.

Bruno