Duy Nguyen <pclouds@xxxxxxxxx> writes: > 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. 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. >> 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. To expect 'echo' into the file is to expect and encourage that people muck with the internal implementation details by hand, which we do not generally do for things inside .git [*1*]. If we consider the contents of $this_file not an implementation detail but a part of the published API (i.e. "writing this string into the file is a valid way to make Git do this"), rtrim would at least be consistent with how the existing code deals with symrefs, so I wouldn't say "does not make much sense" if you are going in that direction ;-) [Footnote] *1* ... except for .git/config, to which we say "It's a simple text file; don't be afraied to edit it away!". -- 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