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

[Jonathan Tan <jonathantanmy@google.com>]

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

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