Re: [PATCH 1/2] xopen: explicitly report creation failures

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2021, 16:49 UTC
Message-ID: <xmqqa6l4f8x4.fsf@gitster.g>
In-Reply-To: <f44bf316-efae-34ce-33e0-0161c3bb78a0@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 19 quoted lines

> Am 26.08.21 um 01:46 schrieb Carlo Arenas:
>> On Wed, Aug 25, 2021 at 2:11 PM René Scharfe <l.s.r@web.de> wrote:
>>>
>>> diff --git a/wrapper.c b/wrapper.c
>>> index 563ad590df..7c6586af32 100644
>>> --- a/wrapper.c
>>> +++ b/wrapper.c
>>> @@ -193,7 +193,9 @@ int xopen(const char *path, int oflag, ...)
>>>                 if (errno == EINTR)
>>>                         continue;
>>>
>>> -               if ((oflag & O_RDWR) == O_RDWR)
>>> +               if ((oflag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
>>> +                       die_errno(_("unable to create '%s'"), path);
>>
>> probably over conservative, but && errno == EEXIST?
>
> No matter what error we got, if O_CREAT and O_EXCL were both given then
> we tried to create a file, so this message applies.

100% agreed.

Show 15 quoted lines

>>> +               else if ((oflag & O_RDWR) == O_RDWR)
>>>                         die_errno(_("could not open '%s' for reading and writing"), path);
>>>                 else if ((oflag & O_WRONLY) == O_WRONLY)
>>>                         die_errno(_("could not open '%s' for writing"), path);
>>
>> Since you are already changing this code, why not take the opportunity
>> to refactor it
>> and remove the " == FLAG" part of these conditionals which is
>> otherwise redundant?
>
> The repetition is unsightly, but it's a different issue that should be
> addressed separately.  Simply removing the comparison feels iffy,
> though.  POSIX doesn't seem to forbid e.g. O_RDONLY to be 1, O_WRONLY
> to be 2 and O_RDWR to be 3, and then you need to check all masked bits.
> I can't think of simpler alternative to the comparison.

I fully agree that such a change, if done, must be done in an unrelated patch.

It is funny that the code is already prepared for such a case where RDWR is defined as RDONLY|WRONLY. I wonder if we wrote the series of comparisons in this order on purpose, or we were just lucky, when we did 3ff53df7 (wrapper: implement xopen(), 2015-08-04) ;-)

>
>> Either way "Reviewed-by", and indeed a nice cleanup.
>
> Thank you!

Yes, indeed, this is nicely done.