> On Tue, Dec 22, 2020 at 01:54:18PM -0800, Jonathan Tan wrote:
>
> > -static int ls_refs_config(const char *var, const char *value, void *data)
> > +static void send_possibly_unborn_head(struct ls_refs_data *data)
> > {
> > + struct strbuf namespaced = STRBUF_INIT;
> > + struct object_id oid;
> > + int flag;
> > + int oid_is_null;
> > +
> > + memset(&oid, 0, sizeof(oid));
> > + strbuf_addf(&namespaced, "%sHEAD", get_git_namespace());
> > + resolve_ref_unsafe(namespaced.buf, 0, &oid, &flag);
>
> It feels weird to call resolve_ref_unsafe() without checking the return
> value. How do we detect errors?
>
> I think the logic is that we make assumptions about which fields it will
> touch (i.e., zeroing the flags, and not touching our zero'd oid), and
> then check those. That feels a bit non-obvious and intimate with the
> implementation, though (and was presumably the source of the "oops, we
> need to clear the oid bug between v3 and v4).
>
> I feel like that deserves a comment, but I also wonder if:
>
> refname = resolve_ref_unsafe(namespaced.buf, 0, &oid, &flag);
> if (!refname)
> return; /* broken, bad name, not even a symref, etc */
>From my reading of this part of refs_resolve_ref_unsafe():
if (!(read_flags & REF_ISSYMREF)) {
if (*flags & REF_BAD_NAME) {
oidclr(oid);
*flags |= REF_ISBROKEN;
}
return refname;
}
it seems that resolve_ref_unsafe() returns non-NULL if the ref is not a
symref but is otherwise valid. But this is exactly what we want -
send_possibly_unborn_head() must send HEAD in this situation anyway.
Thanks - I've switched to checking the return value.
(It was a bit confusing that refs_resolve_ref_unsafe() returns one of
its input arguments if it succeeds and NULL if it fails, but that's
outside the scope of this patch, I think.)
> > + if (!oid_is_null ||
> > + (data->unborn && data->symrefs && (flag & REF_ISSYMREF)))
> > + send_ref(namespaced.buf, oid_is_null ? NULL : &oid, flag, data);
>
> It likewise feels a bit funny that we determine the symref name in the
> earlier call to resolve_ref_unsafe(), but we do not pass it here (and in
> fact, we'll end up looking it up again!).
>
> But that is not much different than what we do for normal refs passed to
> the send_ref() callback. It would be nice if the iteration could pass in
> "by the way, here is the symref value" to avoid that.
Yes, that would be nice.
> But in practice it
> isn't a big deal, since we only do the lookup when we see the ISSYMREF
> flag set. So typically it is only one or two extra ref resolutions.
OK.
> > @@ -91,7 +118,7 @@ int ls_refs(struct repository *r, struct strvec *keys,
> >
> > memset(&data, 0, sizeof(data));
> >
> > - git_config(ls_refs_config, NULL);
> > + git_config(ls_refs_config, &data);
>
> You will probably not be surprised that I would suggest defaulting
> data->allow_unborn to 1 before this config call. :)
I don't think many people have made comments either way, so I'll go
ahead with defaulting it to true. I can see arguments for both sides.
> > @@ -103,14 +130,31 @@ int ls_refs(struct repository *r, struct strvec *keys,
> > data.symrefs = 1;
> > else if (skip_prefix(arg, "ref-prefix ", &out))
> > strvec_push(&data.prefixes, out);
> > + else if (data.allow_unborn && !strcmp("unborn", arg))
> > + data.unborn = 1;
> > }
>
> So if we have not set allow_unborn, we will not accept the client saying
> "unborn". Which makes sense, because we would not have advertised it in
> that case.
>
> But we use the same boolean for advertising, too. So this loses the
> "allow us to accept it, but not advertise it" logic that your earlier
> versions had, doesn't it?
Yes, it does.
> And that is the important element for making
> things work across a non-atomic deploy of versions.
>
> This straight-boolean version works as long as you can atomically update
> the _config_ on each version. But that seems like roughly the same
> problem (having dealt with this on GitHub servers, they are not
> equivalent, and depending on your infrastructure, it definitely _can_ be
> easier to do one versus the other. But it seems like a funny place to
> leave this upstream feature).
Well, I was just agreeing with what you said [1]. :-)
[1] https://lore.kernel.org/git/X9xJLWdFJfNJTn0p@xxxxxxxxxxxxxxxxxxxxxxx/
> Or is the intent that an unconfigured reader would silently ignore the
> unborn flag in that case? That would at least not cause it to bail on
> the client in a mixed-version environment. But it does feel like a
> confusing result.
Right now, an old server would ignore "unborn", yes. I'm not sure of
what the intent should be - tightening ls-refs and fetch to forbid
unknown arguments seems like a good idea to me.
