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

Jonathan Tan [off-list ref] writes:

quoted

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.

Sorry, I do not quite follow.  If "unborn" is conditionally allowed,
you can extend what I suggested above like so:

	if (we see we got an unborn request) {
-		if (allowed)
+		if (partially allowed)
+			record that we got unborn request and will
+			partially respond to it
+		else if (allowed)
			record that we got unborn request;
+		else
+			report that we don't accept unborn request;
	}

This will matter even more if you write more else-if.  The
downstream of else-if clauses are forced to interpret (and fail)
"unborn" request they are not interested in.

quoted

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.

So a dangling symref, e.g. "refs/remotes/origin/HEAD -> trunk" when
no "refs/remotes/origin/trunk" exists, is not reported to send_ref()
in the same way as an unborn "HEAD"?  I would have expected that we'd
report where it points at, and for that to work, you'd have to use
not just the vanilla send_ref() as the callback, but something that
knows how to do "are we expected to send unborn symrefs" logic, like
send_possibly_unborn_head does.

That "changed to tolerate ... should work" worries me.

If "for_each_namespaced_ref(send_ref, &data)" will never call send_ref()
with NULL (as in (void *)0) oid, then that would be OK, but if it
ends up calling with NULL somehow, it is responsible to ensure that
data->symrefs is true and flag has REF_ISSYMREF set, or send_ref()
would misbehave, (see the first part of your message, which I am
responding to), no?

So a dangling symref, e.g. "refs/remotes/origin/HEAD -> trunk" when
no "refs/remotes/origin/trunk" exists, is not reported to send_ref()
in the same way as an unborn "HEAD"?

I've tried it, and yes, for_each_namespaced_ref() will not report it.
Looking at the code, I think it is packed_ref_iterator_advance() which
checks for broken objects and skips over them.

I would have expected that we'd
report where it points at, and for that to work, you'd have to use
not just the vanilla send_ref() as the callback, but something that
knows how to do "are we expected to send unborn symrefs" logic, like
send_possibly_unborn_head does.

That "changed to tolerate ... should work" worries me.

If "for_each_namespaced_ref(send_ref, &data)" will never call send_ref()
with NULL (as in (void *)0) oid, then that would be OK,

If it called send_ref() with (void *)0 OID, it would segfault with the
current code, which calls oid_to_hex() on the OID unconditionally.

but if it
ends up calling with NULL somehow, it is responsible to ensure that
data->symrefs is true and flag has REF_ISSYMREF set, or send_ref()
would misbehave, (see the first part of your message, which I am
responding to), no?

If it did, then yes.