Re: [PATCH v3 2/3] transfer.advertiseObjectInfo: add object-info config

[Calvin Wan <calvinwan@google.com>]

> 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@pobox.com> wrote:
>
> Calvin Wan <calvinwan@google.com> 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.