Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> It would even work if the current line is shorter, but as you point out:
> it is wasteful. And it could be improved to be more readable. I reworked
> it, and it now looks like this:
>
> if (!pipe_command(&cp, NULL, 0, &out, 0, NULL, 0)) {
> const char *line = out.buf;
>
> while (*line) {
> const char *eol = strchrnul(line, '\n'), *p;
> size_t len = eol - line;
> char *branch;
>
> if (!skip_prefix(line, "ref: ", &p) ||
> !strip_suffix_mem(line, &len, "\tHEAD")) {
> line = eol + (*eol == '\n');
> continue;
> }
>
> eol = line + len;
> if (skip_prefix(p, "refs/heads/", &p)) {
> branch = xstrndup(p, eol - p);
> strbuf_release(&out);
> return branch;
> }
>
> error(_("remote HEAD is not a branch: '%.*s'"),
> (int)(eol - p), p);
> strbuf_release(&out);
> return NULL;
> }
> }
>
> It now parses the output line by line, looking for the expected prefix and
> suffix, then verifies the ref name format, and either returns the short
> branch name or errors out with the message that this is not a branch.
It is much easier to read and understand how the loop works with
above.
>> > + error(_("remote HEAD is not a branch: '%.*s'"),
>> > + (int)(head - ref), ref);
>> > + strbuf_release(&out);
>> > + return NULL;
>>
>> OK. Any symref whose basename is HEAD in their remote-tracking
>> hierarchy would have been rejected earlier in the loop.
>>
>> Is there a particular reason why we return early here, instead of
>> breaking out of hte loop and let the generic "failed to get" code
>> path below to handle this case?
>
> Yes, the reason is that I wanted to err on the side of caution. If the
> remote repository reports a default branch that is not a default branch at
> all, I do not want to pretend that things are fine and then run into
> trouble later when we set up a non-branch as remote-tracking target or
> something like that.
Wouldn't we have the same problem when the remote end does not
advertise HEAD and we fall back to "local default", though? We'd
run into trouble later as we use "local default" that may correspond
to a non-branch there as remote-tracking target, or something like
that.
Not that I care too deeply in the error case, though. I just felt
that this early return was an uneven way to follow the principle to
err on the side of caution, as we continue with the local default
when the other side fails to tell us what their HEAD points at.
Thanks.
