Johannes Schindelin [off-list ref] 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.
quoted
quoted
+ 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.
Hi Junio,
On Fri, 3 Sep 2021, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
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.
quoted
quoted
quoted
+ 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.
On Wed, Sep 08 2021, Johannes Schindelin wrote:
[...]
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.
I don't think we have any such exception to our usual style & preferred
code patterns in contrib/* or compat/* in general. In the latter case we
have e.g. compat/regex/ and other externally-imported codebases, which
we've tried to stylistically modify as little as possible to make
subsequent imports easier, ditto sha1dc/ etc.
I think the general (but unwritten) rule has been to draw the
distinction on whether or not code is still externally maintained and
expected to be imported, or if it's expected to be maintained in git.git
going forward.
I think that this proposed series falls thoroughly in the latter
category, but maybe I've misunderstood it..
Also re my [1] I had some (still relevant, but unaddressed) points on v1
about how placing this in contrib/* made certain aspects of integrating
it into our build system harder. I was imagining that distinction as
purely an internal implementation detail to git.git (make install
etc. would behave the same), but per the above it seems to come with
deeper connotations than that at least in your mind.
1. https://lore.kernel.org/git/87r1dydp4m.fsf@evledraar.gmail.com