Re: [PATCH v4 1/3] ls-refs: report unborn targets of symrefs

On Tue, Dec 22, 2020 at 01:54:18PM -0800, Jonathan Tan wrote:

quoted

-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.)

quoted

+	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.

quoted

@@ -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.

quoted

@@ -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@coredump.intra.peff.net/ (local)

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.