Re: [PATCH 1/3] reflog: libify delete reflog function and helpers

[Taylor Blau <me@ttaylorr.com>]

On Sat, Feb 19, 2022 at 03:53:14AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Feb 18 2022, Junio C Hamano wrote:
>
> > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> diff --git a/reflog.h b/reflog.h
> >> new file mode 100644
> >> index 00000000000..e4a8a104f45
> >> --- /dev/null
> >> +++ b/reflog.h
> >> @@ -0,0 +1,49 @@
> >> +#ifndef REFLOG_H
> >> +#define REFLOG_H
> >> +
> >> +#include "cache.h"
> >> +#include "commit.h"
> >> +
> >> +/* Remember to update object flag allocation in object.h */
> >> +#define INCOMPLETE	(1u<<10)
> >> +#define STUDYING	(1u<<11)
> >> +#define REACHABLE	(1u<<12)
> >
> > These names were unique enough back when they were part of internal
> > implementation detail inside builtin/reflog.c but it no longer is in
> > the scope it is visible.  builtin/stash.c (and whatever other things
> > that may want to deal with reflogs in the future) is about stash
> > entries and wants to have a way to differentiate these symbols from
> > others and hint that these are about reflog operation.
> >
> > Perhaps prefix them with REFLOG_ or something?
> >
> > Or, do we even NEED to expose these bits outside the implementation
> > of reflog.c?  If these three constants are used ONLY by reflog.c and
> > not by builtin/reflog.c (or builtin/stash.c), then moving them to
> > where they are used, i.e. in reflog.c, without exposing them to
> > others in reflog.h, would be a far better thing to do.  That way,
> > they can stay with their original name, without having to add a
> > differentiating prefix.
>
> No objection to this, but FWIW the general pattern for these object.h
> flags is to use these un-prefixed names:
>
>      git grep -A20 'Remember to update object flag allocation' | grep define
>
> To be fair the only one that's really comparable is the revision.h ones,
> but those are very non-namespace-y, with names like SEEN, ADDED, SHOWN
> etc.
>
> I'd be fine with just leaving these as-is, especially with compilers
> warning about macro re-definitions.

I think I have a mild preference to avoid polluting the set of macros
with something like INCOMPLETE outside of reflog.c's compilation unit
when possible. Though I don't really feel strongly either way.

In any case, let's make sure to update object.h's flag allocation table,
which as of this patch still indicates that these bits belong to
"builtin/reflog.c" (instead of "reflog.h" or "reflog.c", depending on
what the outcome of this discussion is).

Thanks,
Taylor