Re: [PATCH v2 1/3] ls-refs: report unborn targets of symrefs
From: Jonathan Tan <hidden>
Date: 2020-12-16 23:50:16
quoted
@@ -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.
quoted
+ 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.
quoted
@@ -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.)
quoted
- 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.