On Thu, Jul 24, 2014 at 4:16 AM, Junio C Hamano <gitster@xxxxxxxxx> 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.
> - 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?
OK. Will do (unless Mike has a different opinion about this).
>
>> + 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.
I think more about "echo abc > $this_file" where the echo command may
output '\r\n' on Windows (wild guess though, I don't use Windows
much). I think I'm going with _rtrim.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html