[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: copy_file_preserving variant that doesn't exit on error?
From: |
Reuben Thomas |
Subject: |
Re: copy_file_preserving variant that doesn't exit on error? |
Date: |
Wed, 11 Jan 2012 00:42:45 +0000 |
On 11 January 2012 00:28, Bruno Haible <address@hidden> wrote:
>
> - 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?
Done.
> - 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.
Done.
> - 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.
Aha, that was the reason for the lack of error message in this case. I
probably knew that when I originally wrote the patch, but forgot, and
just followed Jim's earlier instruction to add an error message.
>> 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".
That's why I said "gnulib's strerror": by using a gnulib function it's
possible to add new error messages.
> <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.
And hence it would be nice to have a strerror that can cope with these.
> Finally, in the unit test,
>
> - Please use ASSERT (from macros.h) instead of assert (from <assert.h>).
Done.
> (Remember that assert() is eliminated if NDEBUG is not defined.)
Surely you mean "if NDEBUG is defined"? Although why would one be
running the tests with NDEBUG 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.
Done. Thanks very much for the review. New patch attached.
--
http://rrt.sc3d.org
0001-copy-file-add-error-code-returning-version.patch
Description: Text Data
- Re: copy_file_preserving variant that doesn't exit on error?, Reuben Thomas, 2012/01/09
- Re: copy_file_preserving variant that doesn't exit on error?, Jim Meyering, 2012/01/10
- Re: copy_file_preserving variant that doesn't exit on error?, Reuben Thomas, 2012/01/10
- Re: copy_file_preserving variant that doesn't exit on error?, Reuben Thomas, 2012/01/10
- Re: copy_file_preserving variant that doesn't exit on error?, Bruno Haible, 2012/01/10
- Re: copy_file_preserving variant that doesn't exit on error?,
Reuben Thomas <=
- Re: copy_file_preserving variant that doesn't exit on error?, Bruno Haible, 2012/01/10
- Re: copy_file_preserving variant that doesn't exit on error?, Reuben Thomas, 2012/01/11
- Re: copy_file_preserving variant that doesn't exit on error?, Bruno Haible, 2012/01/11
- Re: copy_file_preserving variant that doesn't exit on error?, Bruno Haible, 2012/01/11
- Re: copy_file_preserving variant that doesn't exit on error?, Reuben Thomas, 2012/01/12
- Re: copy_file_preserving variant that doesn't exit on error?, Jim Meyering, 2012/01/12
- Re: copy_file_preserving variant that doesn't exit on error?, Reuben Thomas, 2012/01/12
- Re: copy_file_preserving variant that doesn't exit on error?, Bruno Haible, 2012/01/12
- Re: copy_file_preserving variant that doesn't exit on error?, Reuben Thomas, 2012/01/12
- Re: copy_file_preserving variant that doesn't exit on error?, Bruno Haible, 2012/01/12