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

From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v5 1/3] ls-refs: report unborn targets of symrefs
Date: Fri, 29 Jan 2021 14:04:18 -0800	[thread overview]
Message-ID: <xmqqtuqzs8m5.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20210129202306.723272-1-jonathantanmy@google.com> (Jonathan Tan's message of "Fri, 29 Jan 2021 12:23:06 -0800")

Jonathan Tan <jonathantanmy@google.com> writes:

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

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