On 3/10/22 9:42 AM, Ævar Arnfjörð Bjarmason wrote:
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@xxxxxxxxxxxxx>
[...]
[...]
+#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@xxxxxxxxxxxxxxxxx>, 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.
There are two independent issues here.
(1) compiling something that includes <CoreServices.h> with GCC.
(2) the need for the #include <string.h> when compiling with clang.
To address (1), we've #ifdef'd the top of the file and insert just
the essential typedefs and prototypes. (I'll pull them into a separate
local header file as you suggested earlier to make that easier to see.)
But otherwise, GCC is not an issue.
WRT (2) I have tried clang-11 on macOS 10.15 and clang-13 on 11.6 both
with and without the <string.h> and it doesn't matter. Everything
compiles and t7527 passes in all [2x2] cases. So I have to assume that
something has changed in the Apple/clang SDK or runtime libraries or
our source code in that single source file in the ~2 years since Kevin
needed to add it. I do not have access to an older Mac (Apple makes it
hard to test back-compat with older OS's), so I cannot reproduce the
error. But I don't doubt that there was an error at one point -- I just
don't know what it was. So that's my context for saying that I don't
think it is needed now, but I was willing to carry it forward in case
it is still helpful for people with older Macs. FWIW, it really seems
pretty isolated and trivial and would only affect code in this single
source file -- which from a quick scan, doesn't actually reference any
of the functions in <string.h>, so there shouldn't be any need to think
about git-compat-util.h or ctype.h, right?
Jeff
