Re: Problems with 592fc5b349

From: Elijah Newren <newren@gmail.com>
Date: Jun 1, 2023, 14:33 UTC
Message-ID: <CABPp-BHKR5GP2NUFWDMSw-Pnra+yGP0kYAiwu-iWgtu66p-1RQ@mail.gmail.com>
In-Reply-To: <CAOO-Oz2ua31xDOA9hdE-mMx3qwctDHK6Tu6AKdGc1_beuJMkwA@mail.gmail.com>

On Thu, Jun 1, 2023 at 6:47 AM Alejandro R. Sedeño <asedeno@mit.edu> wrote:

Show 12 quoted lines

>
> 592fc5b3495bf4ff17252d31109f1d9c0134684b moved backup definitions of
>
>   #define DT_
>
> from cache.h to dir.h, but did not include dir.h in cache.h despite those
> #defines being used there. Easy fix, `#include "dir.h"` in cache.h,
> which I'd submit as a patch, but then name-hash.c, which includes
> cache.h, which would now include dir.h, ends up with two definitions
> of `struct dir_entry`.
>
> Suggestions?

Oh, interesting; none of our platform testing caught this. After a little digging, I'm guessing you're on cygwin < 1.7? However, I'm still surprised you noticed, on any platform. The only use of the DT_* defines in cache.h is in the inline function ce_to_dtype(). The only places ce_to_dtype() is used are in (1) unpack-trees.c (which includes both cache.h and dir.h) and (2) builtin/ls-files.c (which also includes both cache.h and dir.h). So, as far as I can tell, this can't cause compilation issues anywhere. How did you find this?

In commits in follow-on series, I moved this inline function to a new header, read-cache.h. name-cache.c does not end up including that header, so we could add a #include "dir.h" directive to read-cache.h.

An alternative fix, if you need something for v2.41.0 (am I guessing correctly that you tried out v2.41.0 right after it's release and that's when you found this?), would be to move the DT_ defines from dir.h to statinfo.h (a header included by both dir.h and cache.h). Or perhaps another fix is to stop having two things in the codebase named "struct dir_entry", since it's bound to cause confusion for humans if not also be a lurking timebomb for some future code file that needs access to both. But I still don't understand why any suggestions are needed for an immediate fix, since all users of ce_to_dtype() should have the necessary headers. Is there an issue where "inline" is ignored, and this function is being defined & compiled for every file that includes cache.h, and then the linker removes the duplicates or something?