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