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

>> +     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?

>  - The above duplicates the logic in resolve_ref_unsafe() and
>    resolve_gitlink_ref_recursive(); three places now knows what a
>    textual symref looks like (i.e. begins with "ref:", zero or more
>    whitespaces, the target ref and then zero or more trailing
>    whitespaces).  Perhaps we need to consolidate the code further,
>    so that this knowledge does not leak out of refs.c?

>
>> +     if (strncmp(start, new->path, end - start) ||
>> +         new->path[end - start] != '\0')
>> +             goto done;
>> +     if (id) {
>> +             strbuf_reset(&path);
>> +             strbuf_addf(&path, "%s/repos/%s/gitdir",
>> +                         get_git_common_dir(), id);
>> +             if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
>> +                     goto done;
>> +             while (gitdir.len && (gitdir.buf[gitdir.len - 1] == '\n' ||
>> +                                   gitdir.buf[gitdir.len - 1] == '\r'))
>> +                     gitdir.buf[--gitdir.len] = '\0';
>
> Accepting arbitrary numbers of '\r' and '\n' sounds as if the code
> is allowing it, but text editors would not end their files with a
> nonsense sequence like "\r\r\n\r" unless the end-user works to do
> so, and if you are prepared to be lenient to noisy human input, not
> trimming trailing whitespaces does not look it is going far enough
> to help them.
>
> I do not see a good reason to allow random text editors to edit this
> file in the first place, so my preference is:
>
>         if (strbuf_read_file(...) < 0 ||
>             gitdir.len == 0 ||
>             gitdir.buf[gitdir.len - 1] != '\n')
>             goto error_return;
>         gitdir.buf[--gitdir.len] = '\0';
>
> Alternatively, if you are trying to be lenient to human input, I
> would understand:
>
>         if (strbuf_read_file(...) < 0)
>                 goto error_return;
>         strbuf_rtrim(&gitdir);
>
> The code in the patch, which is something in between, does not make
> much sense to me.