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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 29, 2021, 22:04 UTC
Message-ID: <xmqqtuqzs8m5.fsf@gitster.c.googlers.com>
In-Reply-To: <20210129202306.723272-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 27 quoted lines

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

Show 18 quoted lines

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