{"thread":{"id":"25527","subject":"git-update-index loses executable bit for unmerged files when core.filemode is false","startedAt":"2010-10-22T17:28:47Z","lastAt":"2010-10-25T08:59:11Z","messageCount":6,"participants":["Stefan Haller","Jakub Narebski","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"154173","messageId":"1jqpu2f.1qxnixxtdqhreM%lists@haller-berlin.de","threadId":"25527","inReplyTo":null,"subject":"git-update-index loses executable bit for unmerged files when core.filemode is false","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2010-10-22T17:28:47Z","receivedAt":"2010-10-22T17:28:47Z","isPatch":false,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"There's a bug with the handling of the executable bit when core.filemode\nis false: when you have an executable file that has unmerged changes,\nand you stage it with \"git update-index\", the executable bit is lost.\nIf you stage it with \"git add\" instead, it works fine.\n\n    git init test\n    cd test\n    git config core.filemode false\n    touch foo\n    git add foo\n    git update-index --chmod=+x foo\n    git commit -m \"Initial revision\"\n    git branch br\n    echo bla >foo\n    git commit -a -m \"Changed foo\"\n    git checkout br\n    echo blubb >foo\n    git commit -a -m \"Changed foo\"\n    git merge master\n    git update-index foo\n    git diff --staged\n\nSee how the diff shows that the mode goes from 100755 to 100644.  If you\nreplace the second-to-last line with \"git add foo\", it doesn't.\n\nI started to trace this down, but I didn't get very far, as I'm not\nfamiliar enough with the git codebase yet; so any help would be\nappreciated.\n\n\n-- \nStefan Haller\nBerlin, Germany\nhttp://www.haller-berlin.de/\n"},{"id":"154312","messageId":"1jqvbx3.1icsj8j1jf26lfM%lists@haller-berlin.de","threadId":"25527","inReplyTo":"1jqpu2f.1qxnixxtdqhreM%lists@haller-berlin.de","subject":"[PATCH] Add tests to demonstrate update-index bug with core.symlinks/core.filemode","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2010-10-24T11:40:04Z","receivedAt":"2010-10-24T11:40:04Z","isPatch":true,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"When calling update-index on an unmerged file that is executable and\ncore.filemode is false, or that is a symlink and core.symlink is false,\nthe executable bit or the symlink property is lost.\n---\nStefan Haller <lists@haller-berlin.de> wrote:\n\n> There's a bug with the handling of the executable bit when core.filemode\n> is false: when you have an executable file that has unmerged changes,\n> and you stage it with \"git update-index\", the executable bit is lost.\n> If you stage it with \"git add\" instead, it works fine.\n\nIt turns out that the same bug exists for symlinks when core.symlink is\nfalse. Here's a patch that adds two tests that demonstrate the problems.\n(I suspect both have a similar cause, and/or a similar solution.)\n\nThis is the first time I write a git test, so please point out anything\nI might have done wrong. Also, I still don't have much of an idea how or\nwhere to fix the problem, so any guidance towards that is much\nappreciated.\n\n t/t2107-update-index-executable-bit-merged.sh |   44 +++++++++++++++++++++++++\n t/t2108-update-index-symlink-merged.sh        |   43 ++++++++++++++++++++++++\n 2 files changed, 87 insertions(+), 0 deletions(-)\n create mode 100755 t/t2107-update-index-executable-bit-merged.sh\n create mode 100755 t/t2108-update-index-symlink-merged.sh\n\ndiff --git a/t/t2107-update-index-executable-bit-merged.sh b/t/t2107-update-index-executable-bit-merged.sh\nnew file mode 100755\nindex 0000000..7a8f740\n--- /dev/null\n+++ b/t/t2107-update-index-executable-bit-merged.sh\n@@ -0,0 +1,44 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2010 Stefan Haller\n+#\n+\n+test_description='git update-index on filesystem w/o symlinks test.\n+\n+This tests that git update-index keeps the executable bit when staging\n+an unmerged file after a merge if core.filemode is false.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success \\\n+'preparation' '\n+git config core.filemode false &&\n+touch foo &&\n+git add foo &&\n+git update-index --chmod=+x foo &&\n+git commit -m \"Create\"'\n+\n+test_expect_success \\\n+'modify the file on two branches and merge' '\n+git branch br &&\n+test_commit \"Modify_on_master\" foo &&\n+git checkout br -- &&\n+test_commit \"Modify_on_branch\" foo &&\n+test_must_fail git merge master'\n+\n+test_expect_success \\\n+'double-check that file is indeed unmerged' '\n+git ls-files --unmerged --error-unmatch -- foo'\n+\n+test_expect_success \\\n+'stage unmerged file with update-index' '\n+git update-index -- foo'\n+\n+test_expect_failure \\\n+'check that filemode is still 100755' '\n+case \"`git ls-files --stage --cached -- foo`\" in\n+\"100755 \"*foo) echo pass;;\n+*) echo fail; git ls-files --stage --cached -- foo; (exit 1);;\n+esac'\n+\n+test_done\ndiff --git a/t/t2108-update-index-symlink-merged.sh b/t/t2108-update-index-symlink-merged.sh\nnew file mode 100755\nindex 0000000..7e28e91\n--- /dev/null\n+++ b/t/t2108-update-index-symlink-merged.sh\n@@ -0,0 +1,43 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2010 Stefan Haller\n+#\n+\n+test_description='git update-index on filesystem w/o symlinks test.\n+\n+This tests that git update-index keeps the executable bit when staging\n+an unmerged file after a merge if core.filemode is false.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success \\\n+'preparation' '\n+git config core.symlinks false &&\n+l=$(printf file | git hash-object -t blob -w --stdin) &&\n+echo \"120000 $l    symlink\" | git update-index --index-info &&\n+git commit -m \"Create\"'\n+\n+test_expect_success \\\n+'modify the symlink on two branches and merge' '\n+git branch br &&\n+test_commit \"Modify_on_master\" symlink &&\n+git checkout br -- &&\n+test_commit \"Modify_on_branch\" symlink &&\n+test_must_fail git merge master'\n+\n+test_expect_success \\\n+'double-check that file is indeed unmerged' '\n+git ls-files --unmerged --error-unmatch -- symlink'\n+\n+test_expect_success \\\n+'stage unmerged file with update-index' '\n+git update-index -- symlink'\n+\n+test_expect_failure \\\n+'check that file is still a symlink' '\n+case \"`git ls-files --stage --cached -- symlink`\" in\n+\"120000 \"*symlink) echo pass;;\n+*) echo fail; git ls-files --stage --cached -- symlink; (exit 1);;\n+esac'\n+\n+test_done\n-- \n1.7.3.1.57.gb5d9d\n"},{"id":"154345","messageId":"1jqvvxl.1e5c93nipc126M%lists@haller-berlin.de","threadId":"25527","inReplyTo":"1jqvbx3.1icsj8j1jf26lfM%lists@haller-berlin.de","subject":"Re: [PATCH] Add tests to demonstrate update-index bug with core.symlinks/core.filemode","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2010-10-24T18:53:13Z","receivedAt":"2010-10-24T18:53:13Z","isPatch":true,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"Stefan Haller <lists@haller-berlin.de> wrote:\n\n> Stefan Haller <lists@haller-berlin.de> wrote:\n> \n> > There's a bug with the handling of the executable bit when core.filemode\n> > is false: when you have an executable file that has unmerged changes,\n> > and you stage it with \"git update-index\", the executable bit is lost.\n> > If you stage it with \"git add\" instead, it works fine.\n> \n> It turns out that the same bug exists for symlinks when core.symlink is\n> false. Here's a patch that adds two tests that demonstrate the problems.\n> (I suspect both have a similar cause, and/or a similar solution.)\n\nOK, so I found commit 2031427 (git add: respect core.filemode with\nunmerged entries), and the corresponding email thread at\n<http://article.gmane.org/gmane.comp.version-control.git/51182/>, that\nfixed the same bug for git add in 2007.\n\nSo maybe the fix for update-index is as simple as replacing the\ncache_name_pos call in process_path() with index_name_pos_also_unmerged,\nbut I'm afraid of breaking something else in that area.  Advice welcome.\n\nBTW, I'm not convinced that the logic in index_name_pos_also_unmerged of\nalways preferring stage 2 over stage 3 is good in all cases.  For the\ncase where a regular file is changed into a symlink or vice versa it\nprobably doesn't matter, as the resolution will always require human\nintervention, but if the mode just changes from 644 to 755 or back, it\nseems wrong to always pick the mode of \"ours\" over \"theirs\".  Instead,\nit should pick whichever mode differs from stage 1, if the other\ndoesn't.\n\n\n-- \nStefan Haller\nBerlin, Germany\nhttp://www.haller-berlin.de/\n"},{"id":"154352","messageId":"m37hh7jh17.fsf@localhost.localdomain","threadId":"25527","inReplyTo":"1jqvbx3.1icsj8j1jf26lfM%lists@haller-berlin.de","subject":"Re: [PATCH] Add tests to demonstrate update-index bug with core.symlinks/core.filemode","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-10-24T23:41:44Z","receivedAt":"2010-10-24T23:41:44Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"lists@haller-berlin.de (Stefan Haller) writes:\n\n> This is the first time I write a git test, so please point out anything\n> I might have done wrong. Also, I still don't have much of an idea how or\n> where to fix the problem, so any guidance towards that is much\n> appreciated.\n> \n>  t/t2107-update-index-executable-bit-merged.sh |   44 +++++++++++++++++++++++++\n>  t/t2108-update-index-symlink-merged.sh        |   43 ++++++++++++++++++++++++\n>  2 files changed, 87 insertions(+), 0 deletions(-)\n>  create mode 100755 t/t2107-update-index-executable-bit-merged.sh\n>  create mode 100755 t/t2108-update-index-symlink-merged.sh\n\nI guess that because those two tests are conceptually about the same\nthing, namely errors in git-update-index handling permissions which\ncannot be represented on filesystem (core.filemode and/or\ncore.symlinks is false).\n \n> diff --git a/t/t2107-update-index-executable-bit-merged.sh b/t/t2107-update-index-executable-bit-merged.sh\n> new file mode 100755\n> index 0000000..7a8f740\n> --- /dev/null\n> +++ b/t/t2107-update-index-executable-bit-merged.sh\n> @@ -0,0 +1,44 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2010 Stefan Haller\n> +#\n> +\n> +test_description='git update-index on filesystem w/o symlinks test.\n> +\n> +This tests that git update-index keeps the executable bit when staging\n> +an unmerged file after a merge if core.filemode is false.'\n\nAll right.\n\n> +\n> +. ./test-lib.sh\n\nAll right.\n\n> +\n> +test_expect_success \\\n> +'preparation' '\n> +git config core.filemode false &&\n> +touch foo &&\n> +git add foo &&\n> +git update-index --chmod=+x foo &&\n> +git commit -m \"Create\"'\n\nThe suggested way of coding in test script looks like the following:\n\n  +test_expect_success 'preparation' '\n  +\tgit config core.filemode false &&\n  +\t>foo &&\n  +\tgit add foo &&\n  +\tgit update-index --chmod=+x foo &&\n  +\tgit commit -m \"Create\"\n  +'\n\nBTW. does it matter that 'foo' is empty?\n\n[...]\n> +test_expect_failure \\\n> +'check that filemode is still 100755' '\n> +case \"`git ls-files --stage --cached -- foo`\" in\n> +\"100755 \"*foo) echo pass;;\n> +*) echo fail; git ls-files --stage --cached -- foo; (exit 1);;\n> +esac'\n\nWouldn't it be better to simply prepare expected output (perhaps with\nstubs for hashes), and compare actual with expected output?\n\nAlso, weren't you able to use test_tick, test_commit, test_merge\nfunctions from test-lib.sh?\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"154390","messageId":"7vtykaiv4b.fsf@alter.siamese.dyndns.org","threadId":"25527","inReplyTo":"1jqvvxl.1e5c93nipc126M%lists@haller-berlin.de","subject":"Re: [PATCH] Add tests to demonstrate update-index bug with core.symlinks/core.filemode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-25T07:34:12Z","receivedAt":"2010-10-25T07:34:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"lists@haller-berlin.de (Stefan Haller) writes:\n\n> OK, so I found commit 2031427 (git add: respect core.filemode with\n> unmerged entries), and the corresponding email thread ...\n\nI haven't had a chance to take a look at your issue with update-index, but\nthe patch you quoted here was the first thing that came to my mind, and my\ngut feeling is that the same fix (or at least a fix in the same spirit) is\nappropriate for update-index.\n\nAlso I agree with you in that we should attempt the three-way merge of\nmode bits (not just in update-index but also in add) when the user tries\nto add contents from the working tree that does not have trustworthy\nexecutable bit.\n\n 1. If stages 2 and 3 have the same executable bits, we can take that\n    result they agree upon, without any warning;\n\n 2. If stages 2 and 3 are different, and if there is stage 1, we should\n    take the one that is different from stage 1;  We _might_ want to warn\n    in this case, but I am not sure.\n\n 3. If there is no stage 1 present but stages 2 and 3 have different bits,\n    we should take the bit from stage 2 (for the sake of backward\n    compatibility), but I think we should warn the user that we did so;\n\n 4. If only one of stages 2 or 3 are present, we should take the bit from\n    the one that exists (again for the sake of backward compatibility),\n    but we should warn in this case as well, I think.\n"},{"id":"154392","messageId":"1jqwy91.df4hcq1wxhklzM%lists@haller-berlin.de","threadId":"25527","inReplyTo":"m37hh7jh17.fsf@localhost.localdomain","subject":"Re: [PATCH] Add tests to demonstrate update-index bug with core.symlinks/core.filemode","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2010-10-25T08:59:11Z","receivedAt":"2010-10-25T08:59:11Z","isPatch":true,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"Jakub Narebski <jnareb@gmail.com> wrote:\n\n> > +test_expect_success \\\n> > +'preparation' '\n> > +git config core.filemode false &&\n> > +touch foo &&\n> > +git add foo &&\n> > +git update-index --chmod=+x foo &&\n> > +git commit -m \"Create\"'\n> \n> The suggested way of coding in test script looks like the following:\n> \n>   +test_expect_success 'preparation' '\n>   +   git config core.filemode false &&\n>   +   >foo &&\n>   +   git add foo &&\n>   +   git update-index --chmod=+x foo &&\n>   +   git commit -m \"Create\"\n>   +'\n\nOK, will use that style next time, thanks.  I copied the style from\nt2102...\n\n> BTW. does it matter that 'foo' is empty?\n\nNo, it doesn't make a difference.\n\n> [...]\n> > +test_expect_failure \\\n> > +'check that filemode is still 100755' '\n> > +case \"`git ls-files --stage --cached -- foo`\" in\n> > +\"100755 \"*foo) echo pass;;\n> > +*) echo fail; git ls-files --stage --cached -- foo; (exit 1);;\n> > +esac'\n> \n> Wouldn't it be better to simply prepare expected output (perhaps with\n> stubs for hashes), and compare actual with expected output?\n\nMaybe; again, I just copied that from t2102.  (That's also why I felt\nuncomfortable putting that copyright notice at the top of my files...)\n\n> Also, weren't you able to use test_tick, test_commit, test_merge\n> functions from test-lib.sh?\n\nI used test_commit where possible; for the initial commit I couldn't.\nAs for test_merge and test_tick, I could have used them, yes; what's the\nbenefit though?\n\n\n-- \nStefan Haller\nBerlin, Germany\nhttp://www.haller-berlin.de/\n"}]}