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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 29, 2022, 23:09 UTC
Message-ID: <YkORvY3duS7dQRRY@nand.local>
In-Reply-To: <CAFySSZB5aTAsYn=Oqd_hUCQu+X1+GCgpvscLYqk+qzGQc3sAVw@mail.gmail.com>

On Tue, Mar 29, 2022 at 03:48:28PM -0700, Calvin Wan wrote:

Show 11 quoted lines

> > 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

Most of the others are defined statically within serve.c, but there are a couple of exceptions:

  - `ls_refs_advertise()` is defined in ls-refs.c, and
  - `upload_pack_advertise()` is defined in upload-pack.c.

Even though cap_object_info() is defined in protocol-caps.c, I think we should probably keep the definition of `object_info_advertise()` local to serve.c.

If there were a object-info.c, it would make sense to put it there, but since there isn't, it makes more sense to keep it local to serve.c, which avoids us having to declare the function in a header file.

Thanks, Taylor