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
