{"thread":{"id":"8026","subject":"[PATCH] remove unnecessary loop","startedAt":"2007-05-08T03:18:31Z","lastAt":"2007-05-09T01:03:38Z","messageCount":9,"participants":["Liu Yubao","Junio C Hamano","Alex Riesen","Jan Hudec","Eric Blake"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"41400","messageId":"463FEC07.8080605@gmail.com","threadId":"8026","inReplyTo":null,"subject":"[PATCH] remove unnecessary loop","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2007-05-08T03:18:31Z","receivedAt":"2007-05-08T03:18:31Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Hi,\n Here is a minor optimization, the involved second \"for\" loop doesn't\nneed to start from beginning.\n\nSigned-off-by: Liu Yubao \n---\n builtin-add.c | 9 ++++-----\n 1 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-add.c b/builtin-add.c\nindex 5e6748f..9d10fdc 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -239,20 +239,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tdie(\"index file corrupt\");\n \n \tif (!ignored_too) {\n-\t\tint has_ignored = 0;\n \t\tfor (i = 0; i < dir.nr; i++)\n \t\t\tif (dir.entries[i]->ignored)\n-\t\t\t\thas_ignored = 1;\n-\t\tif (has_ignored) {\n+\t\t\t\tbreak;\n+\t\tif (i < dir.nr) {\n \t\t\tfprintf(stderr, ignore_warning);\n-\t\t\tfor (i = 0; i < dir.nr; i++) {\n+\t\t\tdo {\n \t\t\t\tif (!dir.entries[i]->ignored)\n \t\t\t\t\tcontinue;\n \t\t\t\tfprintf(stderr, \"%s\", dir.entries[i]->name);\n \t\t\t\tif (dir.entries[i]->ignored_dir)\n \t\t\t\t\tfprintf(stderr, \" (directory)\");\n \t\t\t\tfputc('\\n', stderr);\n-\t\t\t}\n+\t\t\t} while (++i < dir.nr);\n \t\t\tfprintf(stderr,\n \t\t\t\t\"Use -f if you really want to add them.\\n\");\n \t\t\texit(1);\n-- \n1.5.2.rc0.95.ga0715-dirty\n"},{"id":"41414","messageId":"4640015F.1080407@gmail.com","threadId":"8026","inReplyTo":"463FEC07.8080605@gmail.com","subject":"Re: [PATCH] remove unnecessary loop","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2007-05-08T04:49:35Z","receivedAt":"2007-05-08T04:49:35Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Liu Yubao wrote:\n> Hi,\n> Here is a minor optimization, the involved second \"for\" loop doesn't\n> need to start from beginning.\n> \n\nI found it when I debugged a strange problem on Cygwin, at last, I think\nit's a bug of Cygwin.\n\n$ touch hello.exe\n$ git add hello\nThe following paths are ignored by one of your .gitignore files:\nhello\nUse -f if you really want to add them.\n\nHere is a ugly fix, I don't hope it will be merged into git tree as it's not\ngit's fault, I will file a bug report for Cygwin.\n\n---\n builtin-add.c | 14 +++++++++++++-\n 1 files changed, 13 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-add.c b/builtin-add.c\nindex 9d10fdc..ff1e74f 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -42,6 +42,9 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p\n \tfor (i = 0; i < specs; i++) {\n \t\tstruct stat st;\n \t\tconst char *match;\n+#ifdef __CYGWIN__\n+\t\tint fd;\n+#endif\n \t\tif (seen[i])\n \t\t\tcontinue;\n \n@@ -50,9 +53,18 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p\n \t\t\tcontinue;\n \n \t\t/* Existing file? We must have ignored it */\n+#ifdef __CYGWIN__\n+\t\t/*\n+\t\t * On cygwin, lstat(\"hello\", &st) returns 0 when\n+\t\t * \"hello.exe\" exists, so test with open() again.\n+\t\t */\n+\t\tif (lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {\n+\t\t\tstruct dir_entry *ent;\n+\t\t\tclose(fd);\n+#else\n \t\tif (!lstat(match, &st)) {\n \t\t\tstruct dir_entry *ent;\n-\n+#endif\n \t\t\tent = dir_add_name(dir, match, strlen(match));\n \t\t\tent->ignored = 1;\n \t\t\tif (S_ISDIR(st.st_mode))\n-- \n1.5.2.rc0.95.ga0715-dirty\n"},{"id":"41416","messageId":"7virb352ha.fsf@assigned-by-dhcp.cox.net","threadId":"8026","inReplyTo":"4640015F.1080407@gmail.com","subject":"Re: [PATCH] remove unnecessary loop","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-08T05:05:53Z","receivedAt":"2007-05-08T05:05:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Liu Yubao writes:\n\n> Here is a ugly fix, I don't hope it will be merged into git tree as it's not\n> git's fault, I will file a bug report for Cygwin.\n\n> @@ -50,9 +53,18 @@ static void prune_directory(struct dir_struct *dir, const char **pathspec, int p\n> \t\t\tcontinue;\n> \n> \t\t/* Existing file? We must have ignored it */\n> +#ifdef __CYGWIN__\n> +\t\t/*\n> +\t\t * On cygwin, lstat(\"hello\", &st) returns 0 when\n> +\t\t * \"hello.exe\" exists, so test with open() again.\n> +\t\t */\n> +\t\tif (lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {\n> +\t\t\tstruct dir_entry *ent;\n> +\t\t\tclose(fd);\n> +#else\n\nWe have lstat() everywhere, so if we were to work this around\nwithout (or \"waiting for\") a proper fix on the Cygwin side, you\nwould be better off wrapping the above sequence in a separate\nfunction (say \"sane_lstat()\"), and do\n\n\t#ifdef __CYGWIN__\n #define lstat(a,b) sane_lstat(a,b)\n #endif\n\nsomewhere near the top of git-compat-util.h\n"},{"id":"41430","messageId":"81b0412b0705080208x3713cbc1y3c870383b586c877@mail.gmail.com","threadId":"8026","inReplyTo":"4640015F.1080407@gmail.com","subject":"Re: [PATCH] remove unnecessary loop","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-08T09:08:35Z","receivedAt":"2007-05-08T09:08:35Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/8/07, Liu Yubao wrote:\n> +#ifdef __CYGWIN__\n> + /*\n> + * On cygwin, lstat(\"hello\", &st) returns 0 when\n> + * \"hello.exe\" exists, so test with open() again.\n> + */\n> + if (lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {\n\nThis does not \"test again\" if lstat returns 0. If lstat returns 0\n(file stat info\nobtained) the open is not even called. Besides, cygwin lies not only about\n.exe but also about .lnk files.\n\nP.S. Somehow I have the feeling that even if it is a stupidity in cygwin\nthey will not fix it (nor will they admit it is a bug).\n"},{"id":"41431","messageId":"20070508093902.GB9007@efreet.light.src","threadId":"8026","inReplyTo":"4640015F.1080407@gmail.com","subject":"Re: [PATCH] remove unnecessary loop","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2007-05-08T09:39:02Z","receivedAt":"2007-05-08T09:39:02Z","isPatch":true,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"On Tue, May 08, 2007 at 12:49:35 +0800, Liu Yubao wrote:\n> +#ifdef __CYGWIN__\n> +\t\t/*\n> +\t\t * On cygwin, lstat(\"hello\", &st) returns 0 when\n> +\t\t * \"hello.exe\" exists, so test with open() again.\n> +\t\t */\n> +\t\tif (lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {\n> +\t\t\tstruct dir_entry *ent;\n> +\t\t\tclose(fd);\n> +#else\n> \t\tif (!lstat(match, &st)) {\n> \t\t\tstruct dir_entry *ent;\n> -\n> +#endif\n\nYou seem to have reversed the sense of the test.\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec \n"},{"id":"41436","messageId":"20070508101317.GC9007@efreet.light.src","threadId":"8026","inReplyTo":"81b0412b0705080208x3713cbc1y3c870383b586c877@mail.gmail.com","subject":"Re: [PATCH] remove unnecessary loop","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2007-05-08T10:13:17Z","receivedAt":"2007-05-08T10:13:17Z","isPatch":true,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"On Tue, May 08, 2007 at 11:08:35 +0200, Alex Riesen wrote:\n> On 5/8/07, Liu Yubao wrote:\n> >+#ifdef __CYGWIN__\n> >+ /*\n> >+ * On cygwin, lstat(\"hello\", &st) returns 0 when\n> >+ * \"hello.exe\" exists, so test with open() again.\n> >+ */\n> >+ if (lstat(match, &st) && -1 != (fd = open(match, \n> >O_RDONLY))) {\n> \n> This does not \"test again\" if lstat returns 0. If lstat returns 0\n> (file stat info\n> obtained) the open is not even called. Besides, cygwin lies not only about\n> .exe but also about .lnk files.\n> \n> P.S. Somehow I have the feeling that even if it is a stupidity in cygwin\n> they will not fix it (nor will they admit it is a bug).\n\nThey will not. Because it is not a bug. It seems to be (part of) workaround\nto get programs written for unix work in windows.\n\nOne reason for such workaround I can think of is, that some programs try to\nfind themselves and since their argv[0] often does NOT contain the extension,\nthe stat has to succeed for them.\n\nUsing open here unfortunately won't work though, because:\n - For stale links open will fail, but the lstat should succeed. This does\n apply to cygwin, because cygwin emulates links.\n - I'd expect open to actually succeed in this case, because there are\n programs that don't only try to find themselves, but also open themselves,\n because they bundle some data.\n\nAnother problem is, that the file might exist or might be cygwin artefact and\nthere does not seem to be an easy way to tell.\n\nIMHO the described problem is harmless (you know the file does not exist, so\nyou should have no reason to add it and nothing happens if you don't) and\nhappens very rarely (adding binaries to version control is usually not a good\nidea), so I suggest to let this be, as the workaround can easily cause other\nproblems.\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec \n"},{"id":"41445","messageId":"46406A47.7050400@byu.net","threadId":"8026","inReplyTo":"81b0412b0705080208x3713cbc1y3c870383b586c877@mail.gmail.com","subject":"Re: [PATCH] remove unnecessary loop","fromName":"Eric Blake","fromEmail":"ebb9@byu.net","sentAt":"2007-05-08T12:17:11Z","receivedAt":"2007-05-08T12:17:11Z","isPatch":true,"sender":{"key":"eblake@redhat.com","avatar":"https://avatars.githubusercontent.com/u/32933908?v=4"},"body":"-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA1\n\nAccording to Alex Riesen on 5/8/2007 3:08 AM:\n> This does not \"test again\" if lstat returns 0. If lstat returns 0\n> (file stat info\n> obtained) the open is not even called. Besides, cygwin lies not only about\n> .exe but also about .lnk files.\n> \n> P.S. Somehow I have the feeling that even if it is a stupidity in cygwin\n> they will not fix it (nor will they admit it is a bug).\n\nIt is a limitation of cygwin, and the cygwin developers will admit it; but\nthey will also stand behind calling it a feature rather than a bug due to\nthe attempts to make cygwin behave more like Linux in spite of Window's\ninsistence on file suffixes. The cygwin port of coreutils has to do\nsimilar stat() tricks to reverse engineer some of the .exe magic present\nin cygwin. However, it is possible to override the magic without\nresorting to a full-blown open(), via careful use of additional stat()s or\nreadlink()s (trailing . is not legal in Windows, and on cygwin is only\nlegal on managed mounts, so stat(\"foo.\") will fail when stat(\"foo\")\nsucceeds if the reason stat(\"foo\") succeeded was due only to the existence\nof foo.exe):\n\n/* Return -1 if PATH not found, 0 if PATH spelled correctly, and 1 if PATH\n had \".exe\" automatically appended by cygwin. Don't change errno. */\nint\ncygwin_spelling (char const *path)\n{\n char path_exact[PATH_MAX + 9];\n int saved_errno = errno;\n int result = 0; /* Start with assumption that PATH is okay. */\n int len = strlen (path);\n\n if (! path || ! *path || len > PATH_MAX)\n /* PATH will cause EINVAL or ENAMETOOLONG, treat it as non-existing. */\n return -1;\n if (path[len - 1] == '.' || path[len-1] == '/')\n /* Don't change spelling if there is a trailing `.' or `/'. */\n return 0;\n if (readlink (path, NULL, 0) < 0)\n { /* PATH is not a symlink. */\n if (errno == EINVAL)\n\t{ /* PATH exists. Appending trailing `.' exposes whether it is\n\t PATH or PATH.exe for normal disk files, but also check appending\n\t trailing `.exe' to be sure on virtual/managed directories. */\n\t strcat (strcpy (path_exact, path), \".\");\n\t if (access (path_exact, F_OK) < 0)\n\t { /* PATH. does not exist. */\n\t strcat (path_exact, \"exe\");\n\t if (access (path_exact, F_OK) == 0)\n\t\t/* But PATH.exe does, so append .exe. */\n\t\tresult = 1;\n\t }\n\t}\n else\n\t/* PATH does not exist. */\n\tresult = -1;\n }\n else\n { /* PATH is a symlink. Appending trailing `.lnk' exposes whether\n\t it is PATH.lnk or PATH.exe.lnk; but does not help with\n\t old-style symlinks where it was just PATH and the system\n\t attribute set. */\n strcat (strcpy (path_exact, path), \".lnk\");\n if (readlink (path_exact, NULL, 0) < 0)\n\t{\n\t strcat (strcpy (path_exact, path), \".exe.lnk\");\n\t if (readlink (path_exact, NULL, 0) == 0)\n\t result = 1;\n\t}\n }\n\n errno = saved_errno;\n return result;\n}\n\n\nIn the upcoming cygwin 1.7.0, you can set CYGWIN=transparent_exe which\nwill cause ENOENT when dealing with any explicit .exe. When enabled, that\nwill make it impossible to have both foo and foo.exe in the current\ndirectory, and make it so that stat can never lie - stat(\"foo.exe\") will\nfail, and if stat(\"foo\") succeeds, you no longer care if it succeeded\nbecause of the Windows file foo or because of foo.exe, because the .exe is\ntransparent to cygwin.\n\n- --\nDon't work too hard, make some time for fun as well!\n\nEric Blake ebb9@byu.net\n-----BEGIN PGP SIGNATURE-----\nVersion: GnuPG v1.4.5 (Cygwin)\nComment: Public key at home.comcast.net/~ericblake/eblake.gpg\nComment: Using GnuPG with Mozilla - http://enigmail.mozdev.org\n\niD8DBQFGQGpH84KuGfSFAYARAsCdAKCmqdgsppPY0MhxDWZ6QQxXExn2gwCeLN39\nZl3sRk/0IkkHkIyjf4RpAAA=\n=rQrT\n-----END PGP SIGNATURE-----\n"},{"id":"41448","messageId":"81b0412b0705080538r2691d232r9c073bd73934b1d8@mail.gmail.com","threadId":"8026","inReplyTo":"20070508101317.GC9007@efreet.light.src","subject":"Re: [PATCH] remove unnecessary loop","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-08T12:38:01Z","receivedAt":"2007-05-08T12:38:01Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 5/8/07, Jan Hudec wrote:\n> >\n> > P.S. Somehow I have the feeling that even if it is a stupidity in cygwin\n> > they will not fix it (nor will they admit it is a bug).\n>\n> They will not. Because it is not a bug. It seems to be (part of) workaround\n> to get programs written for unix work in windows.\n>\n\nJust as I said. Why don't you just realize that windows is plainly\nstupid, illogical piece of sh%t and state clearly that people have to\nbreak their programs so-and-so to work there? Instead, everyone\nhas to put the most stupid workarounds POSIX ever seen in their\ncode just to get core functionality (which even HP-UX got right).\n"},{"id":"41524","messageId":"46411DEA.6060404@gmail.com","threadId":"8026","inReplyTo":"20070508093902.GB9007@efreet.light.src","subject":"Re: [PATCH] remove unnecessary loop","fromName":"Liu Yubao","fromEmail":"yubao.liu@gmail.com","sentAt":"2007-05-09T01:03:38Z","receivedAt":"2007-05-09T01:03:38Z","isPatch":true,"sender":{"key":"yubao.liu@gmail.com","avatar":null},"body":"Jan Hudec wrote:\n> On Tue, May 08, 2007 at 12:49:35 +0800, Liu Yubao wrote:\n>> +#ifdef __CYGWIN__\n>> +\t\t/*\n>> +\t\t * On cygwin, lstat(\"hello\", &st) returns 0 when\n>> +\t\t * \"hello.exe\" exists, so test with open() again.\n>> +\t\t */\n>> +\t\tif (lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {\n>> +\t\t\tstruct dir_entry *ent;\n>> +\t\t\tclose(fd);\n>> +#else\n>> \t\tif (!lstat(match, &st)) {\n>> \t\t\tstruct dir_entry *ent;\n>> -\n>> +#endif\n> \n> You seem to have reversed the sense of the test.\n> \nSorry I made a mistake, Junio's suggestion is pretty clean, and\nthat test should be\n\t\tif (!lstat(match, &st) && -1 != (fd = open(match, O_RDONLY))) {\n\nYesterday I digged the Cygwin mail archive, I found it's a concession for windows\nas you said in the previous message. I agree with you, just let it be.\n\nOnce more, I get the lesson: Windows is poor, sigh...\n"}]}