{"thread":{"id":"18354","subject":"[PATCH] disable post-checkout test on Cygwin","startedAt":"2009-03-17T16:26:43Z","lastAt":"2009-03-18T18:56:31Z","messageCount":14,"participants":["Alex Riesen","Junio C Hamano","Johannes Sixt","Mark Levedahl"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"108252","messageId":"81b0412b0903170926p4f2d536el2b96a71c79c0159e@mail.gmail.com","threadId":"18354","inReplyTo":null,"subject":"[PATCH] disable post-checkout test on Cygwin","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-03-17T16:26:43Z","receivedAt":"2009-03-17T16:26:43Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"It is broken because of the tricks we have to play with\nlstat to get the bearable perfomance out of the call.\nSadly, it disables access to Cygwin's executable attribute,\nwhich Windows filesystems do not have at all.\n\nSigned-off-by: Alex Riesen \n---\n t/t5403-post-checkout-hook.sh | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\n2009/3/3 Jeff King :\n> +mkdir -p templates/hooks\n> +cat >templates/hooks/post-checkout <<'EOF'\n> +#!/bin/sh\n> +echo $@ > $GIT_DIR/post-checkout.args\n> +EOF\n> +chmod +x templates/hooks/post-checkout\n> +\n> +test_expect_success 'post-checkout hook is triggered by clone' '\n> +       git clone --template=templates . clone3 &&\n> +       test -f clone3/.git/post-checkout.args\n> +'\n\nThis is broken on cygwin: the hook script won't be not marked executable\nby copy_file, because the native Win32 stat(2) routines are used and\nreport the mode of source file as 0666.\n\n\nFrom e5394ee710460e25369b4755798930a3f19085c5 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Tue, 17 Mar 2009 17:22:53 +0100\nSubject: [PATCH] disable post-checkout test on Cygwin\n\nIt is broken because of the tricks we have to play with\nlstat to get the bearable perfomance out of the call.\nSadly, it disables access to Cygwin's executable attribute,\nwhich Windows filesystems do not have at all.\n\nSigned-off-by: Alex Riesen \n---\n t/t5403-post-checkout-hook.sh | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\nindex 4fdb418..5858b86 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -71,6 +71,7 @@ test_expect_success 'post-checkout receives the right args when not switching br\n test $old = $new -a $flag = 0\n '\n \n+if test \"$(git config --bool core.filemode)\" = true; then\n mkdir -p templates/hooks\n cat >templates/hooks/post-checkout <<'EOF'\n #!/bin/sh\n@@ -82,5 +83,6 @@ test_expect_success 'post-checkout hook is triggered by clone' '\n \tgit clone --template=templates . clone3 &&\n \ttest -f clone3/.git/post-checkout.args\n '\n+fi\n \n test_done\n-- \n1.6.2.142.gaf8db\n\n"},{"id":"108260","messageId":"7vprggqeh2.fsf@gitster.siamese.dyndns.org","threadId":"18354","inReplyTo":"81b0412b0903170926p4f2d536el2b96a71c79c0159e@mail.gmail.com","subject":"Re: [PATCH] disable post-checkout test on Cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-17T16:52:09Z","receivedAt":"2009-03-17T16:52:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> It is broken because of the tricks we have to play with\n> lstat to get the bearable perfomance out of the call.\n> Sadly, it disables access to Cygwin's executable attribute,\n> which Windows filesystems do not have at all.\n\nHmm, perhaps when checking hooks to see if they are executable, Cygwin\nport should avoid using the \"tricks\"? Compared to paths inside the\nworktree the number of hooks is a lot smaller, no?\n"},{"id":"108263","messageId":"49BFD6DD.1010800@viscovery.net","threadId":"18354","inReplyTo":"7vprggqeh2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] disable post-checkout test on Cygwin","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-03-17T16:59:09Z","receivedAt":"2009-03-17T16:59:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Alex Riesen writes:\n> \n>> It is broken because of the tricks we have to play with\n>> lstat to get the bearable perfomance out of the call.\n>> Sadly, it disables access to Cygwin's executable attribute,\n>> which Windows filesystems do not have at all.\n> \n> Hmm, perhaps when checking hooks to see if they are executable, Cygwin\n> port should avoid using the \"tricks\"? Compared to paths inside the\n> worktree the number of hooks is a lot smaller, no?\n\nHmm. Nowadays, we run hooks through run_hook() in run_command.c. It uses\naccess(..., X_OK), not lstat(). We don't play games with access(), do we?\n\n-- Hannes\n"},{"id":"108278","messageId":"20090317202818.GA13458@blimp.localdomain","threadId":"18354","inReplyTo":"49BFD6DD.1010800@viscovery.net","subject":"Re: [PATCH] disable post-checkout test on Cygwin","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-03-17T20:28:18Z","receivedAt":"2009-03-17T20:28:18Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Johannes Sixt, Tue, Mar 17, 2009 17:59:09 +0100:\n> Junio C Hamano schrieb:\n> > Alex Riesen writes:\n> > \n> >> It is broken because of the tricks we have to play with\n> >> lstat to get the bearable perfomance out of the call.\n> >> Sadly, it disables access to Cygwin's executable attribute,\n> >> which Windows filesystems do not have at all.\n> > \n> > Hmm, perhaps when checking hooks to see if they are executable, Cygwin\n> > port should avoid using the \"tricks\"? Compared to paths inside the\n> > worktree the number of hooks is a lot smaller, no?\n> \n> Hmm. Nowadays, we run hooks through run_hook() in run_command.c. It uses\n\nThe problem is that copy_templates_1 does an lstat on the files in\ntemplates directory and gets 0666 mode (regular file, non-exec) for\nexecutable file under current Cygwin port. The st_mode of that lstat\nis passed to copy_file mentioned, which is useless now as we use the\nWin32 version of lstat, which doesn't do x-bit.\n\n> access(..., X_OK), not lstat(). We don't play games with access(), do we?\n> \n\naccess(..., X_OK) will return -1.\n"},{"id":"108279","messageId":"20090317203421.GB13458@blimp.localdomain","threadId":"18354","inReplyTo":"7vprggqeh2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] disable post-checkout test on Cygwin","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-03-17T20:34:21Z","receivedAt":"2009-03-17T20:34:21Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Tue, Mar 17, 2009 17:52:09 +0100:\n> Alex Riesen writes:\n> \n> > It is broken because of the tricks we have to play with\n> > lstat to get the bearable perfomance out of the call.\n> > Sadly, it disables access to Cygwin's executable attribute,\n> > which Windows filesystems do not have at all.\n> \n> Hmm, perhaps when checking hooks to see if they are executable, Cygwin\n> port should avoid using the \"tricks\"? Compared to paths inside the\n> worktree the number of hooks is a lot smaller, no?\n\nYes, the damn thing is just hard to disable without ifdef in\nbuiltin-db.c or builtin-clone.c\n"},{"id":"108281","messageId":"7v63i7ridk.fsf@gitster.siamese.dyndns.org","threadId":"18354","inReplyTo":"20090317202818.GA13458@blimp.localdomain","subject":"Re: [PATCH] disable post-checkout test on Cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-17T20:42:31Z","receivedAt":"2009-03-17T20:42:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Johannes Sixt, Tue, Mar 17, 2009 17:59:09 +0100:\n>> Junio C Hamano schrieb:\n>> > Alex Riesen writes:\n>> > \n>> >> It is broken because of the tricks we have to play with\n>> >> lstat to get the bearable perfomance out of the call.\n>> >> Sadly, it disables access to Cygwin's executable attribute,\n>> >> which Windows filesystems do not have at all.\n>> > \n>> > Hmm, perhaps when checking hooks to see if they are executable, Cygwin\n>> > port should avoid using the \"tricks\"? Compared to paths inside the\n>> > worktree the number of hooks is a lot smaller, no?\n>> \n>> Hmm. Nowadays, we run hooks through run_hook() in run_command.c. It uses\n>\n> The problem is that copy_templates_1 does an lstat on the files in\n> templates directory and gets 0666 mode (regular file, non-exec) for\n> executable file under current Cygwin port. The st_mode of that lstat\n> is passed to copy_file mentioned, which is useless now as we use the\n> Win32 version of lstat, which doesn't do x-bit.\n\nAhhh.\n\nI do not mind the patch as a band-aid to make the testsuite pass, so I'll\napply your patch as-is. Thanks.\n\nBut isn't this something shops that do deploy Cygwin version of git want\nto see fixed, so that they can have a site-wide policy implemented in the\nhooks copied from templates? I think we could pass mode 0 to copy_files()\nand have the function special case it (and allow a platform specific\ncopy_files() implementated by Cygwin). lstat() in the copy_templates_1()\ncodepath is primarily done to see if we need to descend into a directory\nor symlink() and our use of st.st_mode to pass to copy_files() is a no\ncost side effect on platforms with x-bit support.\n\n>> access(..., X_OK), not lstat(). We don't play games with access(), do we?\n>\n> access(..., X_OK) will return -1.\n\nThat codepath would also need to be fixed if Cygwin wants to use hooks, I\nwould guess.\n"},{"id":"108286","messageId":"20090317213820.GC13458@blimp.localdomain","threadId":"18354","inReplyTo":"7v63i7ridk.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Define a version of lstat(2) specially for copy operation","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-03-17T21:38:21Z","receivedAt":"2009-03-17T21:38:21Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"So that Cygwin port can continue work around its supporting\nlibrary and get access to its faked file attributes.\n\nSigned-off-by: Alex Riesen \n---\nJunio C Hamano, Tue, Mar 17, 2009 21:42:31 +0100:\n> But isn't this something shops that do deploy Cygwin version of git want\n> to see fixed, so that they can have a site-wide policy implemented in the\n\nFrankly, I doubt they know or care.\n\n> hooks copied from templates? I think we could pass mode 0 to copy_files()\n> and have the function special case it (and allow a platform specific\n> copy_files() implementated by Cygwin). lstat() in the copy_templates_1()\n> codepath is primarily done to see if we need to descend into a directory\n> or symlink() and our use of st.st_mode to pass to copy_files() is a no\n> cost side effect on platforms with x-bit support.\n\nAnd I don't think that the platform broken in so many ways deserves\nthat kind of treatement. Maybe this patch is enough. Will test it\ntomorrow, when I get to mine so much hated Windows system.\n\n> >> access(..., X_OK), not lstat(). We don't play games with access(), do we?\n> > access(..., X_OK) will return -1.\n> \n> That codepath would also need to be fixed if Cygwin wants to use hooks, I\n> would guess.\n\nI hope not. In the reply to Johannes' example I was referring to the\ncopied file, the one which was created with open(..., O_CREAT..., 0666),\n0666 being there because of our lstat stub.\n\n builtin-init-db.c | 2 +-\n git-compat-util.h | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex ee3911f..f3f781b 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -66,7 +66,7 @@ static void copy_templates_1(char *path, int baselen,\n \t\telse\n \t\t\texists = 1;\n \n-\t\tif (lstat(template, &st_template))\n+\t\tif (lstat_for_copy(template, &st_template))\n \t\t\tdie(\"cannot stat template %s\", template);\n \n \t\tif (S_ISDIR(st_template.st_mode)) {\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 878d83d..4c23478 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -85,11 +85,16 @@\n #undef _XOPEN_SOURCE\n #include \n #define _XOPEN_SOURCE 600\n+static inline int lstat_for_copy(const char *file_name, struct stat *buf)\n+{\n+\treturn lstat(file_name, buf);\n+}\n #include \"compat/cygwin.h\"\n #else\n #undef _ALL_SOURCE /* AIX 5.3L defines a struct list with _ALL_SOURCE. */\n #include \n #define _ALL_SOURCE 1\n+#define lstat_for_copy lstat\n #endif\n #else \t/* __MINGW32__ */\n /* pull in Windows compatibility stuff */\n-- \n1.6.2.1.171.g3422\n"},{"id":"108325","messageId":"gppp48$5sc$1@ger.gmane.org","threadId":"18354","inReplyTo":"20090317213820.GC13458@blimp.localdomain","subject":"Re: [PATCH] Define a version of lstat(2) specially for copy operation","fromName":"Mark Levedahl","fromEmail":"mlevedahl@gmail.com","sentAt":"2009-03-18T03:17:25Z","receivedAt":"2009-03-18T03:17:25Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"Alex Riesen wrote:\n\n> So that Cygwin port can continue work around its supporting\n> library and get access to its faked file attributes.\n> \n> Signed-off-by: Alex Riesen \n> ---\n> Junio C Hamano, Tue, Mar 17, 2009 21:42:31 +0100:\n>> But isn't this something shops that do deploy Cygwin version of git want\n>> to see fixed, so that they can have a site-wide policy implemented in the\n> \n> Frankly, I doubt they know or care.\n\nPlease don't presume to speak for the world here. I have a not insignificant \ngroup of users, many on Cygwin, and we depend upon identical behavior \nbetween Linux and Cygwin versions of git. I maintain my own local build of \ngit for my group, among other reasons is to disable the non-Posix lstat \nhack. Using the Win32 lstat does not speed up git that much (maybe 20-30% \nin my experience) and this trade-off of compatibility vs being not quite so \ndreadfully slow is certainly not worth it for me. (Obviously, others have a \ndifferent view, or this feature would not exist).\n\nSo, I strongly urge keeping the differences between POSIX/Linux git and \nCygwin git as minimal as possible.\n\nMark Levedahl\n"},{"id":"108348","messageId":"81b0412b0903180022w4b95106ds54a16a835b53c1d@mail.gmail.com","threadId":"18354","inReplyTo":"20090317213820.GC13458@blimp.localdomain","subject":"Re: [PATCH] Define a version of lstat(2) specially for copy operation","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-03-18T07:22:24Z","receivedAt":"2009-03-18T07:22:24Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/3/17 Alex Riesen :\n>> hooks copied from templates?  I think we could pass mode 0 to copy_files()\n>> and have the function special case it (and allow a platform specific\n>> copy_files() implemented by Cygwin).  lstat() in the copy_templates_1()\n>> codepath is primarily done to see if we need to descend into a directory\n>> or symlink() and our use of st.st_mode to pass to copy_files() is a no\n>> cost side effect on platforms with x-bit support.\n>\n> And I don't think that the platform broken in so many ways deserves\n> that kind of treatment. Maybe this patch is enough. Will test it\n> tomorrow, when I get to mine so much hated Windows system.\n>\n\nOk, I just tested it and it works. Junio, this patch can replace the previous\nchange which disabled the test.\n"},{"id":"108350","messageId":"7vmybjl1l6.fsf@gitster.siamese.dyndns.org","threadId":"18354","inReplyTo":"20090317213820.GC13458@blimp.localdomain","subject":"Re: [PATCH] Define a version of lstat(2) specially for copy operation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-18T07:41:41Z","receivedAt":"2009-03-18T07:41:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> So that Cygwin port can continue work around its supporting\n> library and get access to its faked file attributes.\n>\n> Signed-off-by: Alex Riesen \n> ...\n> diff --git a/builtin-init-db.c b/builtin-init-db.c\n> index ee3911f..f3f781b 100644\n> --- a/builtin-init-db.c\n> +++ b/builtin-init-db.c\n> @@ -66,7 +66,7 @@ static void copy_templates_1(char *path, int baselen,\n> \t\telse\n> \t\t\texists = 1;\n> \n> -\t\tif (lstat(template, &st_template))\n> +\t\tif (lstat_for_copy(template, &st_template))\n> \t\t\tdie(\"cannot stat template %s\", template);\n> \n> \t\tif (S_ISDIR(st_template.st_mode)) {\n\nYuck; that's a bit too ugly for generic code. Will there be other places\nthat this needs to be used? If so, we'd probably need to encourage its\nuse where appropriate, which is even uglier but we cannot avoid it...\n\nAlso when the underlying system does not know the executable bit, how\nwould this help? I thought that earlier you said the part that checks if\nit wants to execute hooks with access(X_OK) will fail, so...\n"},{"id":"108354","messageId":"49C0A919.7070606@viscovery.net","threadId":"18354","inReplyTo":"7vmybjl1l6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Define a version of lstat(2) specially for copy operation","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-03-18T07:56:09Z","receivedAt":"2009-03-18T07:56:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Alex Riesen writes:\n> \n>> So that Cygwin port can continue work around its supporting\n>> library and get access to its faked file attributes.\n>>\n>> Signed-off-by: Alex Riesen \n>> ...\n>> diff --git a/builtin-init-db.c b/builtin-init-db.c\n>> index ee3911f..f3f781b 100644\n>> --- a/builtin-init-db.c\n>> +++ b/builtin-init-db.c\n>> @@ -66,7 +66,7 @@ static void copy_templates_1(char *path, int baselen,\n>> \t\telse\n>> \t\t\texists = 1;\n>> \n>> -\t\tif (lstat(template, &st_template))\n>> +\t\tif (lstat_for_copy(template, &st_template))\n>> \t\t\tdie(\"cannot stat template %s\", template);\n>> \n>> \t\tif (S_ISDIR(st_template.st_mode)) {\n> \n> Yuck; that's a bit too ugly for generic code. Will there be other places\n> that this needs to be used? If so, we'd probably need to encourage its\n> use where appropriate, which is even uglier but we cannot avoid it...\n> \n> Also when the underlying system does not know the executable bit, how\n> would this help? I thought that earlier you said the part that checks if\n> it wants to execute hooks with access(X_OK) will fail, so...\n\nThe \"underlying system\" in this case is Cygwin, and it *does* have an\nexecutable bit.\n\nBut the FS gymnastics that implement it are slow and affect all lstat()\ncalls, so we have replaced lstat() with a simpler and faster\nimplementation. Only that the replacement doesn't know about the X bit\nanymore; it always returns mode 0666.\n\nTherefore, if a file is created whose mode is influenced by the fast\nlstat(), then it will always be non-X. The access(, X_OK) call on the hook\nscript would do the right thing if only the script were created with the\ncorrect mode. access(, X_OK) fails because the file was created with non-X\npermissions.\n\n-- Hannes\n"},{"id":"108357","messageId":"7v8wn3i3ee.fsf@gitster.siamese.dyndns.org","threadId":"18354","inReplyTo":"49C0A919.7070606@viscovery.net","subject":"Re: [PATCH] Define a version of lstat(2) specially for copy operation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-18T09:30:49Z","receivedAt":"2009-03-18T09:30:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> The \"underlying system\" in this case is Cygwin, and it *does* have an\n> executable bit.\n>\n> But the FS gymnastics that implement it are slow and affect all lstat()\n> calls, so we have replaced lstat() with a simpler and faster\n> implementation. Only that the replacement doesn't know about the X bit\n> anymore; it always returns mode 0666.\n>\n> Therefore, if a file is created whose mode is influenced by the fast\n> lstat(), then it will always be non-X. The access(, X_OK) call on the hook\n> script would do the right thing if only the script were created with the\n> correct mode. access(, X_OK) fails because the file was created with non-X\n> permissions.\n\nOK. To make sure that I understand the situation better, allow me to\nparaphrase what you said.\n\nI've always thought core.filemode was about \"On FAT the filesystem does\nnot have x-bit and Cygwin fakes it by looking at .exe extension and using\nother heuristics\", but this lstat() \"trick\" is not about that. It is \"On\nWindows you need to issue multiple FS API calls in order to get full\ninformation to fill struct stat, which is expensive. We omit the one to\nlearn about the x-bit; it won't make a difference because most people run\nwith core.filemode.\"\n\nIf that is what is going on, I have a few quick questions:\n\n (1) How much of \"struct stat\" information can you get with the easiest\n and quickest FS API call on Windows? Does it give you big enough\n subset of what we store in the cache entry to detect modification\n reliably?\n\n (2) When you do an equivalent of \"chmod +x path\" on Windows, does it\n change one of the fields in your answer to (1)? In other words, can\n you detect such a change with the fastest and reliable-enough FS API\n call?\n\nIf answers to both questions are \"yes\", then I suspect we might be able to\nimprove the situation without sacrificing performance too much.\n\nIn the generic part, we use lstat(2) for various purposes:\n\n * To learn existence and type of a FS entity, in order to decide if we\n need to descend into a directory or treat it as a blob;\n\n * To learn the current \"status\" that can be compared with what is stored\n in the cache entry, in order to decide if the FS entity hasn't been\n modified;\n\n * To learn the last-modified timestamp, in order to implement the\n racy-git avoidance logic;\n\n * To learn x-bit, so that we can update it in the cache entry, and give\n the appropriate x-bit to a file that we create (Alex's\n lstat_for_copy() is about this usage).\n\nIf the first three can be learned with a fast-and-reliable-enough FS API\ncall, and the x-bit and perhaps something else that do not fall into the\nfirst three need a lot more work to figure out, we could split lstat()\ninto two. The result from a \"fast\" variant of lstat() is used for the\nfirst three and allow platform implementation of it to be incomplete\n(i.e. the Cygwin \"trick\" to omit x-bit is OK for that purpose), and add\nanother \"slow\" variant to complement it:\n\n\tint lstat_fast(const char *path, struct stat *st);\n void lstat_remainder(const char *path, struct stat *st);\n\nOn POSIX systems, we implement it as\n\n #define lstat_fast(path,st) lstat((path),(st))\n #define lstat_remainder(path,st) do { ; /* nothing */ } while (0)\n \nbut on Windows and Cygwin, we might implement the format to get\ngood-enough information for comparison purposes and implement latter to\nfill the x-bit and some other parts that are expensive to learn.\n\nMost of the callers that are only interested in seeing if a path has\nchanged can use the lstat_fast(), while the codepath that actually does\nuse the information for modified path can augment the information a\nprevious call to \"fast\" variant obtained with an additional call to the\n\"slow\" lstat_remainder().\n\nThe attached patch to convert a part of read-cache.c is only for\nillustration purposes.\n\nThe refresh_cache_ent() function first calls lstat_fast() to get cheap\ninformation that is good enough, gives it to ie_match_stat(), to see if\nanything has changed, and let fill_stat_cache_info() to update the cached\ninformation for later comparison.\n\nWe do not have to touch ce_match_stat_basic() that does care about x-bit\nbut assuming that the answer to question (2) is \"yes\", we would not miss\nMODE_CHANGED report from it; other information such as timestamps would\nhave changed in such a case as well, and ie_match_stat() that called\nce_match_stat_basic() will still say \"changed\" for such an entry.\n\nThe add_file_to_index() function checks if the path exists with the \"fast\"\none, gives the result to add_to_index() that wants the full mode bits\nhence invokes the \"slow\" one to fill in the remainder.\n\nIf you have to pass information between \"fast\" and \"slow\" variant other\nthan what you can put in \"struct stat\" in order for \"slow\" one to take\nadvantage of what \"fast\" one already has done, we would need to introduce\nsomething like\n\n\tstruct gitstat {\n \tstruct stat st;\n\t\tsome other info for platform;\n\t};\n\nand pass that around instead, which would become a larger change, though.\nI am hoping it won't be the case.\n\nAnd if we do not need such an extra \"struct gitstat\", then there is no\nreason for us to introduce lstat_fast(). We can just use lstat(), keep\nthe \"fast and incomplete\" Cygwin one as lstat(), but insert calls to\nlstat_remainder() at strategic places. Immediately before copy_file() is\ncalled in builtin-init-db.c::copy_templates_1() would be one of those\nstrategic places.\n\n\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 3f58711..d12d11b 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -562,7 +562,7 @@ static void record_intent_to_add(struct cache_entry *ce)\n int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)\n {\n \tint size, namelen, was_same;\n-\tmode_t st_mode = st->st_mode;\n+\tmode_t st_mode;\n \tstruct cache_entry *ce, *alias;\n \tunsigned ce_option = CE_MATCH_IGNORE_VALID|CE_MATCH_RACY_IS_DIRTY;\n \tint verbose = flags & (ADD_CACHE_VERBOSE | ADD_CACHE_PRETEND);\n@@ -571,6 +571,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \tint add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|\n \t\t\t (intent_only ? ADD_CACHE_NEW_ONLY : 0));\n \n+\tlstat_remainder(path, st);\n+\tst_mode = st->st_mode;\n \tif (!S_ISREG(st_mode) && !S_ISLNK(st_mode) && !S_ISDIR(st_mode))\n \t\treturn error(\"%s: can only add regular files, symbolic links or git-directories\", path);\n \n@@ -637,7 +639,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n int add_file_to_index(struct index_state *istate, const char *path, int flags)\n {\n \tstruct stat st;\n-\tif (lstat(path, &st))\n+\tif (lstat_fast(path, &st))\n \t\tdie(\"%s: unable to stat (%s)\", path, strerror(errno));\n \treturn add_to_index(istate, path, &st, flags);\n }\n@@ -1013,7 +1015,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \t\treturn ce;\n \t}\n \n-\tif (lstat(ce->name, &st) < 0) {\n+\tif (lstat_fast(ce->name, &st) < 0) {\n \t\tif (err)\n \t\t\t*err = errno;\n \t\treturn NULL;\n"},{"id":"108363","messageId":"49C0C983.5060602@viscovery.net","threadId":"18354","inReplyTo":"7v8wn3i3ee.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Define a version of lstat(2) specially for copy operation","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-03-18T10:14:27Z","receivedAt":"2009-03-18T10:14:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> I've always thought core.filemode was about \"On FAT the filesystem does\n> not have x-bit and Cygwin fakes it by looking at .exe extension and using\n> other heuristics\", but this lstat() \"trick\" is not about that. It is \"On\n> Windows you need to issue multiple FS API calls in order to get full\n> information to fill struct stat, which is expensive. We omit the one to\n> learn about the x-bit; it won't make a difference because most people run\n> with core.filemode.\"\n> \n> If that is what is going on, I have a few quick questions:\n> \n> (1) How much of \"struct stat\" information can you get with the easiest\n> and quickest FS API call on Windows? Does it give you big enough\n> subset of what we store in the cache entry to detect modification\n> reliably?\n\nYes.\n\n> (2) When you do an equivalent of \"chmod +x path\" on Windows, does it\n> change one of the fields in your answer to (1)? In other words, can\n> you detect such a change with the fastest and reliable-enough FS API\n> call?\n\nUnfortunately, no. That's why we have this discussion.\n\n> If answers to both questions are \"yes\", then I suspect we might be able to\n> improve the situation without sacrificing performance too much.\n> \n> In the generic part, we use lstat(2) for various purposes:\n> \n> * To learn existence and type of a FS entity, in order to decide if we\n> need to descend into a directory or treat it as a blob;\n\nThe fast version does not detect Cygwin's symbolic links, hence, not all\ntypes of FS entity that we are interested in are covered.\n\n> * To learn the current \"status\" that can be compared with what is stored\n> in the cache entry, in order to decide if the FS entity hasn't been\n> modified;\n> \n> * To learn the last-modified timestamp, in order to implement the\n> racy-git avoidance logic;\n> \n> * To learn x-bit, so that we can update it in the cache entry, and give\n> the appropriate x-bit to a file that we create (Alex's\n> lstat_for_copy() is about this usage).\n> \n> If the first three can be learned with a fast-and-reliable-enough FS API\n> call, and the x-bit and perhaps something else that do not fall into the\n> first three need a lot more work to figure out, we could split lstat()\n> into two. The result from a \"fast\" variant of lstat() is used for the\n> first three and allow platform implementation of it to be incomplete\n> (i.e. the Cygwin \"trick\" to omit x-bit is OK for that purpose), and add\n> another \"slow\" variant to complement it:\n\nSorry that I skip your elaboration on this. First, I barely understand\nread-cache.c; and second, I have the feeling that this is\nover-engineering: It introduces a genericity that we don't need in\npractice. We don't need it because Cygwin by default is built with\nNO_TRUSTABLE_FILEMODE, and the X-bit is uninteresting in this case anyway\n[*]. And there is no other platform that would make use of it. Not even\nthe MinGW port, because it wouldn't have a \"slow\" part, either.\n\n[*] Ok, Cygwin's slow part would also recognize symbolic links, but we\nhaven't heard complaints about the fast lstat() implementation in this\nrespect. Perhaps because we have an escape door for people who need it\n(core.ignorecygwinfstricks=false).\n\n> And if we do not need such an extra \"struct gitstat\", then there is no\n> reason for us to introduce lstat_fast(). We can just use lstat(), keep\n> the \"fast and incomplete\" Cygwin one as lstat(), but insert calls to\n> lstat_remainder() at strategic places. Immediately before copy_file() is\n> called in builtin-init-db.c::copy_templates_1() would be one of those\n> strategic places.\n\nBut that wouldn't be different in principle from Alex's patch that\nintroduced lstat_for_copy(), would it? Since it would be used in exactly\nthe same strategic places.\n\n-- Hannes\n"},{"id":"108408","messageId":"7vprgeek2o.fsf@gitster.siamese.dyndns.org","threadId":"18354","inReplyTo":"49C0C983.5060602@viscovery.net","subject":"Re: [PATCH] Define a version of lstat(2) specially for copy operation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-18T18:56:31Z","receivedAt":"2009-03-18T18:56:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Sorry that I skip your elaboration on this. First, I barely understand\n> read-cache.c; and second, I have the feeling that this is\n> over-engineering: It introduces a genericity that we don't need in\n> practice.\n\nThat's Ok. The whole speculation depended on your answer to (2) being\n\"yes\", and because it is not the case, you can safely skip it.\n\nI just wanted to see if we can sprinkle a handful of lstat_remainder()\ncalls that disappear on POSIX side without changing anything else, which\nwould be the least impact solution from my point of view.\n\n> But that wouldn't be different in principle from Alex's patch that\n> introduced lstat_for_copy(), would it? Since it would be used in exactly\n> the same strategic places.\n\nHm, I sort of agree.\n\nIf you imagine somebody with POSIX background who is new to git's codebase\nand is trying understand the resulting code, I thought lstat() followed by\nlstat_remainder() is conceptually slightly easier to explain (\"'remainder'\nis needed on platforms whose lstat() do not get the x-bit right, and we\nuse it only where it matters\"), but I do not think the difference is that\ngreat.\n\nYou need to explain when to add the \"remainder\" one after an existing\nlstat() call in this API, or you need to explain when to replace an\nexisting lstat() with \"for copy\" in the Alex's API. Either way you need\nto contaminate the generic codepath with the distasteful concept that \"On\nsome platforms, you lstat() may not do what you asked it to do\".\n\nAnd that was what I was unhappy about Alex's patch to begin with. Not the\n\"patch\" nor \"Alex\", but the fact that we have to deal with the \"issue\".\n\nAnd lstat() followed by lstat_remainder() does not solve that issue at\nall. So let's scrap this lstat_fast()/lstat_remainder().\n"}]}