| Message ID | pull.1352.v2.git.git.1665783944.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fsmonitor: Implement fsmonitor for Linux | expand |
"Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes: > Goal is to deliver fsmonitor for Linux that is on par with fsmonitor for > Windows and Mac OS. > > This patch set builds upon previous work for done for Windows and Mac OS > (first 6 patches) to implement a fsmonitor back-end for Linux based on the > Linux inotify API. Again, the first six patches are a part of what is queued as ed/fsmonitor-on-networked-macos that is now in 'next' but lacks a fix-up commit from Jeff King. I understand that it might not be easy/possible (e.g. perhaps it is a limitation of GGG?), but I really prefer not to see them re-posted as part of this series, as I have to apply them and make sure there are no changes from the last one before discarding them. Anyway, thanks for an update. Will requeue.
> -----Original Message----- > From: Junio C Hamano <gitster@pobox.com> > Sent: Friday, October 14, 2022 7:33 PM > To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com> > Cc: git@vger.kernel.org; Eric Sunshine <sunshine@sunshineco.com>; Ævar > Arnfjörð Bjarmason <avarab@gmail.com>; Eric DeCosta > <edecosta@mathworks.com> > Subject: Re: [PATCH v2 00/12] fsmonitor: Implement fsmonitor for Linux > > "Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > Goal is to deliver fsmonitor for Linux that is on par with fsmonitor > > for Windows and Mac OS. > > > > This patch set builds upon previous work for done for Windows and Mac > > OS (first 6 patches) to implement a fsmonitor back-end for Linux based > > on the Linux inotify API. > > Again, the first six patches are a part of what is queued as ed/fsmonitor-on- > networked-macos that is now in 'next' but lacks a fix-up commit from Jeff > King. > > I understand that it might not be easy/possible (e.g. perhaps it is a limitation > of GGG?), but I really prefer not to see them re-posted as part of this series, > as I have to apply them and make sure there are no changes from the last > one before discarding them. > > Anyway, thanks for an update. Will requeue. I looks like I can use GGG with the next branch, but I will have to open a new PR (and close the existing one). -Eric
At $DAYJOB, we observed that this topic breaks MacOS builds with sha1dc:
$ make NO_APPLE_COMMON_CRYPTO=1 DC_SHA1=1 NO_OPENSSL=1 compat/fsmonitor/fsm-ipc-darwin.o
CC compat/fsmonitor/fsm-ipc-darwin.o
compat/fsmonitor/fsm-ipc-darwin.c:13:2: error: unknown type name 'SHA_CTX'; did you mean 'SHA1_CTX'?
SHA_CTX sha1ctx;
^~~~~~~
SHA1_CTX
./sha1dc/sha1.h:55:3: note: 'SHA1_CTX' declared here
} SHA1_CTX;
^
compat/fsmonitor/fsm-ipc-darwin.c:16:21: error: use of undeclared identifier 'SHA_DIGEST_LENGTH'
unsigned char hash[SHA_DIGEST_LENGTH];
^
compat/fsmonitor/fsm-ipc-darwin.c:31:2: error: implicit declaration of function 'SHA1_Init' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
SHA1_Init(&sha1ctx);
^
compat/fsmonitor/fsm-ipc-darwin.c:31:2: note: did you mean 'SHA1DCInit'?
./sha1dc/sha1.h:58:6: note: 'SHA1DCInit' declared here
void SHA1DCInit(SHA1_CTX*);
^
compat/fsmonitor/fsm-ipc-darwin.c:32:2: error: implicit declaration of function 'SHA1_Update' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
SHA1_Update(&sha1ctx, r->worktree, strlen(r->worktree));
^
compat/fsmonitor/fsm-ipc-darwin.c:32:2: note: did you mean 'SHA1DCUpdate'?
./sha1dc/sha1.h:96:6: note: 'SHA1DCUpdate' declared here
void SHA1DCUpdate(SHA1_CTX*, const char*, size_t);
^
compat/fsmonitor/fsm-ipc-darwin.c:33:2: error: implicit declaration of function 'SHA1_Final' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
SHA1_Final(hash, &sha1ctx);
^
compat/fsmonitor/fsm-ipc-darwin.c:33:2: note: did you mean 'SHA1DCFinal'?
./sha1dc/sha1.h:100:6: note: 'SHA1DCFinal' declared here
int SHA1DCFinal(unsigned char[20], SHA1_CTX*);
^
5 errors generated.
make: *** [compat/fsmonitor/fsm-ipc-darwin.o] Error 1
Without NO_OPENSSL, this still fails, but with slightly different error
messages.
$ make NO_APPLE_COMMON_CRYPTO=1 DC_SHA1=1 compat/fsmonitor/fsm-ipc-darwin.o
CC compat/fsmonitor/fsm-ipc-darwin.o
compat/fsmonitor/fsm-ipc-darwin.c:31:2: error: 'SHA1_Init' is deprecated [-Werror,-Wdeprecated-declarations]
SHA1_Init(&sha1ctx);
^
/opt/local/include/openssl/sha.h:49:1: note: 'SHA1_Init' has been explicitly marked deprecated here
OSSL_DEPRECATEDIN_3_0 int SHA1_Init(SHA_CTX *c);
^
/opt/local/include/openssl/macros.h:182:49: note: expanded from macro 'OSSL_DEPRECATEDIN_3_0'
# define OSSL_DEPRECATEDIN_3_0 OSSL_DEPRECATED(3.0)
^
/opt/local/include/openssl/macros.h:62:52: note: expanded from macro 'OSSL_DEPRECATED'
# define OSSL_DEPRECATED(since) __attribute__((deprecated))
^
compat/fsmonitor/fsm-ipc-darwin.c:32:2: error: 'SHA1_Update' is deprecated [-Werror,-Wdeprecated-declarations]
SHA1_Update(&sha1ctx, r->worktree, strlen(r->worktree));
^
/opt/local/include/openssl/sha.h:50:1: note: 'SHA1_Update' has been explicitly marked deprecated here
OSSL_DEPRECATEDIN_3_0 int SHA1_Update(SHA_CTX *c, const void *data, size_t len);
^
/opt/local/include/openssl/macros.h:182:49: note: expanded from macro 'OSSL_DEPRECATEDIN_3_0'
# define OSSL_DEPRECATEDIN_3_0 OSSL_DEPRECATED(3.0)
^
/opt/local/include/openssl/macros.h:62:52: note: expanded from macro 'OSSL_DEPRECATED'
# define OSSL_DEPRECATED(since) __attribute__((deprecated))
^
compat/fsmonitor/fsm-ipc-darwin.c:33:2: error: 'SHA1_Final' is deprecated [-Werror,-Wdeprecated-declarations]
SHA1_Final(hash, &sha1ctx);
^
/opt/local/include/openssl/sha.h:51:1: note: 'SHA1_Final' has been explicitly marked deprecated here
OSSL_DEPRECATEDIN_3_0 int SHA1_Final(unsigned char *md, SHA_CTX *c);
^
/opt/local/include/openssl/macros.h:182:49: note: expanded from macro 'OSSL_DEPRECATEDIN_3_0'
# define OSSL_DEPRECATEDIN_3_0 OSSL_DEPRECATED(3.0)
^
/opt/local/include/openssl/macros.h:62:52: note: expanded from macro 'OSSL_DEPRECATED'
# define OSSL_DEPRECATED(since) __attribute__((deprecated))
^
3 errors generated.
make: *** [compat/fsmonitor/fsm-ipc-darwin.o] Error 1
"Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Goal is to deliver fsmonitor for Linux that is on par with fsmonitor for
> Windows and Mac OS.
>
> This patch set builds upon previous work for done for Windows and Mac OS
> (first 6 patches) to implement a fsmonitor back-end for Linux based on the
> Linux inotify API. inotify differs significantly from the equivalent Windows
> and Mac OS APIs in that a watch must be registered for every directory of
> interest (rather than a singular watch at the root of the directory tree)
> and special care must be taken to handle directory renames correctly.
>
> More information about inotify:
> https://man7.org/linux/man-pages/man7/inotify.7.html
>
> v1 differs from v0:
>
> * Code review feedback
> * Update how and which code can be shared between Mac OS and Linux
> * Increase polling frequency to every 1ms (matches Mac OS)
> * Updates to t7527 to improve test stability
>
Eric DeCosta <edecosta@mathworks.com> writes: >> I understand that it might not be easy/possible (e.g. perhaps it is a limitation >> of GGG?), but I really prefer not to see them re-posted as part of this series, >> as I have to apply them and make sure there are no changes from the last >> one before discarding them. >> >> Anyway, thanks for an update. Will requeue. > > I looks like I can use GGG with the next branch, but I will have > to open a new PR (and close the existing one). Please don't. The macOS topic (thanks for working on it, by the way) is about to graduate to 'master', and the situation would probably "improve" anyway when it happens, I hope. Thanks.
Glen Choo <chooglen@google.com> writes:
> At $DAYJOB, we observed that this topic breaks MacOS builds with sha1dc:
Thanks for a report.
How dissapointing. The thing is that the topic has been in 'next'
since the 11th (i.e. early last week), and I know that you guys rely
on the tip of 'next' in working order to cut your internal releases,
but we did not hear about this until now. What makes it taste even
worse is that nobody else caught this, even though we seem to have a
couple of macOS jobs at GitHub Actions CI, there we didn't see any
breakage related to this.
Possible action items:
* See what configurations our two macOS jobs are using. If neither
is using sha1dc, I would say that is criminal [*] and at least
one of them should be updated to do so right away.
* The "two macOS CI jobs sharing too many configuration knobs" may
not be limited to just SHA-1 implementation, but unlike Linux
builds and tests, we may have similar "monoculture" issue in our
macOS CI builds. Those users, who depend on macOS port being
healthy, should help identify unnecessary overlaps between the
two, and more importantly, make sure we have CI builds with
configuration similar to what they actually use.
* Adding a few build-only-without-tests CI jobs also might help.
* Those who depend on working macOS port, especially those with
corporate backing who choose to use configurations that are
different from what we have CI builds for, are requested to
arrange a more frequent build test to catch a problem like this
much earlier.
Anything else I forgot that we can do to improve the situation? I
personally do not use macOS, I am not interested in using one, but
I do value those who choose to use macOS have happy git working on
their platform, so the stakeholders need to chip in.
Thanks.
[Footnote]
* Until the world migrates over to SHA-256, the collision detecting
SHA-1 implementation is what we must use unless there is a strong
reason not to. If we are not testing something that ought to be
the default, we are not doing a very good job.
Hi Eric, On Mon, 17 Oct 2022, Eric DeCosta wrote: > > -----Original Message----- > > From: Junio C Hamano <gitster@pobox.com> > > Sent: Friday, October 14, 2022 7:33 PM > > To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com> > > Cc: git@vger.kernel.org; Eric Sunshine <sunshine@sunshineco.com>; Ævar > > Arnfjörð Bjarmason <avarab@gmail.com>; Eric DeCosta > > <edecosta@mathworks.com> > > Subject: Re: [PATCH v2 00/12] fsmonitor: Implement fsmonitor for Linux > > > > "Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > > > Goal is to deliver fsmonitor for Linux that is on par with fsmonitor > > > for Windows and Mac OS. > > > > > > This patch set builds upon previous work for done for Windows and Mac > > > OS (first 6 patches) to implement a fsmonitor back-end for Linux based > > > on the Linux inotify API. > > > > Again, the first six patches are a part of what is queued as ed/fsmonitor-on- > > networked-macos that is now in 'next' but lacks a fix-up commit from Jeff > > King. > > > > I understand that it might not be easy/possible (e.g. perhaps it is a limitation > > of GGG?), but I really prefer not to see them re-posted as part of this series, > > as I have to apply them and make sure there are no changes from the last > > one before discarding them. GitGitGadget mirrors all branches from gitster/git to gitgitgadget/git, so if you open a PR in the latter repository, you can use all of those branches as targets. But this PR is in git/git, which does not offer that. > > Anyway, thanks for an update. Will requeue. > > I looks like I can use GGG with the next branch, but I will have to open a new PR (and close the existing one). No, you do not have to open a new PR for that. You just hit the Edit button on the top (as if you wanted to change the title) and then select the target ("base") branch in the drop-down box under the title. Ciao, Dscho
Cc-ed Johannes, who would know a lot more about CI than I do. Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> writes: > >> At $DAYJOB, we observed that this topic breaks MacOS builds with sha1dc: > > Thanks for a report. > > How dissapointing. The thing is that the topic has been in 'next' > since the 11th (i.e. early last week), and I know that you guys rely > on the tip of 'next' in working order to cut your internal releases, > but we did not hear about this until now. Yes. Unfortunately, we (Google's Git team) release on a weekly cadence; we merge on Fridays and build on Mondays (PST), which makes this timing extremely unfortunate. > Possible action items: > > * See what configurations our two macOS jobs are using. If neither > is using sha1dc, I would say that is criminal [*] and at least > one of them should be updated to do so right away. I'm not too familiar with the CI, but I took a quick peek at ci/lib.sh and noticed that none of the jobs build with sha1dc, not even the Linux or Windows ones, so.. > * The "two macOS CI jobs sharing too many configuration knobs" may > not be limited to just SHA-1 implementation, but unlike Linux > builds and tests, we may have similar "monoculture" issue in our > macOS CI builds. Those users, who depend on macOS port being > healthy, should help identify unnecessary overlaps between the > two, and more importantly, make sure we have CI builds with > configuration similar to what they actually use. I don't think this is a macOS-specific issue; our CI just doesn't do a good job with testing various build configurations. > * Adding a few build-only-without-tests CI jobs also might help. This sounds like the way to go IMO. It might be too expensive to run the full test suite on every build configuration, but build-without-test might be ok. > * Those who depend on working macOS port, especially those with > corporate backing who choose to use configurations that are > different from what we have CI builds for, are requested to > arrange a more frequent build test to catch a problem like this > much earlier. I wished we had caught it sooner too. The folks here generally agree that our weekly release cycle is not ideal for reasons such as this. Hopefully this is good motivation to move that work forward, though I can't promise anything right now. > Anything else I forgot that we can do to improve the situation? I > personally do not use macOS, I am not interested in using one, but > I do value those who choose to use macOS have happy git working on > their platform, so the stakeholders need to chip in. There's nothing else I can think of at the moment. Thanks for your patience and for moving the conversation along. > > Thanks. > > > [Footnote] > > * Until the world migrates over to SHA-256, the collision detecting > SHA-1 implementation is what we must use unless there is a strong > reason not to. If we are not testing something that ought to be > the default, we are not doing a very good job.
Glen Choo <chooglen@google.com> writes: > I wished we had caught it sooner too. The folks here generally agree > that our weekly release cycle is not ideal for reasons such as this. > Hopefully this is good motivation to move that work forward, though I > can't promise anything right now. It is perfectly OK to have an automated trial build job that runs more frequently than your weekly release cycle, though. It should usually yield only a single bit of usable information (e.g. "there is no 'does not even build from the source' issue in upstream") that may give you assurance (e.g. "if we maintain the course, the next real build for release would hopefully go smoothly"), but when it breaks, you have more time to react. Thanks.
On Mon, Oct 17 2022, Junio C Hamano wrote: > Glen Choo <chooglen@google.com> writes: > >> At $DAYJOB, we observed that this topic breaks MacOS builds with sha1dc: > > Thanks for a report. > > How dissapointing. The thing is that the topic has been in 'next' > since the 11th (i.e. early last week), and I know that you guys rely > on the tip of 'next' in working order to cut your internal releases, > but we did not hear about this until now. What makes it taste even > worse is that nobody else caught this, even though we seem to have a > couple of macOS jobs at GitHub Actions CI, there we didn't see any > breakage related to this. FWIW I see you caught it on the 9th in https://lore.kernel.org/git/xmqqh70c62w0.fsf@gitster.g/, but then the base topic was merged down on the 17th. > Possible action items: > > * See what configurations our two macOS jobs are using. If neither > is using sha1dc, I would say that is criminal [*] and at least > one of them should be updated to do so right away. I submitted a v2 of my series to finally make OSX use SHA1DC by default: https://lore.kernel.org/git/cover-v2-0.4-00000000000-20221019T010222Z-avarab@gmail.com/ As part of that we'll CI that & the current "apple common crypto" implementations, currently we just do the latter, which is why we didn't catch this. We could follow-up and CI the OpenSSL one too, but that was a larger change, so I punted on it. In any case, https://lore.kernel.org/git/patch-v2-1.4-392fabdb456-20221019T010222Z-avarab@gmail.com/ in that series un-breaks master, maybe you're interested in peeling it off & fast-tracking it? I didn't submit it separately because I don't know how much of a rush we're in there (given that CI isn't broken).
On Tue, Oct 18 2022, Glen Choo wrote: > Cc-ed Johannes, who would know a lot more about CI than I do. > > Junio C Hamano <gitster@pobox.com> writes: > >> Glen Choo <chooglen@google.com> writes: >> >>> At $DAYJOB, we observed that this topic breaks MacOS builds with sha1dc: >> >> Thanks for a report. >> >> How dissapointing. The thing is that the topic has been in 'next' >> since the 11th (i.e. early last week), and I know that you guys rely >> on the tip of 'next' in working order to cut your internal releases, >> but we did not hear about this until now. > > Yes. Unfortunately, we (Google's Git team) release on a weekly cadence; > we merge on Fridays and build on Mondays (PST), which makes this timing > extremely unfortunate. > >> Possible action items: >> >> * See what configurations our two macOS jobs are using. If neither >> is using sha1dc, I would say that is criminal [*] and at least >> one of them should be updated to do so right away. > > I'm not too familiar with the CI, but I took a quick peek at ci/lib.sh > and noticed that none of the jobs build with sha1dc, not even the Linux > or Windows ones, so.. All of our jobs except the OSX one build with SHA1_DC, because it's the default. Per my just-sent https://lore.kernel.org/git/cover-v2-0.4-00000000000-20221019T010222Z-avarab@gmail.com/ the blind spot has been lack fo SHA1_DC on OSX, for others it's the reverse, we don't test e.g. BLK_SHA1. In practice we've been catching SHA-implementation specific code early because the OSX implementation was different, but in this case it's OSX-only code, so it only supported the Apple Common Crypto backend. >> * The "two macOS CI jobs sharing too many configuration knobs" may >> not be limited to just SHA-1 implementation, but unlike Linux >> builds and tests, we may have similar "monoculture" issue in our >> macOS CI builds. Those users, who depend on macOS port being >> healthy, should help identify unnecessary overlaps between the >> two, and more importantly, make sure we have CI builds with >> configuration similar to what they actually use. > > I don't think this is a macOS-specific issue; our CI just doesn't do a > good job with testing various build configurations. Yes, definitely. >> * Adding a few build-only-without-tests CI jobs also might help. > > This sounds like the way to go IMO. It might be too expensive to run the > full test suite on every build configuration, but build-without-test > might be ok. Yes, there's not much point in running the full tests for some of these variants. A lot of it we can also get for free, e.g. we run some linux jobs with clang, some with gcc etc., we could also run one with BLK_SHA1, one with OPENSSL_SHA1 etc.
On Tue, Oct 18, 2022 at 9:22 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Tue, Oct 18 2022, Glen Choo wrote: > > I'm not too familiar with the CI, but I took a quick peek at ci/lib.sh > > and noticed that none of the jobs build with sha1dc, not even the Linux > > or Windows ones, so.. > > All of our jobs except the OSX one build with SHA1_DC, because it's the > default. > > Per my just-sent > https://lore.kernel.org/git/cover-v2-0.4-00000000000-20221019T010222Z-avarab@gmail.com/ > the blind spot has been lack fo SHA1_DC on OSX, for others it's the > reverse, we don't test e.g. BLK_SHA1. > > In practice we've been catching SHA-implementation specific code early > because the OSX implementation was different, but in this case it's > OSX-only code, so it only supported the Apple Common Crypto backend. I don't know how germane it is to the current thread, but previous discussions[1,2,3,4] favored dropping use of Apple's Common Crypto altogether since it doesn't seem to buy us much (or anything) and is incomplete; it doesn't support all of the OpenSSL API Git uses. [1]: https://lore.kernel.org/git/CAPig+cTfMx_kwUAxBRHp6kNSOtXsdsv=odUQSRYVpV21DnRuvA@mail.gmail.com/ [2]: https://lore.kernel.org/git/CAMYxyaVQyVRQb-b0nVv412tMZ3rEnOfUPRakg2dEREg5_Ba5Ag@mail.gmail.com/T/ [3]: https://lore.kernel.org/git/20160102234923.GA14424@gmail.com/ [4]: https://lore.kernel.org/git/CAPig+cQ5kKAt2_RQnqT7Rn=uGmHV9VvxpQ+UgDPOj=D=pq6arg@mail.gmail.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Mon, Oct 17 2022, Junio C Hamano wrote: > >> Glen Choo <chooglen@google.com> writes: >> >>> At $DAYJOB, we observed that this topic breaks MacOS builds with sha1dc: >> >> Thanks for a report. >> >> How dissapointing. The thing is that the topic has been in 'next' >> since the 11th (i.e. early last week), and I know that you guys rely >> on the tip of 'next' in working order to cut your internal releases, >> but we did not hear about this until now. What makes it taste even >> worse is that nobody else caught this, even though we seem to have a >> couple of macOS jobs at GitHub Actions CI, there we didn't see any >> breakage related to this. > > FWIW I see you caught it on the 9th in > https://lore.kernel.org/git/xmqqh70c62w0.fsf@gitster.g/, but then the > base topic was merged down on the 17th. Heh, I already forgot I made that comment myself ;-) Thanks for reminding, and thanks for picking the single step to fast-track the fix.
Eric Sunshine <sunshine@sunshineco.com> writes: > I don't know how germane it is to the current thread, but previous > discussions[1,2,3,4] favored dropping use of Apple's Common Crypto > altogether since it doesn't seem to buy us much (or anything) and is > incomplete; it doesn't support all of the OpenSSL API Git uses. Yeah, that matches my recollection. Unless the situation has much changed, dropping it may not be a bad thing to do. But * Fixing the fsmonitor code so that it can also be used with things other than Common Crypto is the most urgent. The topic gave us a grave regression (those who used to successfully build Git can no longer build with their favourite configuration). * Updating the build procedure so that sha1dc is used by default everywhere is a good idea, but that is less urgent and should be done separately, preferrably long after the dust settles from the above. * Removing Common Crypto support may not be a bad idea, but that is even less urgent, unless the support burden is slowing us down or forcing us to settle on common set of features that is too limiting. Thanks.
On Tue, Oct 18 2022, Eric Sunshine wrote: > On Tue, Oct 18, 2022 at 9:22 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> On Tue, Oct 18 2022, Glen Choo wrote: >> > I'm not too familiar with the CI, but I took a quick peek at ci/lib.sh >> > and noticed that none of the jobs build with sha1dc, not even the Linux >> > or Windows ones, so.. >> >> All of our jobs except the OSX one build with SHA1_DC, because it's the >> default. >> >> Per my just-sent >> https://lore.kernel.org/git/cover-v2-0.4-00000000000-20221019T010222Z-avarab@gmail.com/ >> the blind spot has been lack fo SHA1_DC on OSX, for others it's the >> reverse, we don't test e.g. BLK_SHA1. >> >> In practice we've been catching SHA-implementation specific code early >> because the OSX implementation was different, but in this case it's >> OSX-only code, so it only supported the Apple Common Crypto backend. > > I don't know how germane it is to the current thread, but previous > discussions[1,2,3,4] favored dropping use of Apple's Common Crypto > altogether since it doesn't seem to buy us much (or anything) and is > incomplete; it doesn't support all of the OpenSSL API Git uses. > > [1]: https://lore.kernel.org/git/CAPig+cTfMx_kwUAxBRHp6kNSOtXsdsv=odUQSRYVpV21DnRuvA@mail.gmail.com/ > [2]: https://lore.kernel.org/git/CAMYxyaVQyVRQb-b0nVv412tMZ3rEnOfUPRakg2dEREg5_Ba5Ag@mail.gmail.com/T/ > [3]: https://lore.kernel.org/git/20160102234923.GA14424@gmail.com/ > [4]: https://lore.kernel.org/git/CAPig+cQ5kKAt2_RQnqT7Rn=uGmHV9VvxpQ+UgDPOj=D=pq6arg@mail.gmail.com/ Yeah, maybe. I'd be 100% OK with that happening, but I don't use OSX outside fo CI really, so I wanted to avoid scope creep to "non-SHA stuff we use OpenSSL/AppleCommonCrypto/whatever" for, in the cases where we can/do use OpenSSL/AppleCommonCrypto/whatever for SHA also. I.e. that's basically a question about what https://, git-imap-send etc. should be using if not vanilla OpenSSL & the like. But if you/someone can come up with a patch that confidently asserts that it's useless & drops it I'd be happy to either integrate it & submit it as part of this series if it makes things simpler, or alternatively re-roll on top of it. I'm just not confident in making that case myself, since I hardly use the platform, am not too familiar with the various concerns (aside from skimming the links above), and don't really have interest in pursuing that. OTOH if you do want to make the case for dropping Apple Common Crypto that would also presumably be much easier after this series, once we're past justifying the hurdle of "wait, we're switching the thing we use for SHA-1 by default, which we build practically everything in git on?" :) 1. https://lore.kernel.org/git/CAPig+cQ5kKAt2_RQnqT7Rn=uGmHV9VvxpQ+UgDPOj=D=pq6arg@mail.gmail.com/
On Wed, Oct 19, 2022 at 3:16 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Tue, Oct 18 2022, Eric Sunshine wrote: > > On Tue, Oct 18, 2022 at 9:22 PM Ævar Arnfjörð Bjarmason > > <avarab@gmail.com> wrote: > >> In practice we've been catching SHA-implementation specific code early > >> because the OSX implementation was different, but in this case it's > >> OSX-only code, so it only supported the Apple Common Crypto backend. > > > > I don't know how germane it is to the current thread, but previous > > discussions[1,2,3,4] favored dropping use of Apple's Common Crypto > > altogether since it doesn't seem to buy us much (or anything) and is > > incomplete; it doesn't support all of the OpenSSL API Git uses. > > Yeah, maybe. I'd be 100% OK with that happening, but I don't use OSX > outside fo CI really, so I wanted to avoid scope creep to "non-SHA stuff > we use OpenSSL/AppleCommonCrypto/whatever" for, in the cases where we > can/do use OpenSSL/AppleCommonCrypto/whatever for SHA also. Indeed, I was not suggesting that retirement of AppleCommonCrypto be tackled by you or by the series you posted to fix the build failure; doing so is well outside the scope of the immediate problem. I brought up those old discussions only as reference and as a pointer that whatever fix is eventually adopted for the immediate build problem need not necessarily bend over backward for AppleCommonCrypto support if a long-term goal is to drop AppleCommonCrypto (i.e. do as little as necessary to keep AppleCommonCrypto in a working state, but don't go overboard trying to give it first-class support since it will never be a complete replacement for OpenSSL). > But if you/someone can come up with a patch that confidently asserts > that it's useless & drops it I'd be happy to either integrate it & > submit it as part of this series if it makes things simpler, or > alternatively re-roll on top of it. > > I'm just not confident in making that case myself, since I hardly use > the platform, am not too familiar with the various concerns (aside from > skimming the links above), and don't really have interest in pursuing > that. I was not suggesting holding up your series or integrating retirement of AppleCommonCrypto with it; they are separate concerns. > OTOH if you do want to make the case for dropping Apple Common Crypto > that would also presumably be much easier after this series, once we're > past justifying the hurdle of "wait, we're switching the thing we use > for SHA-1 by default, which we build practically everything in git on?" My "don't know how germane it is to the current thread" observation was in response to the `fsmonitor` thread, before I ever saw the series you posted for fixing the build failure, so it wasn't given as any sort of feedback on your series, and wasn't asking you to incorporate retirement of AppleCommonCrypto into your series. That said, it does appear that dropping AppleCommonCrypto should become easier after your changes land if someone opts to tackle the task.
Glen Choo <chooglen@google.com> writes: > At $DAYJOB, we observed that this topic breaks MacOS builds with sha1dc: > > $ make NO_APPLE_COMMON_CRYPTO=1 DC_SHA1=1 NO_OPENSSL=1 compat/fsmonitor/fsm-ipc-darwin.o Glen, Ævar has cherry-picked the SHA_CTX -> git_SHA_CTX build fix and I have merged it to 'next'. Can you test-build to see if that change is sufficient to fix "does not even build from the source" issue for you guys? You do not have to go through any official full qualification cycle or release procedure---just checking that you would be OK when you need to do the build the next time before it has to happen is sufficient. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Possible action items: > > * See what configurations our two macOS jobs are using. If neither > is using sha1dc, I would say that is criminal [*] and at least > one of them should be updated to do so right away. So here is my "panda-see-panda-do" attempt. ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- Subject: [PATCH] ci: use DC_SHA1=1 on osx-clang job for CI All other jobs were using the default DC_SHA1 (which is a recommended practice), but the default for macOS jobs being Apple's common crypt, we didn't catch recent breakage soon enough. We may want to give similar diversity for Linux jobs so that some of them build with other implementations of SHA-1, but let's start small and fill only the immediate need. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- ci/lib.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/lib.sh b/ci/lib.sh index 1b0cc2b57d..5a115704cb 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -259,6 +259,8 @@ macos-latest) MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)" else MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python2)" + MAKEFLAGS="$MAKEFLAGS NO_APPLE_COMMON_CRYPTO=NoThanks" + MAKEFLAGS="$MAKEFLAGS DC_SHA1=1 NO_OPENSSL=1" fi ;; esac
On Thu, Oct 20, 2022 at 12:13 PM Junio C Hamano <gitster@pobox.com> wrote: > So here is my "panda-see-panda-do" attempt. > > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- > Subject: [PATCH] ci: use DC_SHA1=1 on osx-clang job for CI > > All other jobs were using the default DC_SHA1 (which is a > recommended practice), but the default for macOS jobs being Apple's > common crypt, we didn't catch recent breakage soon enough. "recent breakage" is quite vague and probably won't help future readers understand what this is actually fixing. Possible improvements: (1) a prose description of the breakage; (2) the actual compiler error message; (3) a pointer[1] to the email reporting the problem. One or more of the above improvements to the commit message would help future readers. [1]: https://lore.kernel.org/git/kl6l7d0yyu6r.fsf@chooglen-macbookpro.roam.corp.google.com/ > We may want to give similar diversity for Linux jobs so that some of > them build with other implementations of SHA-1, but let's start > small and fill only the immediate need. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > diff --git a/ci/lib.sh b/ci/lib.sh > index 1b0cc2b57d..5a115704cb 100755 > --- a/ci/lib.sh > +++ b/ci/lib.sh > @@ -259,6 +259,8 @@ macos-latest) > MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)" > else > MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python2)" > + MAKEFLAGS="$MAKEFLAGS NO_APPLE_COMMON_CRYPTO=NoThanks" > + MAKEFLAGS="$MAKEFLAGS DC_SHA1=1 NO_OPENSSL=1" > fi > ;; > esac
Eric Sunshine <sunshine@sunshineco.com> writes: > On Thu, Oct 20, 2022 at 12:13 PM Junio C Hamano <gitster@pobox.com> wrote: >> So here is my "panda-see-panda-do" attempt. >> >> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- >> Subject: [PATCH] ci: use DC_SHA1=1 on osx-clang job for CI >> >> All other jobs were using the default DC_SHA1 (which is a >> recommended practice), but the default for macOS jobs being Apple's >> common crypt, we didn't catch recent breakage soon enough. > > "recent breakage" is quite vague and probably won't help future > readers understand what this is actually fixing. Possible > improvements: (1) a prose description of the breakage; (2) the actual > compiler error message; (3) a pointer[1] to the email reporting the > problem. One or more of the above improvements to the commit message > would help future readers. I do not think (2) or (3) would help all that much. A finger that points at the exact commit that broke the build (with the condition under which the build breaks) would probably be the most useful to those who read "git log" output. Thanks. ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- Subject: [PATCH] ci: use DC_SHA1=YesPlease on osx-clang job for CI 7b8cfe34 (Merge branch 'ed/fsmonitor-on-networked-macos', 2022-10-17) broke the build on macOS with sha1dc by bypassing our hash abstraction (git_SHA_CTX etc.), but it wasn't caught before the problematic topic was merged down to the 'master' branch. Nobody was even compile testing with DC_SHA1 set, although it is the recommended choice in these days for folks when they use SHA-1. This was because the default for macOS uses Apple Common Crypto, and both of the two CI jobs did not override the default. Tweak one of them to use DC_SHA1 to improve the coverage. We may want to give similar diversity for Linux jobs so that some of them build with other implementations of SHA-1; they currently all build and test with DC_SHA1 as that is the default on everywhere other than macOS. But let's start small to fill only the immediate need. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- ci/lib.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/lib.sh b/ci/lib.sh index 1b0cc2b57d..51e47aa5d6 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -259,6 +259,8 @@ macos-latest) MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)" else MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python2)" + MAKEFLAGS="$MAKEFLAGS NO_APPLE_COMMON_CRYPTO=NoThanks" + MAKEFLAGS="$MAKEFLAGS DC_SHA1=YesPlease NO_OPENSSL=NoThanks" fi ;; esac
On Thu, Oct 20, 2022 at 1:01 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > On Thu, Oct 20, 2022 at 12:13 PM Junio C Hamano <gitster@pobox.com> wrote: > >> All other jobs were using the default DC_SHA1 (which is a > >> recommended practice), but the default for macOS jobs being Apple's > >> common crypt, we didn't catch recent breakage soon enough. > > > > "recent breakage" is quite vague and probably won't help future > > readers understand what this is actually fixing. Possible > > improvements: (1) a prose description of the breakage; (2) the actual > > compiler error message; (3) a pointer[1] to the email reporting the > > problem. One or more of the above improvements to the commit message > > would help future readers. > > I do not think (2) or (3) would help all that much. A finger that > points at the exact commit that broke the build (with the condition > under which the build breaks) would probably be the most useful to > those who read "git log" output. > > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- > Subject: [PATCH] ci: use DC_SHA1=YesPlease on osx-clang job for CI > > 7b8cfe34 (Merge branch 'ed/fsmonitor-on-networked-macos', > 2022-10-17) broke the build on macOS with sha1dc by bypassing our > hash abstraction (git_SHA_CTX etc.), but it wasn't caught before the > problematic topic was merged down to the 'master' branch. Nobody > was even compile testing with DC_SHA1 set, although it is the > recommended choice in these days for folks when they use SHA-1. > > This was because the default for macOS uses Apple Common Crypto, and > both of the two CI jobs did not override the default. Tweak one of > them to use DC_SHA1 to improve the coverage. Thanks. This revised commit message does a much better job of explaining the problem the patch is addressing.
Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> writes: > >> At $DAYJOB, we observed that this topic breaks MacOS builds with sha1dc: >> >> $ make NO_APPLE_COMMON_CRYPTO=1 DC_SHA1=1 NO_OPENSSL=1 compat/fsmonitor/fsm-ipc-darwin.o > > Glen, Ævar has cherry-picked the SHA_CTX -> git_SHA_CTX build fix > and I have merged it to 'next'. Can you test-build to see if that > change is sufficient to fix "does not even build from the source" > issue for you guys? You do not have to go through any official full > qualification cycle or release procedure---just checking that you > would be OK when you need to do the build the next time before it > has to happen is sufficient. > > Thanks. FYI, https://github.com/git/git/actions/runs/3292710187 is a successful (wow, what's a rare event these days) run of CI on 'seen' that includes * Peff's -O0 fix for unconfusing LSan to prevent it from giving false positives * Eric DeCosta's change, cherry-picked by Ævar, to let fsmonitor stuff compile with sha1dc as well as Apple's Common Crypto. * A tweak to make one of the macOS CI jobs to build with sha1dc. among all the other topics in flight. I plan to merge all of them to 'next' (I think the first two are already in 'next') and fast track them down to 'master'.
Goal is to deliver fsmonitor for Linux that is on par with fsmonitor for Windows and Mac OS. This patch set builds upon previous work for done for Windows and Mac OS (first 6 patches) to implement a fsmonitor back-end for Linux based on the Linux inotify API. inotify differs significantly from the equivalent Windows and Mac OS APIs in that a watch must be registered for every directory of interest (rather than a singular watch at the root of the directory tree) and special care must be taken to handle directory renames correctly. More information about inotify: https://man7.org/linux/man-pages/man7/inotify.7.html v1 differs from v0: * Code review feedback * Update how and which code can be shared between Mac OS and Linux * Increase polling frequency to every 1ms (matches Mac OS) * Updates to t7527 to improve test stability Eric DeCosta (12): fsmonitor: refactor filesystem checks to common interface fsmonitor: relocate socket file if .git directory is remote fsmonitor: avoid socket location check if using hook fsmonitor: deal with synthetic firmlinks on macOS fsmonitor: check for compatability before communicating with fsmonitor fsmonitor: add documentation for allowRemote and socketDir options fsmonitor: prepare to share code between Mac OS and Linux fsmonitor: determine if filesystem is local or remote fsmonitor: implement filesystem change listener for Linux fsmonitor: enable fsmonitor for Linux fsmonitor: test updates fsmonitor: update doc for Linux Documentation/config.txt | 2 + Documentation/config/fsmonitor--daemon.txt | 11 + Documentation/git-fsmonitor--daemon.txt | 45 +- Makefile | 9 + builtin/fsmonitor--daemon.c | 11 +- compat/fsmonitor/fsm-health-linux.c | 24 + compat/fsmonitor/fsm-ipc-unix.c | 52 ++ compat/fsmonitor/fsm-ipc-win32.c | 9 + compat/fsmonitor/fsm-listen-darwin.c | 14 +- compat/fsmonitor/fsm-listen-linux.c | 664 +++++++++++++++++++++ compat/fsmonitor/fsm-path-utils-darwin.c | 135 +++++ compat/fsmonitor/fsm-path-utils-linux.c | 169 ++++++ compat/fsmonitor/fsm-path-utils-win32.c | 145 +++++ compat/fsmonitor/fsm-settings-darwin.c | 90 +-- compat/fsmonitor/fsm-settings-linux.c | 11 + compat/fsmonitor/fsm-settings-unix.c | 62 ++ compat/fsmonitor/fsm-settings-unix.h | 11 + compat/fsmonitor/fsm-settings-win32.c | 174 +----- config.mak.uname | 10 + contrib/buildsystems/CMakeLists.txt | 19 +- fsmonitor--daemon.h | 3 + fsmonitor-ipc.c | 18 +- fsmonitor-ipc.h | 4 +- fsmonitor-path-utils.h | 60 ++ fsmonitor-settings.c | 68 ++- fsmonitor-settings.h | 4 +- fsmonitor.c | 7 + t/t7527-builtin-fsmonitor.sh | 72 ++- 28 files changed, 1606 insertions(+), 297 deletions(-) create mode 100644 Documentation/config/fsmonitor--daemon.txt create mode 100644 compat/fsmonitor/fsm-health-linux.c create mode 100644 compat/fsmonitor/fsm-ipc-unix.c create mode 100644 compat/fsmonitor/fsm-ipc-win32.c create mode 100644 compat/fsmonitor/fsm-listen-linux.c create mode 100644 compat/fsmonitor/fsm-path-utils-darwin.c create mode 100644 compat/fsmonitor/fsm-path-utils-linux.c create mode 100644 compat/fsmonitor/fsm-path-utils-win32.c create mode 100644 compat/fsmonitor/fsm-settings-linux.c create mode 100644 compat/fsmonitor/fsm-settings-unix.c create mode 100644 compat/fsmonitor/fsm-settings-unix.h create mode 100644 fsmonitor-path-utils.h base-commit: d420dda0576340909c3faff364cfbd1485f70376 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1352%2Fedecosta-mw%2Ffsmonitor_linux-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1352/edecosta-mw/fsmonitor_linux-v2 Pull-Request: https://github.com/git/git/pull/1352 Range-diff vs v1: 1: ec49a74086d = 1: cd46bed37c3 fsmonitor: refactor filesystem checks to common interface 2: 7bf1cdfe3b2 = 2: 21d114bda4b fsmonitor: relocate socket file if .git directory is remote 3: c5e8b6cfe5d = 3: 664259ed57a fsmonitor: avoid socket location check if using hook 4: 863063aefee = 4: d8f20032d6b fsmonitor: deal with synthetic firmlinks on macOS 5: fa974bfd5ef = 5: ab1e0ab967c fsmonitor: check for compatability before communicating with fsmonitor 6: af7309745f7 = 6: 9c552239b57 fsmonitor: add documentation for allowRemote and socketDir options 7: c085fc15b31 ! 7: 295beb89ab1 fsmonitor: prepare to share code between Mac OS and Linux @@ Commit message fsmonitor: prepare to share code between Mac OS and Linux Linux and Mac OS can share some of the code originally developed for Mac OS. - Rename the shared code from compat/fsmonitor/fsm-*-dawrin.c to - compat/fsmonitor/fsm-*-unix.c - - Update the build to enable sharing of the fsm-*-unix.c files. Minor update to compat/fsmonitor/fsm-ipc-unix.c to make it cross-platform. + Mac OS and Linux can share fsm-ipc-unix.c + + Both platforms can also share compat/fsmonitor/fsm-settings-unix.c but we + will leave room for future, platform-specific checks by having the platform- + specific implementations call into fsm-settings-unix. Signed-off-by: Eric DeCosta <edecosta@mathworks.com> ## Makefile ## @@ Makefile: endif + ifdef FSMONITOR_DAEMON_BACKEND COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND ++ ifdef FSMONITOR_DAEMON_COMMON ++ COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_COMMON).o ++ else ++ COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_BACKEND).o ++ endif COMPAT_OBJS += compat/fsmonitor/fsm-listen-$(FSMONITOR_DAEMON_BACKEND).o -- COMPAT_OBJS += compat/fsmonitor/fsm-health-$(FSMONITOR_DAEMON_BACKEND).o + COMPAT_OBJS += compat/fsmonitor/fsm-health-$(FSMONITOR_DAEMON_BACKEND).o - COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_BACKEND).o -+ COMPAT_OBJS += compat/fsmonitor/fsm-health-$(FSMONITOR_DAEMON_COMMON).o -+ COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_COMMON).o endif ifdef FSMONITOR_OS_SETTINGS COMPAT_CFLAGS += -DHAVE_FSMONITOR_OS_SETTINGS -- COMPAT_OBJS += compat/fsmonitor/fsm-settings-$(FSMONITOR_OS_SETTINGS).o -+ COMPAT_OBJS += compat/fsmonitor/fsm-settings-$(FSMONITOR_DAEMON_COMMON).o ++ ifdef FSMONITOR_DAEMON_COMMON ++ COMPAT_OBJS += compat/fsmonitor/fsm-settings-$(FSMONITOR_DAEMON_COMMON).o ++ endif + COMPAT_OBJS += compat/fsmonitor/fsm-settings-$(FSMONITOR_OS_SETTINGS).o COMPAT_OBJS += compat/fsmonitor/fsm-path-utils-$(FSMONITOR_OS_SETTINGS).o endif - - ## compat/fsmonitor/fsm-health-darwin.c => compat/fsmonitor/fsm-health-unix.c ## + ## compat/fsmonitor/fsm-health-linux.c (new) ## +@@ ++#include "cache.h" ++#include "config.h" ++#include "fsmonitor.h" ++#include "fsm-health.h" ++#include "fsmonitor--daemon.h" ++ ++int fsm_health__ctor(struct fsmonitor_daemon_state *state) ++{ ++ return 0; ++} ++ ++void fsm_health__dtor(struct fsmonitor_daemon_state *state) ++{ ++ return; ++} ++ ++void fsm_health__loop(struct fsmonitor_daemon_state *state) ++{ ++ return; ++} ++ ++void fsm_health__stop_async(struct fsmonitor_daemon_state *state) ++{ ++} ## compat/fsmonitor/fsm-ipc-darwin.c => compat/fsmonitor/fsm-ipc-unix.c ## @@ compat/fsmonitor/fsm-ipc-unix.c: static GIT_PATH_FUNC(fsmonitor_ipc__get_default_path, "fsmonitor--daemon.ipc") @@ compat/fsmonitor/fsm-ipc-unix.c: const char *fsmonitor_ipc__get_path(struct repo repo_config_get_string(r, "fsmonitor.socketdir", &sock_dir); - ## compat/fsmonitor/fsm-settings-darwin.c => compat/fsmonitor/fsm-settings-unix.c ## + ## compat/fsmonitor/fsm-settings-darwin.c ## +@@ + #include "config.h" + #include "fsmonitor.h" + #include "fsmonitor-ipc.h" +-#include "fsmonitor-settings.h" + #include "fsmonitor-path-utils.h" +- +- /* +- * For the builtin FSMonitor, we create the Unix domain socket for the +- * IPC in the .git directory. If the working directory is remote, +- * then the socket will be created on the remote file system. This +- * can fail if the remote file system does not support UDS file types +- * (e.g. smbfs to a Windows server) or if the remote kernel does not +- * allow a non-local process to bind() the socket. (These problems +- * could be fixed by moving the UDS out of the .git directory and to a +- * well-known local directory on the client machine, but care should +- * be taken to ensure that $HOME is actually local and not a managed +- * file share.) +- * +- * FAT32 and NTFS working directories are problematic too. +- * +- * The builtin FSMonitor uses a Unix domain socket in the .git +- * directory for IPC. These Windows drive formats do not support +- * Unix domain sockets, so mark them as incompatible for the daemon. +- * +- */ +-static enum fsmonitor_reason check_uds_volume(struct repository *r) +-{ +- struct fs_info fs; +- const char *ipc_path = fsmonitor_ipc__get_path(r); +- struct strbuf path = STRBUF_INIT; +- strbuf_add(&path, ipc_path, strlen(ipc_path)); +- +- if (fsmonitor__get_fs_info(dirname(path.buf), &fs) == -1) { +- strbuf_release(&path); +- return FSMONITOR_REASON_ERROR; +- } +- +- strbuf_release(&path); +- +- if (fs.is_remote || +- !strcmp(fs.typename, "msdos") || +- !strcmp(fs.typename, "ntfs")) { +- free(fs.typename); +- return FSMONITOR_REASON_NOSOCKETS; +- } +- +- free(fs.typename); +- return FSMONITOR_REASON_OK; +-} ++#include "fsmonitor-settings.h" ++#include "fsm-settings-unix.h" + + enum fsmonitor_reason fsm_os__incompatible(struct repository *r, int ipc) + { +- enum fsmonitor_reason reason; +- +- if (ipc) { +- reason = check_uds_volume(r); +- if (reason != FSMONITOR_REASON_OK) +- return reason; +- } +- +- return FSMONITOR_REASON_OK; ++ return fsm_os__incompatible_unix(r, ipc); + } + + ## compat/fsmonitor/fsm-settings-linux.c (new) ## +@@ ++#include "config.h" ++#include "fsmonitor.h" ++#include "fsmonitor-ipc.h" ++#include "fsmonitor-path-utils.h" ++#include "fsmonitor-settings.h" ++#include "fsm-settings-unix.h" ++ ++enum fsmonitor_reason fsm_os__incompatible(struct repository *r, int ipc) ++{ ++ return fsm_os__incompatible_unix(r, ipc); ++} - ## config.mak.uname ## -@@ config.mak.uname: ifeq ($(uname_S),Linux) - ifneq ($(findstring .el7.,$(uname_R)),) - BASIC_CFLAGS += -std=c99 - endif + ## compat/fsmonitor/fsm-settings-unix.c (new) ## +@@ ++#include "config.h" ++#include "fsmonitor.h" ++#include "fsmonitor-ipc.h" ++#include "fsmonitor-path-utils.h" ++#include "fsm-settings-unix.h" + - endif - ifeq ($(uname_S),GNU/kFreeBSD) - HAVE_ALLOCA_H = YesPlease ++ /* ++ * For the builtin FSMonitor, we create the Unix domain socket for the ++ * IPC in the .git directory. If the working directory is remote, ++ * then the socket will be created on the remote file system. This ++ * can fail if the remote file system does not support UDS file types ++ * (e.g. smbfs to a Windows server) or if the remote kernel does not ++ * allow a non-local process to bind() the socket. (These problems ++ * could be fixed by moving the UDS out of the .git directory and to a ++ * well-known local directory on the client machine, but care should ++ * be taken to ensure that $HOME is actually local and not a managed ++ * file share.) ++ * ++ * FAT32 and NTFS working directories are problematic too. ++ * ++ * The builtin FSMonitor uses a Unix domain socket in the .git ++ * directory for IPC. These Windows drive formats do not support ++ * Unix domain sockets, so mark them as incompatible for the daemon. ++ * ++ */ ++static enum fsmonitor_reason check_uds_volume(struct repository *r) ++{ ++ struct fs_info fs; ++ const char *ipc_path = fsmonitor_ipc__get_path(r); ++ struct strbuf path = STRBUF_INIT; ++ strbuf_add(&path, ipc_path, strlen(ipc_path)); ++ ++ if (fsmonitor__get_fs_info(dirname(path.buf), &fs) == -1) { ++ strbuf_release(&path); ++ return FSMONITOR_REASON_ERROR; ++ } ++ ++ strbuf_release(&path); ++ ++ if (fs.is_remote || ++ !strcmp(fs.typename, "msdos") || ++ !strcmp(fs.typename, "ntfs")) { ++ free(fs.typename); ++ return FSMONITOR_REASON_NOSOCKETS; ++ } ++ ++ free(fs.typename); ++ return FSMONITOR_REASON_OK; ++} ++ ++enum fsmonitor_reason fsm_os__incompatible_unix(struct repository *r, int ipc) ++{ ++ enum fsmonitor_reason reason; ++ ++ if (ipc) { ++ reason = check_uds_volume(r); ++ if (reason != FSMONITOR_REASON_OK) ++ return reason; ++ } ++ ++ return FSMONITOR_REASON_OK; ++} + + ## compat/fsmonitor/fsm-settings-unix.h (new) ## +@@ ++#ifndef FSM_SETTINGS_UNIX_H ++#define FSM_SETTINGS_UNIX_H ++ ++#ifdef HAVE_FSMONITOR_OS_SETTINGS ++/* ++ * Check for compatibility on unix-like systems (e.g. Darwin and Linux) ++ */ ++enum fsmonitor_reason fsm_os__incompatible_unix(struct repository *r, int ipc); ++#endif /* HAVE_FSMONITOR_OS_SETTINGS */ ++ ++#endif /* FSM_SETTINGS_UNIX_H */ + + ## config.mak.uname ## @@ config.mak.uname: ifeq ($(uname_S),Darwin) ifndef NO_UNIX_SOCKETS FSMONITOR_DAEMON_BACKEND = darwin @@ config.mak.uname: ifeq ($(uname_S),Darwin) endif endif -@@ config.mak.uname: ifeq ($(uname_S),Windows) - # support it. - FSMONITOR_DAEMON_BACKEND = win32 - FSMONITOR_OS_SETTINGS = win32 -+ FSMONITOR_DAEMON_COMMON = win32 - - NO_SVN_TESTS = YesPlease - RUNTIME_PREFIX = YesPlease -@@ config.mak.uname: ifeq ($(uname_S),MINGW) - # support it. - FSMONITOR_DAEMON_BACKEND = win32 - FSMONITOR_OS_SETTINGS = win32 -+ FSMONITOR_DAEMON_COMMON = win32 - - RUNTIME_PREFIX = YesPlease - HAVE_WPGMPTR = YesWeDo ## contrib/buildsystems/CMakeLists.txt ## +@@ contrib/buildsystems/CMakeLists.txt: else() + endif() + + if(SUPPORTS_SIMPLE_IPC) +- if(CMAKE_SYSTEM_NAME STREQUAL "Windows") ++ if(CMAKE_SYSTEM_NAME STREQUAL "Linux") ++ add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND) ++ list(APPEND compat_SOURCES compat/fsmonitor/fsm-ipc-unix.c) ++ list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-linux.c) ++ list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-linux.c) ++ list(APPEND compat_SOURCES compat/fsmonitor/fsm-path-utils-linux.c) ++ ++ add_compile_definitions(HAVE_FSMONITOR_OS_SETTINGS) ++ list(APPEND compat_SOURCES compat/fsmonitor/fsm-settings-unix.c) ++ list(APPEND compat_SOURCES compat/fsmonitor/fsm-settings-linux.c) ++ elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows") + add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND) + list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-win32.c) + list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-win32.c) @@ contrib/buildsystems/CMakeLists.txt: if(SUPPORTS_SIMPLE_IPC) list(APPEND compat_SOURCES compat/fsmonitor/fsm-settings-win32.c) elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin") add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND) +- list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-darwin.c) + list(APPEND compat_SOURCES compat/fsmonitor/fsm-ipc-unix.c) -+ list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-unix.c) - list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-darwin.c) -- list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-darwin.c) + list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-darwin.c) - list(APPEND compat_SOURCES compat/fsmonitor/fsm-ipc-darwin.c) ++ list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-darwin.c) list(APPEND compat_SOURCES compat/fsmonitor/fsm-path-utils-darwin.c) add_compile_definitions(HAVE_FSMONITOR_OS_SETTINGS) -- list(APPEND compat_SOURCES compat/fsmonitor/fsm-settings-darwin.c) + list(APPEND compat_SOURCES compat/fsmonitor/fsm-settings-unix.c) + list(APPEND compat_SOURCES compat/fsmonitor/fsm-settings-darwin.c) endif() endif() - 8: 5ecbb3082f1 ! 8: 7d7ef78728f fsmonitor: determine if filesystem is local or remote @@ compat/fsmonitor/fsm-path-utils-linux.c (new) + || strcmp ("-hosts", Fs_name) == 0) +#endif + -+static struct mntent *find_mount(const char *path, const struct statvfs *fs) ++static int find_mount(const char *path, const struct statvfs *fs, ++ struct mntent *ent) +{ + const char *const mounts = "/proc/mounts"; + const char *rp = real_pathdup(path, 1); + struct mntent *ment = NULL; -+ struct mntent *found = NULL; + struct statvfs mntfs; + FILE *fp; ++ int found = 0; + int dlen, plen, flen = 0; + ++ ent->mnt_fsname = NULL; ++ ent->mnt_dir = NULL; ++ ent->mnt_type = NULL; ++ + fp = setmntent(mounts, "r"); + if (!fp) { + error_errno(_("setmntent('%s') failed"), mounts); -+ return NULL; ++ return -1; + } + + plen = strlen(rp); @@ compat/fsmonitor/fsm-path-utils-linux.c (new) + default: + error_errno(_("statvfs('%s') failed"), ment->mnt_dir); + endmntent(fp); -+ return NULL; ++ return -1; + } + } + @@ compat/fsmonitor/fsm-path-utils-linux.c (new) + * The pointer points to a static area of memory which is + * overwritten by subsequent calls to getmntent(). + */ -+ if (!found) -+ CALLOC_ARRAY(found, 1); -+ free(found->mnt_dir); -+ free(found->mnt_type); -+ free(found->mnt_fsname); -+ found->mnt_dir = xmemdupz(ment->mnt_dir, strlen(ment->mnt_dir)); -+ found->mnt_type = xmemdupz(ment->mnt_type, strlen(ment->mnt_type)); -+ found->mnt_fsname = xmemdupz(ment->mnt_fsname, strlen(ment->mnt_fsname)); ++ found = 1; ++ free(ent->mnt_fsname); ++ free(ent->mnt_dir); ++ free(ent->mnt_type); ++ ent->mnt_fsname = xstrdup(ment->mnt_fsname); ++ ent->mnt_dir = xstrdup(ment->mnt_dir); ++ ent->mnt_type = xstrdup(ment->mnt_type); + } + } + } + endmntent(fp); + -+ return found; ++ if (!found) ++ return -1; ++ ++ return 0; +} + +int fsmonitor__get_fs_info(const char *path, struct fs_info *fs_info) +{ -+ struct mntent *ment; ++ struct mntent ment; + struct statvfs fs; + + if (statvfs(path, &fs)) + return error_errno(_("statvfs('%s') failed"), path); + -+ ment = find_mount(path, &fs); -+ if (!ment) ++ ++ if (find_mount(path, &fs, &ment) < 0) { ++ free(ment.mnt_fsname); ++ free(ment.mnt_dir); ++ free(ment.mnt_type); + return -1; ++ } + + trace_printf_key(&trace_fsmonitor, + "statvfs('%s') [flags 0x%08lx] '%s' '%s'", -+ path, fs.f_flag, ment->mnt_type, ment->mnt_fsname); -+ -+ if (ME_REMOTE(ment->mnt_fsname, ment->mnt_type)) -+ fs_info->is_remote = 1; -+ else -+ fs_info->is_remote = 0; ++ path, fs.f_flag, ment.mnt_type, ment.mnt_fsname); + -+ fs_info->typename = ment->mnt_fsname; -+ free(ment->mnt_dir); -+ free(ment->mnt_type); -+ free(ment); ++ fs_info->is_remote = ME_REMOTE(ment.mnt_fsname, ment.mnt_type); ++ fs_info->typename = ment.mnt_fsname; ++ free(ment.mnt_dir); ++ free(ment.mnt_type); + + trace_printf_key(&trace_fsmonitor, + "'%s' is_remote: %d", 9: 7465fe7a8b4 ! 9: 4f9c5358475 fsmonitor: implement filesystem change listener for Linux @@ compat/fsmonitor/fsm-listen-linux.c (new) + +/* + * Non-blocking read of the inotify events stream. The inotify fd is polled -+ * every few millisec to help minimize the number of queue overflows. ++ * frequently to help minimize the number of queue overflows. + */ +void fsm_listen__loop(struct fsmonitor_daemon_state *state) +{ @@ compat/fsmonitor/fsm-listen-linux.c (new) + for(;;) { + switch (state->listen_data->shutdown) { + case SHUTDOWN_CONTINUE: -+ poll_num = poll(fds, 1, 10); ++ poll_num = poll(fds, 1, 1); + if (poll_num == -1) { + if (errno == EINTR) + continue; 10: 3a2db9aa076 ! 10: 07650ecd27b fsmonitor: enable fsmonitor for Linux @@ Metadata ## Commit message ## fsmonitor: enable fsmonitor for Linux - Uodate build to enable fsmonitor for Linux. + Update build to enable fsmonitor for Linux. Signed-off-by: Eric DeCosta <edecosta@mathworks.com> ## config.mak.uname ## @@ config.mak.uname: ifeq ($(uname_S),Linux) + ifneq ($(findstring .el7.,$(uname_R)),) BASIC_CFLAGS += -std=c99 endif - + # The builtin FSMonitor on Linux builds upon Simple-IPC. Both require + # Unix domain sockets and PThreads. + ifndef NO_PTHREADS @@ config.mak.uname: ifeq ($(uname_S),Linux) endif ifeq ($(uname_S),GNU/kFreeBSD) HAVE_ALLOCA_H = YesPlease - - ## contrib/buildsystems/CMakeLists.txt ## -@@ contrib/buildsystems/CMakeLists.txt: else() - endif() - - if(SUPPORTS_SIMPLE_IPC) -- if(CMAKE_SYSTEM_NAME STREQUAL "Windows") -+ if(CMAKE_SYSTEM_NAME STREQUAL "Linux") -+ add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND) -+ list(APPEND compat_SOURCES compat/fsmonitor/fsm-ipc-unix.c) -+ list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-unix.c) -+ list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-linux.c) -+ list(APPEND compat_SOURCES compat/fsmonitor/fsm-path-utils-linux.c) -+ -+ add_compile_definitions(HAVE_FSMONITOR_OS_SETTINGS) -+ list(APPEND compat_SOURCES compat/fsmonitor/fsm-settings-unix.c) -+ elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows") - add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND) - list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-win32.c) - list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-win32.c) 11: 743bdacded5 ! 11: 6682938fff8 fsmonitor: test updates @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'edit some files' ' test-tool fsmonitor-client query --token 0 && ++ test_might_fail git fsmonitor--daemon stop && ++ + grep "^event: dir1/modified$" .git/trace && + grep "^event: dir2/modified$" .git/trace && + grep "^event: modified$" .git/trace && @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'create some files' ' start_daemon --tf "$PWD/.git/trace" && @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'create some files' ' test-tool fsmonitor-client query --token 0 && ++ test_might_fail git fsmonitor--daemon stop && ++ + grep "^event: dir1/new$" .git/trace && + grep "^event: dir2/new$" .git/trace && + grep "^event: new$" .git/trace @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'delete some files' ' start_daemon --tf "$PWD/.git/trace" && @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'delete some files' ' test-tool fsmonitor-client query --token 0 && ++ test_might_fail git fsmonitor--daemon stop && ++ + grep "^event: dir1/delete$" .git/trace && + grep "^event: dir2/delete$" .git/trace && + grep "^event: delete$" .git/trace @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'rename some files' ' start_daemon --tf "$PWD/.git/trace" && @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'rename some files' ' test-tool fsmonitor-client query --token 0 && ++ test_might_fail git fsmonitor--daemon stop && ++ + grep "^event: dir1/rename$" .git/trace && + grep "^event: dir2/rename$" .git/trace && + grep "^event: rename$" .git/trace && @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'rename directory' ' start_daemon --tf "$PWD/.git/trace" && @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'rename directory' ' test-tool fsmonitor-client query --token 0 && ++ test_might_fail git fsmonitor--daemon stop && ++ + grep "^event: dirtorename/*$" .git/trace && + grep "^event: dirrenamed/*$" .git/trace + ' @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'file changes to directory' ' start_daemon --tf "$PWD/.git/trace" && @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'file changes to directory' ' test-tool fsmonitor-client query --token 0 && ++ test_might_fail git fsmonitor--daemon stop && ++ + grep "^event: delete$" .git/trace && + grep "^event: delete/new$" .git/trace + ' @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'directory changes to a file' ' start_daemon --tf "$PWD/.git/trace" && @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'directory changes to a file' test-tool fsmonitor-client query --token 0 && ++ test_might_fail git fsmonitor--daemon stop && ++ + grep "^event: dir1$" .git/trace + ' + @@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'flush cached data' ' test-tool -C test_flush fsmonitor-client query --token "builtin:test_00000002:0" >actual_2 && nul_to_q <actual_2 >actual_q2 && 12: 77ed35b3b80 = 12: dd73e126810 fsmonitor: update doc for Linux