Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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?
