Re: [PATCH/WIP v3 01/31] wrapper: implement xopen()

[Johannes Schindelin <johannes.schindelin@gmx.de>]

Hi Stefan,

On 2015-06-24 18:59, Stefan Beller wrote:
> On Wed, Jun 24, 2015 at 9:28 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>>
>> On 2015-06-18 13:25, Paul Tan wrote:
>>
>>> +             int fd = open(path, oflag, mode);
>>> +             if (fd >= 0)
>>> +                     return fd;
>>> +             if (errno == EINTR)
>>> +                     continue;
>>> +             die_errno(_("could not open '%s'"), path);
>>
>> It is often helpful to know whether a path was opened for reading or writing, so maybe we should have something like
>>
>> if (oflag & O_WRITE)
>>     die_errno(_("could not open '%s' for writing"), path);
>> else if (oflag & O_READ)
>>     die_errno(_("could not open '%s' for reading"), path);
>> else
>>     die_errno(_("could not open '%s'"), path);
>>
>> ? I know it is a bit of duplication, but I fear we cannot get around that without breaking i18n support.
> 
> This distinction was part of earlier series, but Torsten Boegershausen
> suggested to leave
> it out. [compare
> http://thread.gmane.org/gmane.comp.version-control.git/270048/focus=270049]

So sorry that I missed that (it is still somewhere in my ever-growing inbox). I would have politely disagreed with Torsten if I had not missed it, though.

IMO the varargs make the code more cumbersome to read (and even fragile, because you can easily call `xopen(path, O_WRITE | O_CREATE)` and would not even get so much as a compiler warning!) and the error message does carry value: it helps you resolve the issue (it is completely unnecessary to check write permissions of the directory when a file could not be opened for reading, for example, yet if the error message does not say that and you suspect that the file could not be opened for *writing* that is exactly what you would waste your time checking).

Ciao,
Dscho