Re: [PATCH 3/5] checkout --to: no auto-detach if the ref is already checked out

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2014, 18:13 UTC
Message-ID: <xmqq61imaamd.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqr41aaey0.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 24 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Thu, Jul 24, 2014 at 4:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> +     if (strbuf_read_file(&sb, path.buf, 0) <= 0 ||
>>>> +         !skip_prefix(sb.buf, "ref:", &start))
>>>> +             goto done;
>>>>       while (isspace(*start))
>>>>               start++;
>>>>       end = start;
>>>>       while (*end && !isspace(*end))
>>>>               end++;
>>>
>>> Not new in this round of update, and may not even be an issue, but:
>>>
>>>  - Earlier, the code returned early on a negative return value from
>>>    read-file (i.e., an error), but this round it also does so for
>>>    zero.  Intended?
>>
>> Yes. But it does not make any difference. strbuf_read_file returns
>> sb.len, if it's empty, the next skip_prefix would fail anyway.
>
> Yes but changing < 0 to <= 0 is inconsistent with that; I would
> understand if you changed it to <= 4, which would be consistent with
> the reasoning, though.

Just to make sure. I am not saying changing < 0 to <= 4 is a good idea. I think checking for strbuf_read_file() failure with < 0 and then checking for malformatted input (or detached head perhaps?) with !skip_prefix(), i.e. testing two logically separate things with two separate conditions concatenated together with ||, would be the most natural way to express it.