Re: [PATCH 1/7] for_each_*_object: store flag definitions in a single location

From: Stefan Beller <sbeller@google.com>
Date: Aug 10, 2018, 23:27 UTC
Message-ID: <CAGZ79kYStPNCodJBqZ-ELh2oEhEyQFFjr0SVn0Bn+gtAspbZiA@mail.gmail.com>
In-Reply-To: <20180810230906.GA19875@sigill.intra.peff.net>

On Fri, Aug 10, 2018 at 4:09 PM Jeff King <peff@peff.net> wrote:

Show 11 quoted lines

>
> These flags were split between cache.h and packfile.h,
> because some of the flags apply only to packs. However, they
> share a single numeric namespace, since both are respected
> for the packed variant. Let's make sure they're defined
> together so that nobody accidentally adds a new flag in one
> location that duplicates the other.
>
> While we're here, let's also put them in an enum (which
> helps debugger visibility) and use "(1<<n)" rather than
> counting powers of 2 manually.

All good, but ...

>  cache.h    | 13 ++++++++++++-
>  packfile.h |  8 ++------
>  2 files changed, 14 insertions(+), 7 deletions(-)

rubs me the wrong way. ;-)

cache.h is such a misnomer of a name, and a kitchen sink of a file in the Git project that in an ideal world it would be way smaller and contain only things related to some caching related code.

I would suggest object.h or object-store.h instead. Probably the object-store as that will be the only external exposure and hopefully we'd get the objects in a similar shape as the refs subsystem eventually?

I might be biased by commits such as 4f39cd821d1 (pack: move pack name-related functions, 2017-08-18)