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