Duy Nguyen <pclo...@gmail.com> writes:
> On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano <gits...@pobox.com> wrote:
> ...
>> The reason to store the errno in saved_errno here is not because we
>> want to help code after "if (res) {...}", but the patch sent as-is
>> gives that impression and is confusing to the readers.
>>
>> Perhaps all hunks of this patch share the same issue? I could
>> locally amend, of course, but I'd like to double check before doing
>> so myself---perhaps you did it this way for a good reason that I am
>> missing?
>
> One thing I like about putting saved_errno right next to the related
> syscall is, the syscall is visible from the diff (previously some are
> out of context). This is really minor though.
I agree that this is minor.
I care more about looking at errno ONLY after we saw our call to a
system library function indicated an error, and I wanted to avoid
doing:
res = dry_run ? 0 : rmdir(path);
saved_errno = errno;
if (res) {
... do something else ...
errno = saved_errno;
call_something_that_uses_errno();
When our call to rmdir() did not fail, or when we didn't even call
rmdir() at all, what is in errno has nothing to do with what we are
doing, and making a copy of it makes the code doubly confusing.
Rather, I'd prefer to see:
res = dry_run ? 0 : rmdir(path);
if (res) {
int saved_errno = errno;
... do something else ...
errno = saved_errno;
call_something_that_uses_errno();
which makes it clear what is going on when reading the resulting
code.
For now, I'll queue a separate SQUASH??? and wait for others to
comment.
Thanks.
