Hi Junio,
On Fri, 3 Sep 2021, Junio C Hamano wrote:
> 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.
Excellent.
> >> > + 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.
All true, there will be trouble at some stage.
The difference between the two cases, in my mind, is that cloning an empty
repository would run into the latter code path and might still have a
chance to work as intended by using the local default branch name.
In any case, as I indicated earlier, I am _very_ interested in moving as
much functionality as possible from Scalar to Git proper. In this
instance, I hope to move most of the code from `scalar.c` to `clone.c`
(guarded by one or more new options). And as soon as that happens, the
discussion we're having right now will be moot ;-)
Which means that I want to weigh how much effort to put into polishing an
unlikely code path on one side, and on the other side how much effort to
put into moving the functionality away from `contrib/` and deleting that
unlikely code path.
In the same vein, while this patch series contains (mostly) code in
`contrib/` (and therefore technically does not need to adhere strictly to
Git's code style), it is probably wise to pay closer attention to the code
style particularly in those parts that are prone to be moved verbatim (or
close to verbatim) to Git proper.
Thanks,
Dscho
> 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.
>
