Thread (2 messages) 2 messages, 2 authors, 2020-12-16

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help