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

Jonathan Tan [off-list ref] writes:

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.

And this caller makes sure that send_ref()'s expectation holds.

The above is a usual sequence of "an unspecified allow-unborn
defaults to true, but the configuration can turn it off".  OK

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

On Tue, Jan 26, 2021 at 01:38:49PM -0800, Junio C Hamano wrote:

quoted

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

Doing it that way is friendlier, but loosens enforcement of:

  Client will then send a space separated list of capabilities it wants
  to be in effect. The client MUST NOT ask for capabilities the server
  did not say it supports.

from Documentation/technical/protocol-capabilities.txt.

It does solve Jonathan's racy cluster-deploy problem, though. See the
discussion in the v4 thread (sorry, seems not to have hit the archive
yet, but hopefully this link will work soon):

  https://lore.kernel.org/git/YBCitNb75rpnuW2L@coredump.intra.peff.net/

-Peff

quoted

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

Thanks for checking.

quoted

 	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.

Thanks for checking.

quoted

+	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

Later, you address this issue again so I'll comment there.

quoted

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

My thinking was (and is) that falling through in the case of a
disallowed argument (as opposed to a completely unrecognized argument)
makes it more straightforward later if we ever decide to tighten
validation of the ls-refs request - we would only have to put some code
at the end that reports back to the user.

If we write it as you suggest, we would have to remember to replace the
"we are not accepting the request" part (as in the comment in your
suggested code) with an error report, but perhaps that is a good thing -
we would be able to insert a custom error message instead of an
information-hiding "argument not supported".

I'm OK either way.

quoted

 	}
 
 	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)?

I assume that by "this caller" you mean for_each_namespaced_ref(), since
you mention an iteration. I believe so - send_ref has been changed to
tolerate a NULL (as in (void*)0, not 0{40}) oid, and that is the only
change, so if it worked previously, it should still work now.

quoted

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

Perhaps what I could do is have a static variable that tracks whether
config has been read and what the config is (or if the default variable
is used), and have each function call another function that sets that
static variable if config has not yet been read. I think that will
address this concern.