On Thu, Jan 17, 2019 at 11:27:11AM -0500, Jeff King wrote:
> -- >8 --
> Subject: [PATCH] add: use separate ADD_CACHE_RENORMALIZE flag
>
> Commit 9472935d81 (add: introduce "--renormalize", 2017-11-16) taught
> git-add to pass HASH_RENORMALIZE to add_to_index(), which then passes
> the flag along to index_path(). However, the flags taken by
> add_to_index() and the ones taken by index_path() are distinct
> namespaces. We cannot take HASH_* flags in add_to_index(), because they
> overlap with the ADD_CACHE_* flags we already take (in this case,
> HASH_RENORMALIZE conflicts with ADD_CACHE_IGNORE_ERRORS).
>
> We can solve this by adding a new ADD_CACHE_RENORMALIZE flag, and using
> it to set HASH_RENORMALIZE within add_to_index(). In order to make it
> clear that these two flags come from distinct sets, let's also change
> the name "newflags" in the function to "hash_flags".
By the way, I wondered if there was a good way for the compiler to help
us find an error like this. There's no type-checking here, since all of
the flags are "int", with the values #define macros. Could enums give us
better safety?
My experiments suggest no, since the compiler is pretty loose about what
it will allow, even with -Wenum-compare. In particular, I think it's
happy to allow bitwise-AND even against tags from other enums. But if
somebody can figure out a way to make it work, I'm all ears. :)
A more drastic option is to replace the flag int with a struct
containing a bitfield. I.e., something like:
struct add_cache_flags {
unsigned ignore_errors : 1;
unsigned renormalize : 1;
... etc ...
};
That gives us real type safety and eliminates the need to manually
assign numbers to each flag. But there are downsides:
- it's syntactically more awkward to modify the flags. I.e.:
foo(flags | BAR);
becomes:
struct flags new_flags = old_flags;
new_flags.bar = 1;
foo(&new_flags);
There might be some C99-isms that can help us out (though I suspect
not completely -- even if we can use anonymous structs, I don't
think there's such a thing as "initialize this anonymous struct and
then modify these fields").
- there's no grouping, so you can't mask off certain values
So I dunno. Maybe that road is too painful, and we're stuck with what C
gives us.
-Peff
