Junio C Hamano [off-list ref] writes:
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).
After giving the above a bit more thought, here are a few random
thoughts around the area.
* As "struct protocol_capability" indicates, we have <name of
service, the logic to advertise, the logic to serve> as a
three-tuple for services. The serving logic should know what
advertising logic advertised (or more precisely, what information
advertising logic used to make that decision) so that they can
work consistently.
For that, there should be a mechanism that advertising logic can
use to leave a note to serving logic, perhaps by adding a "void
*" to both of these functions. The advertising function would
allocate a piece of memory it wants to use and returns the
pointer to it to the caller in serve.c, and that pointer is given
to the corresponding ls_refs() when it is called by serve.c.
Then ls_refs_advertise can say "I found this configuration
setting and decided to advertise" to later ls_refs() and the
latter can say "ah, as you have advertised, I have to respond to
such a request".
* I am not sure if "lsrefs.allowunborn = yes/no" is a good way to
configure this feature. Wouldn't it be more natural to make this
three-way, i.e. "lsrefs.unborn = advertise/serve/ignore", where
the server operator can choose among (1) advertise the presence
of the capability and respond to requests, (2) do not advertise
the capability but if a request comes, respond to it, and (3) do
not advertise and do not respond. We could throw in 'deny' that
causes the request to result in a failure but I do not care too
deeply about that fourth option.
Using such a configuration mechanism, ls_refs_advertise may leave
the value of "lsrefs.unborn" (or lack thereof) it found and used
to base its decision to advertise, for use by ls_refs. ls_refs
in turn can use the value found there to decide if it ignores or
responds to the "unborn" request.
Junio C Hamano [off-list ref] writes:
quoted
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).
After giving the above a bit more thought, here are a few random
thoughts around the area.
* As "struct protocol_capability" indicates, we have <name of
service, the logic to advertise, the logic to serve> as a
three-tuple for services. The serving logic should know what
advertising logic advertised (or more precisely, what information
advertising logic used to make that decision) so that they can
work consistently.
For that, there should be a mechanism that advertising logic can
use to leave a note to serving logic, perhaps by adding a "void
*" to both of these functions. The advertising function would
allocate a piece of memory it wants to use and returns the
pointer to it to the caller in serve.c, and that pointer is given
to the corresponding ls_refs() when it is called by serve.c.
Then ls_refs_advertise can say "I found this configuration
setting and decided to advertise" to later ls_refs() and the
latter can say "ah, as you have advertised, I have to respond to
such a request".
Usually the advertising is in the same file as the serving (true for
ls-refs and fetch, so far) so I think it's easier if they just
communicate on their own instead of through this "void *". I agree with
the communication idea, though.
* I am not sure if "lsrefs.allowunborn = yes/no" is a good way to
configure this feature. Wouldn't it be more natural to make this
three-way, i.e. "lsrefs.unborn = advertise/serve/ignore", where
the server operator can choose among (1) advertise the presence
of the capability and respond to requests, (2) do not advertise
the capability but if a request comes, respond to it, and (3) do
not advertise and do not respond. We could throw in 'deny' that
causes the request to result in a failure but I do not care too
deeply about that fourth option.
Using such a configuration mechanism, ls_refs_advertise may leave
the value of "lsrefs.unborn" (or lack thereof) it found and used
to base its decision to advertise, for use by ls_refs. ls_refs
in turn can use the value found there to decide if it ignores or
responds to the "unborn" request.
lsrefs.unborn = advertise/serve/ignore was how it was in version 2 [1]
(with different names) and I changed it due to Peff's suggestion, but
perhaps I landed up in the unhappy middle [2]. I think we should just
pick a standard and use it for this feature and whatever future features
may come. The distinction between advertise and serve ("allow" in my
version 2) is useful in some cases but is useless once migration has
occurred (and thus clutters the code), but perhaps it could be argued
that all servers need to do the migration at least once.
[1] https://lore.kernel.org/git/cover.1608084282.git.jonathantanmy@google.com/
[2] https://lore.kernel.org/git/YBCitNb75rpnuW2L@coredump.intra.peff.net/