Re: [PATCH v4 1/3] ls-refs: report unborn targets of symrefs
- From
Jeff King <peff@peff.net>
- Date
- Jan 21, 2021, 20:48 UTC
- Message-ID
- <YAnotHAiuSz4Du/0@coredump.intra.peff.net>
- In-Reply-To
- <a66e50626ef9343e0351fc6cfe8abcf4f9eed7f3.1608673963.git.jonathantanmy@google.com>
On Tue, Dec 22, 2020 at 01:54:18PM -0800, Jonathan Tan wrote:
Show 11 quoted lines
> -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 */
/* * now we can look at oid even if we didn't memset() it, because * a successful return from resolve_ref_unsafe() means that it * has cleared it if appropriate */ oid_is_null = is_null_oid(&oid); ...etc...
> + 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. 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.
Show 6 quoted lines
> @@ -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. :)
Show 7 quoted lines
> @@ -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? 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).
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.
-Peff