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

[Jeff King <peff@peff.net>]

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 */

  /*
   * 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.

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

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