{"thread":{"id":"6852","subject":"[PATCH] Fix incorrect diff of a link -> file change if core.filemode = false.","startedAt":"2007-02-16T23:09:02Z","lastAt":"2007-02-17T17:49:00Z","messageCount":7,"participants":["Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"34838","messageId":"200702170009.02500.johannes.sixt@telecom.at","threadId":"6852","inReplyTo":null,"subject":"[PATCH] Fix incorrect diff of a link -> file change if core.filemode = false.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-02-16T23:09:02Z","receivedAt":"2007-02-16T23:09:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"After this sequence:\n\n$ ln -s foo A\n$ git add A\n$ git commit -m link\n$ rm A && echo bar > A\n\nthe working copy contains a regular file A but HEAD contains A as a link.\n\nNormally, at this point 'git diff HEAD' displays this change as a removal\nof the link followed by an addition of a new file. But in a repository where\ncore.filemode is false this is displayed as a modification of the link.\nThe reason is that the check when the cached mode is allowed to override the\nfile's actual mode is not strict enough.\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n diff-lib.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 91cd877..5fc1910 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -171,7 +171,8 @@ static int get_stat_data(struct cache_entry *ce,\n \t\tchanged = ce_match_stat(ce, &st, 0);\n \t\tif (changed) {\n \t\t\tmode = create_ce_mode(st.st_mode);\n-\t\t\tif (!trust_executable_bit && S_ISREG(st.st_mode))\n+\t\t\tif (!trust_executable_bit &&\n+\t\t\t    S_ISREG(st.st_mode) && S_ISREG(ntohl(ce->ce_mode)))\n \t\t\t\tmode = ce->ce_mode;\n \t\t\tsha1 = no_sha1;\n \t\t}\n-- \n1.5.0.19.gddff\n"},{"id":"34839","messageId":"7vejop1ysu.fsf@assigned-by-dhcp.cox.net","threadId":"6852","inReplyTo":"200702170009.02500.johannes.sixt@telecom.at","subject":"Re: [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-16T23:13:21Z","receivedAt":"2007-02-16T23:13:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> After this sequence:\n>\n> $ ln -s foo A\n> $ git add A\n> $ git commit -m link\n> $ rm A && echo bar > A\n>\n> the working copy contains a regular file A but HEAD contains A as a link.\n>\n> Normally, at this point 'git diff HEAD' displays this change as a removal\n> of the link followed by an addition of a new file. But in a repository where\n> core.filemode is false this is displayed as a modification of the link.\n> The reason is that the check when the cached mode is allowed to override the\n> file's actual mode is not strict enough.\n>\n> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n\nI do not follow.  core.filemode aka trust_executable_bit is\nabout executable bit and not symlink.\n"},{"id":"34841","messageId":"200702170030.04014.johannes.sixt@telecom.at","threadId":"6852","inReplyTo":"7vejop1ysu.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-02-16T23:30:03Z","receivedAt":"2007-02-16T23:30:03Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Saturday 17 February 2007 00:13, Junio C Hamano wrote:\n> Johannes Sixt <johannes.sixt@telecom.at> writes:\n> > After this sequence:\n> >\n> > $ ln -s foo A\n> > $ git add A\n> > $ git commit -m link\n> > $ rm A && echo bar > A\n> >\n> > the working copy contains a regular file A but HEAD contains A as a link.\n> >\n> > Normally, at this point 'git diff HEAD' displays this change as a removal\n> > of the link followed by an addition of a new file. But in a repository\n> > where core.filemode is false this is displayed as a modification of the\n> > link. The reason is that the check when the cached mode is allowed to\n> > override the file's actual mode is not strict enough.\n> >\n> > Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n>\n> I do not follow.  core.filemode aka trust_executable_bit is\n> about executable bit and not symlink.\n\nCorrect. But the breakage is still there. As you recall, the properties \nS_IFREG and S_IFLNK are recorded in the st_mode, and the line of code that I \nfixed trusts the index in too many cases, and copies the index's S_IFLNK over \nthe the stat() result, which said S_IFREG. So the diff engine loses the \ninformation that it is should diff against a regular file, and thinks that \nthe working copy is a symlink.\n\nPlease change the last sentence of the commit message, which hopefully makes \nthe issue a bit clearer:\n\nThe fix is that the cached mode must only be allowed to override the file's \nactual mode (which includes the type information) if _both_ the working tree \nentry and the cached entry are regular files.\n\n-- Hannes\n"},{"id":"34843","messageId":"7vodntzljb.fsf@assigned-by-dhcp.cox.net","threadId":"6852","inReplyTo":"200702170030.04014.johannes.sixt@telecom.at","subject":"Re: [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-17T00:15:52Z","receivedAt":"2007-02-17T00:15:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> The fix is that the cached mode must only be allowed to override the file's \n> actual mode (which includes the type information) if _both_ the working tree \n> entry and the cached entry are regular files.\n\nAh, I misunderstood.\n\nI suspect you do not want to get random, unreliable executable\nbit from lstat(), even when previously index recorded non\nregular file for a path that is now a regular file.  In\nbuiltin-update-index.c, add_file_to_cache() has this:\n\n\tce->ce_mode = create_ce_mode(st.st_mode);\n\tif (!trust_executable_bit) {\n\t\t/* If there is an existing entry, pick the mode bits\n\t\t * from it, otherwise assume unexecutable.\n\t\t */\n\t\tint pos = cache_name_pos(path, namelen);\n\t\tif (0 <= pos)\n\t\t\tce->ce_mode = active_cache[pos]->ce_mode;\n\t\telse if (S_ISREG(st.st_mode))\n\t\t\tce->ce_mode = create_ce_mode(S_IFREG | 0666);\n\t}\n\nand I agree what it _tries_ to do, although I think the above\nalso makes the regular file being added to a symlink and needs\nto be fixed.  You helped us find another bug.\n\n\t# In a new, empty directory...\n\t$ git init\n        $ ln -s foo A\n        $ git add A\n        $ git ls-files -s\n\t120000 19102815663d23f8b75a47e7a01965dcdc96468c 0\tA\n        $ git config core.filemode false\n        $ rm A ; echo foo >A\n        $ git add A\n        $ git ls-files -s\n\t120000 19102815663d23f8b75a47e7a01965dcdc96468c 0\tA\n\nThe fix is probably like this:\n\n\tce->ce_mode = create_ce_mode(st.st_mode);\n\tif (!trust_executable_bit && S_ISREG(st.st_mode)) {\n\t\t/* If there is an existing entry, pick the mode bits\n\t\t * from it, otherwise assume unexecutable.\n\t\t */\n\t\tint pos = cache_name_pos(path, namelen);\n\t\tif (0 <= pos &&\n                    S_ISREG(ntohl(active_cache[pos]->ce_mode)))\n\t\t\tce->ce_mode = active_cache[pos]->ce_mode;\n\t\telse \n\t\t\tce->ce_mode = create_ce_mode(S_IFREG | 0666);\n\t}\n\nBack to the part of the code you patched, I think the fix should\nbe something like this instead:\n\n\tmode = create_ce_mode(st.st_mode);\n\tif (!trust_executable_bit && S_ISREG(st.st_mode)) {\n\t\t/* If there is an existing entry, pick the mode bits\n\t\t * from it, otherwise assume unexecutable.\n\t\t */\n                if (S_ISREG(ntohl(ce->ce_mode)))\n\t\t\tmode = ce->ce_mode;\n                else\n                \tmode = create_ce_mode(S_IFREG | 0666);\n                \n        }\n"},{"id":"34850","messageId":"7vodntw9u7.fsf@assigned-by-dhcp.cox.net","threadId":"6852","inReplyTo":"7vodntzljb.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Do not take mode bits from index after type change.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-17T06:56:48Z","receivedAt":"2007-02-17T06:56:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When we do not trust executable bit from lstat(2), we copied\nexisting ce_mode bits without checking if the filesystem object\nis a regular file (which is the only thing we apply the \"trust\nexecutable bit\" business) nor if the blob in the index is a\nregular file (otherwise, we should do the same as registering a\nnew regular file, which is to default non-executable).\n\nNoticed by Johannes Sixt.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n * Adds a test -- how about this as a replacement?\n\n builtin-apply.c        |    2 +-\n builtin-update-index.c |   13 +++++++------\n cache.h                |   10 ++++++++++\n diff-lib.c             |    4 +---\n read-cache.c           |   13 +++++++------\n t/t3700-add.sh         |   20 ++++++++++++++++++++\n 6 files changed, 46 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 3fefdac..abe3538 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1988,7 +1988,7 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)\n \t\t\treturn error(\"%s: %s\", old_name, strerror(errno));\n \n \t\tif (!cached)\n-\t\t\tst_mode = ntohl(create_ce_mode(st.st_mode));\n+\t\t\tst_mode = ntohl(ce_mode_from_stat(ce, st.st_mode));\n \n \t\tif (patch->is_new < 0)\n \t\t\tpatch->is_new = 0;\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex 1ac613a..772aaba 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -109,16 +109,17 @@ static int add_file_to_cache(const char *path)\n \tce->ce_flags = htons(namelen);\n \tfill_stat_cache_info(ce, &st);\n \n-\tce->ce_mode = create_ce_mode(st.st_mode);\n-\tif (!trust_executable_bit) {\n+\tif (trust_executable_bit)\n+\t\tce->ce_mode = create_ce_mode(st.st_mode);\n+\telse {\n \t\t/* If there is an existing entry, pick the mode bits\n \t\t * from it, otherwise assume unexecutable.\n \t\t */\n+\t\tstruct cache_entry *ent;\n \t\tint pos = cache_name_pos(path, namelen);\n-\t\tif (0 <= pos)\n-\t\t\tce->ce_mode = active_cache[pos]->ce_mode;\n-\t\telse if (S_ISREG(st.st_mode))\n-\t\t\tce->ce_mode = create_ce_mode(S_IFREG | 0666);\n+\n+\t\tent = (0 <= pos) ? active_cache[pos] : NULL;\n+\t\tce->ce_mode = ce_mode_from_stat(ent, st.st_mode);\n \t}\n \n \tif (index_path(ce->sha1, path, &st, !info_only))\ndiff --git a/cache.h b/cache.h\nindex c62b0b0..04f8e63 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -106,6 +106,16 @@ static inline unsigned int create_ce_mode(unsigned int mode)\n \t\treturn htonl(S_IFLNK);\n \treturn htonl(S_IFREG | ce_permissions(mode));\n }\n+static inline unsigned int ce_mode_from_stat(struct cache_entry *ce, unsigned int mode)\n+{\n+\textern int trust_executable_bit;\n+\tif (!trust_executable_bit && S_ISREG(mode)) {\n+\t\tif (ce && S_ISREG(ntohl(ce->ce_mode)))\n+\t\t\treturn ce->ce_mode;\n+\t\treturn create_ce_mode(0666);\n+\t}\n+\treturn create_ce_mode(mode);\n+}\n #define canon_mode(mode) \\\n \t(S_ISREG(mode) ? (S_IFREG | ce_permissions(mode)) : \\\n \tS_ISLNK(mode) ? S_IFLNK : S_IFDIR)\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 91cd877..556d534 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -170,9 +170,7 @@ static int get_stat_data(struct cache_entry *ce,\n \t\t}\n \t\tchanged = ce_match_stat(ce, &st, 0);\n \t\tif (changed) {\n-\t\t\tmode = create_ce_mode(st.st_mode);\n-\t\t\tif (!trust_executable_bit && S_ISREG(st.st_mode))\n-\t\t\t\tmode = ce->ce_mode;\n+\t\t\tmode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\tsha1 = no_sha1;\n \t\t}\n \t}\ndiff --git a/read-cache.c b/read-cache.c\nindex c54a611..605b352 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -344,16 +344,17 @@ int add_file_to_index(const char *path, int verbose)\n \tce->ce_flags = htons(namelen);\n \tfill_stat_cache_info(ce, &st);\n \n-\tce->ce_mode = create_ce_mode(st.st_mode);\n-\tif (!trust_executable_bit) {\n+\tif (trust_executable_bit)\n+\t\tce->ce_mode = create_ce_mode(st.st_mode);\n+\telse {\n \t\t/* If there is an existing entry, pick the mode bits\n \t\t * from it, otherwise assume unexecutable.\n \t\t */\n+\t\tstruct cache_entry *ent;\n \t\tint pos = cache_name_pos(path, namelen);\n-\t\tif (pos >= 0)\n-\t\t\tce->ce_mode = active_cache[pos]->ce_mode;\n-\t\telse if (S_ISREG(st.st_mode))\n-\t\t\tce->ce_mode = create_ce_mode(S_IFREG | 0666);\n+\n+\t\tent = (0 <= pos) ? active_cache[pos] : NULL;\n+\t\tce->ce_mode = ce_mode_from_stat(ent, st.st_mode);\n \t}\n \n \tif (index_path(ce->sha1, path, &st, 1))\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex caaab26..08e0352 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -30,6 +30,16 @@ test_expect_success \\\n \t *) echo fail; git-ls-files --stage xfoo1; (exit 1);;\n \t esac'\n \n+test_expect_success 'git-add: filemode=0 should not get confused by symlink' '\n+\trm -f xfoo1 &&\n+\tln -s foo xfoo1 &&\n+\tgit-add xfoo1 &&\n+\tcase \"`git-ls-files --stage xfoo1`\" in\n+\t120000\" \"*xfoo1) echo ok;;\n+\t*) echo fail; git-ls-files --stage xfoo1; (exit 1);;\n+\tesac\n+'\n+\n test_expect_success \\\n \t'git-update-index --add: Test that executable bit is not used...' \\\n \t'git config core.filemode 0 &&\n@@ -41,6 +51,16 @@ test_expect_success \\\n \t *) echo fail; git-ls-files --stage xfoo2; (exit 1);;\n \t esac'\n \n+test_expect_success 'git-add: filemode=0 should not get confused by symlink' '\n+\trm -f xfoo2 &&\n+\tln -s foo xfoo2 &&\n+\tgit update-index --add xfoo2 &&\n+\tcase \"`git-ls-files --stage xfoo2`\" in\n+\t120000\" \"*xfoo2) echo ok;;\n+\t*) echo fail; git-ls-files --stage xfoo2; (exit 1);;\n+\tesac\n+'\n+\n test_expect_success \\\n \t'git-update-index --add: Test that executable bit is not used...' \\\n \t'git config core.filemode 0 &&\n-- \n1.5.0.552.ge1b1c\n"},{"id":"34865","messageId":"200702171131.07304.johannes.sixt@telecom.at","threadId":"6852","inReplyTo":"7vodntw9u7.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Do not take mode bits from index after type change.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-02-17T10:31:07Z","receivedAt":"2007-02-17T10:31:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Your patch works here, too. Thanks!\n\nI'd appreciate if you could publish it soon since it has a few conflicts with \nmy current \"don't trust symlinks\" work.\n\n-- Hannes\n"},{"id":"34877","messageId":"7vd548vfn7.fsf@assigned-by-dhcp.cox.net","threadId":"6852","inReplyTo":"200702171131.07304.johannes.sixt@telecom.at","subject":"Re: [PATCH] Do not take mode bits from index after type change.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-17T17:49:00Z","receivedAt":"2007-02-17T17:49:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> Your patch works here, too. Thanks!\n>\n> I'd appreciate if you could publish it soon since it has a few\n> conflicts with my current \"don't trust symlinks\" work.\n\nI think this bugfix should go to 'maint' and in 'master'.  I\ntried to be careful, but extra sets of eyeballs to double-check\nthat I did not break anything are certainly appreciated.\n"}]}