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

[Jonathan Tan <jonathantanmy@google.com>]

> > @@ -74,8 +79,28 @@ 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 null_oid;
> 
> I'd suggest renaming this one, which masks the global null_oid of
> "const struct object_id" type.  This code does not break only
> because is_null_oid() happens to be implemented as a static inline,
> and not as a C-preprocessor macro, right?

OK - will rename.

> > +	strbuf_addf(&namespaced, "%sHEAD", get_git_namespace());
> > +	resolve_ref_unsafe(namespaced.buf, 0, &oid, &flag);
> > +	null_oid = is_null_oid(&oid);
> > +	if (!null_oid || (data->symrefs && (flag & REF_ISSYMREF)))
> > +		send_ref(namespaced.buf, null_oid ? NULL : &oid, flag, data);
> > +	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.unborn", var))
> > +		data->allow_unborn = !strcmp(value, "allow") ||
> > +			!strcmp(value, "advertise");
> 
> Are there differences between allow and advertise?  Would
> lsrefs.allowUnborn that is a boolean, thus allowing the value to be
> parsed by git_config_bool(), make more sense here, I wonder.  Or is
> this meant as some future enhancement, e.g. you plan to have some
> servers that allow "unborn" request even though they do not actively
> advertise the support of the feature?  Without documentation update
> or an in-code comment, it is rather hard to guess the intention
> here.

I'll update the documentation. With this current patch, yes, some
servers will allow "unborn" requests even though they do not actively
advertise it. This allows servers in load-balanced environments to first
be configured to support the feature, then after ensuring that the
configuration for all servers is complete, to turn on advertisement.

> > @@ -91,7 +116,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);
> >  
> >  	while (packet_reader_read(request) == PACKET_READ_NORMAL) {
> >  		const char *arg = request->line;
> > @@ -103,14 +128,35 @@ 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;
> 
> Somehow, it appears to me that writing it in a way along with this
> line ...
> 
> 		else if (!strcmp("unborn", arg))
> 			data.unborn = data.allow_unborn;
> 
> ... would make more sense.  Whether we allowed "unborn" request or
> not, when the other side says "unborn", we are handling the request
> for the unborn feature, and the condition with strcmp() alone
> signals that better (in other words, when we acquire more request
> types, we do not want to pass the control to "else if" clauses that
> may come after this part when we see "unborn" request and when we
> are configured not to accept "unborn" requests.
> 
> It does not make any difference in the current code, of course, and
> it is more about future-proofing the cleanness of the code.

Good point. I'll go ahead and write it as you describe.

I was following the style in upload-pack, where writing it my way versus
your way would make a difference because we die on invalid arguments at
the end. (It does raise the question whether we should die on invalid
arguments, but maybe that's for another time.)

> 
> > -	head_ref_namespaced(send_ref, &data);
> > +	if (data.unborn)
> > +		send_possibly_unborn_head(&data);
> > +	else
> > +		head_ref_namespaced(send_ref, &data);
> 
> I found the "send_possibly 70% duplicates what the more generic
> head_ref_namespaced() does" a bit disturbing.

There's more duplication in refs.c (e.g. head_ref_namespaced() and
refs_head_ref()) too. I'll see if I can refactor those into something
more generic.