Calvin Wan <calvinwan@xxxxxxxxxx> writes:
> Currently, object-info is always advertised by the server. Add a
> config option to optionally advertise it. A subsequent patch uses this
> option for testing.
>
> ---
> Documentation/config/transfer.txt | 4 ++++
> protocol-caps.c | 11 +++++++++++
> protocol-caps.h | 6 ++++++
> serve.c | 2 +-
> 4 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt
> index b49429eb4d..14892a3155 100644
> --- a/Documentation/config/transfer.txt
> +++ b/Documentation/config/transfer.txt
> @@ -77,3 +77,7 @@ transfer.unpackLimit::
> transfer.advertiseSID::
> Boolean. When true, client and server processes will advertise their
> unique session IDs to their remote counterpart. Defaults to false.
> +
> +transfer.advertiseObjectInfo::
> + Boolean. When true or not set, server processes will advertise its
> + ability to accept `object-info` command requests
> \ No newline at end of file
Carefully proofread before sending your changes out ;-)
By mimicking the style of the previous item more closely, i.e.
Boolean. When true, ... requests. Defaults to true.
it would make it easier to read, I suspect.
> diff --git a/protocol-caps.c b/protocol-caps.c
> index bbde91810a..f290e3cca2 100644
> --- a/protocol-caps.c
> +++ b/protocol-caps.c
> @@ -8,6 +8,9 @@
> #include "object-store.h"
> #include "string-list.h"
> #include "strbuf.h"
> +#include "config.h"
> +
> +static int advertise_object_info = -1;
It strikes me somewhat odd that this is the _first_ such capability
that needs a variable here to control if it is advertised or not
depending on the configuration file settings.
In other words, "Why doesn't transfer.advertiseSID also have a
similar thing in this file?" should be the first question that comes
to any reviewers' mind.
Perhaps we just invented a better way to handle such conditional
capability, in which case we may want to port the existing ones that
do not use the same arrangement over to the new way. Even though it
would be outside the scope of the series, it would be a good way to
make sure we won't accumulate too much technical debt to leave some
hint somewhere near here or in protoco-caps.h where this patch
touches. I cannot quite tell what is going on.
> int object_info_advertise(struct repository *r, struct strbuf *value)
> {
> if (advertise_object_info == -1 &&
> git_config_get_bool("transfer.advertiseObjectInfo", &advertise_object_info))
> advertise_object_info = 0;
> return advertise_object_info;
> }
"If unset(yet), try to get_bool() and if it fails, set it to 0"
So advertise_object_info becomes true only if transfer.advertiseObjectInfo
is set to true?
It is inconsistent with what we read in the documentation, isn't it?
> diff --git a/protocol-caps.h b/protocol-caps.h
> index 15c4550360..36f7a46b37 100644
> --- a/protocol-caps.h
> +++ b/protocol-caps.h
> @@ -5,4 +5,10 @@ struct repository;
> struct packet_reader;
> int cap_object_info(struct repository *r, struct packet_reader *request);
>
> +/*
> + * Advertises object-info capability if "objectinfo.advertise" is either
> + * set to true or not set
> + */
> +int object_info_advertise(struct repository *r, struct strbuf *value);
> +
> #endif /* PROTOCOL_CAPS_H */
> diff --git a/serve.c b/serve.c
> index b3fe9b5126..fb4ad367a7 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -133,7 +133,7 @@ static struct protocol_capability capabilities[] = {
> },
> {
> .name = "object-info",
> - .advertise = always_advertise,
> + .advertise = object_info_advertise,
> .command = cap_object_info,
> },
> };
This is a good step to add a test to see if the server does honor
the (1) lack of (2) true set to and (3) false set to the
configuration variable and sends (or does not send) the capability.
