Re: git add —ignore-errors causes --renormalize

From: Jeff King <peff@peff.net>
Date: Jan 17, 2019, 16:45 UTC
Message-ID: <20190117164507.GA27819@sigill.intra.peff.net>
In-Reply-To: <20190117162711.GA7935@sigill.intra.peff.net>

On Thu, Jan 17, 2019 at 11:27:11AM -0500, Jeff King wrote:

Show 15 quoted lines

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