Re: [PATCH v6 16/30] compat/fsmonitor/fsm-listen-darwin: add MacOS header files for FSEvent

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Jeff Hostetler <git@jeffhostetler.com>,
	Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Bagas Sanjaya <bagasdotme@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Tao Klerks <tao@klerks.biz>,
	Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH v6 16/30] compat/fsmonitor/fsm-listen-darwin: add MacOS header files for FSEvent
Date: Thu, 10 Mar 2022 15:42:00 +0100	[thread overview]
Message-ID: <220310.86a6dxkgjr.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2203101445360.357@tvgsbejvaqbjf.bet>

On Thu, Mar 10 2022, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Wed, 9 Mar 2022, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Mar 08 2022, Jeff Hostetler wrote:
>>
>> > On 3/7/22 5:37 AM, Ævar Arnfjörð Bjarmason wrote:
>> >> On Tue, Mar 01 2022, Jeff Hostetler via GitGitGadget wrote:
>> >>
>> >>> From: Jeff Hostetler <jeffhost@microsoft.com>
>> >>> [...]
>> > [...]
>> >>
>> >>> +#else
>> >>> +/*
>> >>> + * Let Apple's headers declare `isalnum()` first, before
>> >>> + * Git's headers override it via a constant
>> >>> + */
>> >>
>> >>
>> >>> +#include <string.h>
>> >>> +#include <CoreFoundation/CoreFoundation.h>
>> >>> +#include <CoreServices/CoreServices.h>
>> >>> +#endif
>> >>
>> >> In cache.h which you'rejust about to include we don't include
>> >> string.h, but we do in git-compat-util.h, but that one includes
>> >> string.h before doing those overrides.
>> >>
>> >> This either isn't needed, or really should be some addition to
>> >> git-compat-util.h instead. I.e. if we've missed some edge case with
>> >> string.h and ctype.h on OSX we should handle that in git-compat-util.h
>> >> rather than <some other file/header> needing a portability workaround.
>> >
>> > [...]
>> >
>> > While it may not (now at least) be necessary, it's not doing
>> > any harm, so I'd rather leave it and not interrupt things.
>> > We can always revisit it later if we want.
>>
>> In terms of figuring out some mysterious portability issue, I think the
>> right time is now rather than later.
>
> I do not see that.
>
> In FSMonitor, this was clearly a problem that needed to be solved (and if
> you try to compile on an older macOS, you will run into those problems
> again).

So you can reproduce an issue where removing the "#include <string.h>"
from compat/fsmonitor/fsm-listen-darwin.c has an effect? Does swaping it
for "ctype.h" also solve that issue?

I was asking why the non-obvious portability hack was needed, and it
seems Jeff suggested it might not be upthread in
<aa6276f9-8d10-22f9-bfc0-2aa718d002e1@jeffhostetler.com>, but here you
seem to have a reproduction of in being needed, without more of the
relevant details (e.g. what OSX version(s))?

> If you are talking about the mysterious portability issue with an eye on
> `git-compat-util.h`, well... you can successfully compile Git's source
> code without this hack in `git-compat-util.h`. That's why the hack is not
> needed. Problem solved. Actually, there was not even a problem.

Do you mean we don't need the ctype.h overrides in git-compat-util.h at
all? I haven't looked into it, but needing to

>> I.e. now this doesn't have anyone relying on it, so we can easily
>> (re)discover whatever issue this was trying to solve.
>>
>> Whereas anyone who'd need to figure out why we include string.h on OSX
>> early in this case later would be left with this otherwise dead-end
>> thread, and a change at that point would possibly break existing code...
>
> Anyone who would need to figure out why we `#include` this header early
> would read the comment about `isalnum()`, I would hope, and understand
> that there are circumstances when Git's `isalnum()` macro interferes with
> Apple's, and that this `#include` order addresses that problem.
>
> They might even get to the point where they find
> https://github.com/dscho/git/commit/0f89c726a1912dce2bdab14aff8ebfec8550340d,
> maybe even the "original original" commit at
> https://github.com/kewillford/git/commit/d11ee4698c63347f40a8993ab86ee4e97f695d9b,
> which was a still-experimental version of the macOS backend, and where the
> `#include` order clearly mattered, else why would Kevin have bothered.
>
> Further, I strongly suspect that it had something to do with
> `CoreFoundation.h` or with `CoreServices.h` being `#include`d, and if you
> care to check the code above the quoted lines, you will see that you
> cannot even `#include` those headers using GCC, it only works with clang:
> https://github.com/jeffhostetler/git/commit/cdef9730b3f93a6f0f98d68ffb81bcb89d6e698e#diff-4e865160016fe490b80ad11273a10daca8bc412a75f2da4c6b08fb9e5e3b5e18R3
>
> At this stage, anybody investigating this issue who is a little bit like
> me would then be a bit bored with the investigation because there is
> actually no breakage here, just a curious `#include` order, and nothing
> else. So they might then drop it and move along.

My implicit observation upthread is that those sorts of details would
ideally be included in a comment or the commit message. I.e. I didn't
quite see why it was needed, and neither could the person submitting the
patch series when asked.

Sure, it's a minor issue, but patch review is also meant to uncover such
small issues.