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

> > @@ -47,7 +49,10 @@ static int send_ref(const char *refname, const struct object_id *oid,
> >  	if (!ref_match(&data->prefixes, refname_nons))
> >  		return 0;
> >  
> > -	strbuf_addf(&refline, "%s %s", oid_to_hex(oid), refname_nons);
> > +	if (oid)
> > +		strbuf_addf(&refline, "%s %s", oid_to_hex(oid), refname_nons);
> > +	else
> > +		strbuf_addf(&refline, "unborn %s", refname_nons);
> 
> When a call is made to this helper with NULL "oid", it unconditionally
> sends the "refname" out as an 'unborn' thing.  If data->symrefs is not
> true, or flag does not have REF_ISSYMREF set, then we'd end up
> sending
> 
>     "unborn" SP refname LF
> 
> without any ref-attribute.  The caller is responsible for ensuring
> that it passes sensible data->symrefs and flag when it passes
> oid==NULL to this function, but it is OK because this is a private
> helper.
> 
> OK.

> >  	if (data->symrefs && flag & REF_ISSYMREF) {
> >  		struct object_id unused;
> >  		const char *symref_target = resolve_ref_unsafe(refname, 0,
> > @@ -74,8 +79,30 @@ static int send_ref(const char *refname, const struct object_id *oid,
> >  	return 0;
> >  }
> >  
> > -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;
> > +
> > +	strbuf_addf(&namespaced, "%sHEAD", get_git_namespace());
> > +	if (!resolve_ref_unsafe(namespaced.buf, 0, &oid, &flag))
> > +		return; /* bad ref */
> > +	oid_is_null = is_null_oid(&oid);
> > +	if (!oid_is_null ||
> > +	    (data->unborn && data->symrefs && (flag & REF_ISSYMREF)))
> > +		send_ref(namespaced.buf, oid_is_null ? NULL : &oid, flag, data);
> 
> And this caller makes sure that send_ref()'s expectation holds.

> > +	strbuf_release(&namespaced);
> > +}
> > +
> > +static int ls_refs_config(const char *var, const char *value, void *cb_data)
> > +{
> > +	struct ls_refs_data *data = cb_data;
> > +
> > +	if (!strcmp("lsrefs.allowunborn", var))
> > +		data->allow_unborn = git_config_bool(var, value);
> > +
> >  	/*
> >  	 * We only serve fetches over v2 for now, so respect only "uploadpack"
> >  	 * config. This may need to eventually be expanded to "receive", but we
> > @@ -91,7 +118,8 @@ int ls_refs(struct repository *r, struct strvec *keys,
> >  
> >  	memset(&data, 0, sizeof(data));
> >  
> > -	git_config(ls_refs_config, NULL);
> > +	data.allow_unborn = 1;
> > +	git_config(ls_refs_config, &data);
> 
> The above is a usual sequence of "an unspecified allow-unborn
> defaults to true, but the configuration can turn it off".  OK

> > @@ -103,14 +131,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;
> 
> I think the use of &&-cascade is iffy here.  Even when we are *not*
> accepting request for unborn, we should still parse it as such.
> This does not matter in today's code, but it is a basic courtesy for
> future developers who may add more "else if" after it.
> 
> IOW
> 
> 		else if (!strcmp("unborn", arg)) {
> 			if (!data.allow_unborn)
> 				; /* we are not accepting the request */
> 			else
> 				data.unborn = 1;
> 		}
> 
> I wrote the above in longhand only for documentation purposes; in
> practice, 
> 
> 		else if (!strcmp("unborn", arg))
>                 	data.unborn = data.allow_unborn;
> 
> may suffice.

> >  	}
> >  
> >  	if (request->status != PACKET_READ_FLUSH)
> >  		die(_("expected flush after ls-refs arguments"));
> >  
> > -	head_ref_namespaced(send_ref, &data);
> > +	send_possibly_unborn_head(&data);
> >  	for_each_namespaced_ref(send_ref, &data);
> 
> And here is another caller of send_ref().  Are we sure that
> send_ref()'s expectation is satisfied by this caller when the
> iteration encounters a broken ref (e.g. refs/heads/broken not a
> symref but names an object that does not exist and get_sha1()
> yielding 0{40}), or a dangling symref (e.g. refs/remotes/origin/HEAD
> pointing at something that does not exist)?

> >  	packet_flush(1);
> >  	strvec_clear(&data.prefixes);
> >  	return 0;
> >  }
> > +
> > +int ls_refs_advertise(struct repository *r, struct strbuf *value)
> > +{
> > +	if (value) {
> > +		int allow_unborn_value;
> > +
> > +		if (repo_config_get_bool(the_repository,
> > +					 "lsrefs.allowunborn",
> > +					 &allow_unborn_value) ||
> > +		    allow_unborn_value)
> > +			strbuf_addstr(value, "unborn");
> > +	}
> 
> This reads "when not explicitly disabled, stuff "unborn" in there".
> 
> It feels somewhat brittle that we have to read the same variable and
> apply the same "default to true" logic in two places and have to
> keep them in sync.  Is this because the decision to advertize or not
> has to be made way before the code that is specific to the
> implementation of ls-refs is run?
> 
> If ls_refs_advertise() is always called first before ls_refs(), I
> wonder if it makes sense to reuse what we found out about the
> configured (or left unconfigured) state here and use it when
> ls_refs() gets called?  I know that the way serve.c infrastructure
> calls "do we advertise?" helper from each protocol-element handler
> is too narrow and does not allow us to pass such a necessary piece
> of information but I view it as a misdesign that can be corrected
> (and until that happens, we could use file-local static limited to
> ls-refs.c).