Re: [PATCH 1/2] git-credential-store: support XDG config dir

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2015, 18:37 UTC
Message-ID: <xmqqk2yv47fn.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACRoPnTBmqtB+mvx9wFG3EUDRjfrsM==GQsso6V3q9NHH1k-YA@mail.gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 20 quoted lines

> On Wed, Mar 4, 2015 at 7:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Paul Tan <pyokagan@gmail.com> writes:
>>
>>>       struct credential entry = CREDENTIAL_INIT;
>>> +     int found_credential = 0;
>>>
>>>       fh = fopen(fn, "r");
>>>       if (!fh) {
>>>               if (errno != ENOENT)
>>>                       die_errno("unable to open %s", fn);
>>> -             return;
>>> +             return 0;
>>
>> Returning found_credential here would be easier to read, no?  After
>> all, that is why you explicitly initialized it to 0 up there to say
>> "no we haven't found any yet".
>
> Actually I think die_errno is a function that does not return at all.
> The return is just to shut the compiler up. Perhaps I shall comment
> that.

Commenting just on this part (I am not agreeing or disagreeing with you on other parts of your message yet).

When fopen() fails because we cannot open an existing file for reading, then die_errno() will trigger and we stop there, in which case the return will not be reached.

But when we try to open fn and we fail only because fn does not exist, we do not say "die". We instead return to the caller, telling it that we have not found any credential so far in the file supplied by the caller.

So the return does matter, and spelling that zero with found_credential does matter for readability, I would think.