> 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.
It doesn't exist in protocol-caps.c, but instead in serve.c. Would be
a good discussion for whether the advertise function should be serve.c
or protocol-caps.c
> "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?
This was the bug I introduced and will reroll
On Tue, Mar 29, 2022 at 3:34 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> 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.
