{"thread":{"id":"13566","subject":"[PATCH] Fix t6031 on filesystems without working exec bit","startedAt":"2008-05-18T14:57:27Z","lastAt":"2008-05-23T19:12:09Z","messageCount":8,"participants":["Alex Riesen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"77228","messageId":"20080518145727.GA3058@steel.home","threadId":"13566","inReplyTo":null,"subject":"[PATCH] Fix t6031 on filesystems without working exec bit","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-05-18T14:57:27Z","receivedAt":"2008-05-18T14:57:27Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"The point of the test is not really to test the ability of the\nfilesystem to keep the given x-bit, but to check is merge-recursive\ncorrectly handles it.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n t/t6031-merge-recursive.sh |    6 ++----\n 1 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\nindex c8310ae..f1c91c8 100755\n--- a/t/t6031-merge-recursive.sh\n+++ b/t/t6031-merge-recursive.sh\n@@ -12,8 +12,7 @@ test_expect_success 'mode change in one branch: keep changed version' '\n \tgit add dummy &&\n \tgit commit -m a &&\n \tgit checkout -b b1 master &&\n-\tchmod +x file1 &&\n-\tgit add file1 &&\n+\tgit update-index --chmod=+x file1 &&\n \tgit commit -m b1 &&\n \tgit checkout a1 &&\n \tgit merge-recursive master -- a1 b1 &&\n@@ -25,8 +24,7 @@ test_expect_success 'mode change in both branches: expect conflict' '\n \tgit checkout -b a2 master &&\n \t: >file2 &&\n \tH=$(git hash-object file2) &&\n-\tchmod +x file2 &&\n-\tgit add file2 &&\n+\tgit update-index --add --chmod=+x file2 &&\n \tgit commit -m a2 &&\n \tgit checkout -b b2 master &&\n \t: >file2 &&\n-- \n1.5.5.1.354.g902c\n"},{"id":"77254","messageId":"7v7idqaocb.fsf@gitster.siamese.dyndns.org","threadId":"13566","inReplyTo":"20080518145727.GA3058@steel.home","subject":"Re: [PATCH] Fix t6031 on filesystems without working exec bit","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2008-05-19T04:51:16Z","receivedAt":"2008-05-19T04:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\n> index c8310ae..f1c91c8 100755\n> --- a/t/t6031-merge-recursive.sh\n> +++ b/t/t6031-merge-recursive.sh\n> @@ -12,8 +12,7 @@ test_expect_success 'mode change in one branch: keep changed version' '\n>  \tgit add dummy &&\n>  \tgit commit -m a &&\n>  \tgit checkout -b b1 master &&\n> -\tchmod +x file1 &&\n> -\tgit add file1 &&\n> +\tgit update-index --chmod=+x file1 &&\n>  \tgit commit -m b1 &&\n>  \tgit checkout a1 &&\n>  \tgit merge-recursive master -- a1 b1 &&\n\nI have to wonder if this is enough on a filesystem with usable executable\nbit.  Has this been tested on both kinds of filesystems?\n\nYou aren't setting +x on work tree file anymore, but only flipping the bit\ninside the index before committing.  Because of this change, after \"b1\"\ncommit, work tree has a local modification relative to the commit (namely,\nreversion of chmod +x is in the work tree), which is different from the\noriginal test sequence.  Doesn't this local modification interact with\nswitching to a1 branch and what merge-recursive does?\n"},{"id":"77256","messageId":"20080519060015.GA3179@steel.home","threadId":"13566","inReplyTo":"7v7idqaocb.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix t6031 on filesystems without working exec bit","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-05-19T06:00:15Z","receivedAt":"2008-05-19T06:00:15Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Mon, May 19, 2008 06:51:16 +0200:\n> Alex Riesen <raa.lkml@gmail.com> writes:\n> \n> > diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\n> > index c8310ae..f1c91c8 100755\n> > --- a/t/t6031-merge-recursive.sh\n> > +++ b/t/t6031-merge-recursive.sh\n> > @@ -12,8 +12,7 @@ test_expect_success 'mode change in one branch: keep changed version' '\n> >  \tgit add dummy &&\n> >  \tgit commit -m a &&\n> >  \tgit checkout -b b1 master &&\n> > -\tchmod +x file1 &&\n> > -\tgit add file1 &&\n> > +\tgit update-index --chmod=+x file1 &&\n> >  \tgit commit -m b1 &&\n> >  \tgit checkout a1 &&\n> >  \tgit merge-recursive master -- a1 b1 &&\n> \n> I have to wonder if this is enough on a filesystem with usable executable\n> bit.  Has this been tested on both kinds of filesystems?\n\nYes and yes.\n\n> You aren't setting +x on work tree file anymore, but only flipping the bit\n> inside the index before committing.  Because of this change, after \"b1\"\n> commit, work tree has a local modification relative to the commit (namely,\n> reversion of chmod +x is in the work tree), which is different from the\n> original test sequence.  Doesn't this local modification interact with\n> switching to a1 branch and what merge-recursive does?\n\nI think no (I depend on this in my workflows on both systems).\n"},{"id":"77399","messageId":"7v8wy360l8.fsf@gitster.siamese.dyndns.org","threadId":"13566","inReplyTo":"20080519060015.GA3179@steel.home","subject":"Re: [PATCH] Fix t6031 on filesystems without working exec bit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-21T17:14:43Z","receivedAt":"2008-05-21T17:14:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> Junio C Hamano, Mon, May 19, 2008 06:51:16 +0200:\n> ...\n>> I have to wonder if this is enough on a filesystem with usable executable\n>> bit.  Has this been tested on both kinds of filesystems?\n>\n> Yes and yes.\n\nI am very much regretting for having extracted the above answer from you.\nAfter actually running the test myself on a machine with executable bit, I\nhave to conclude that I now have to review any patch from you with much\nmore suspicion than I have done before.  Unless the test I ran on my\nmachine is lying to me, that is...\n\n>> You aren't setting +x on work tree file anymore, but only flipping the bit\n>> inside the index before committing.  Because of this change, after \"b1\"\n>> commit, work tree has a local modification relative to the commit (namely,\n>> reversion of chmod +x is in the work tree), which is different from the\n>> original test sequence.  Doesn't this local modification interact with\n>> switching to a1 branch and what merge-recursive does?\n>\n> I think no (I depend on this in my workflows on both systems).\n\nLook at what the \"switch to a1 branch after committing to b1\" does again.\nYou modify \"file1\" and commit to \"b1\" and you now have a local change in\nfile1 (because you update only the index and not the work tree) that is\neffectively a revert of what you committed just now, and a1 and b1 differs\nat that path.  You get \"Entry 'file1' not uptodate. Cannot merge.\"  when\nyou try to switch to \"a1\".\n\n    $ sh t6031-merge-recursive.sh -i -v\n    * expecting success: \n            : >file1 &&\n            git add file1 &&\n            git commit -m initial &&\n            git checkout -b a1 master &&\n            : >dummy &&\n            git add dummy &&\n            git commit -m a &&\n            git checkout -b b1 master &&\n            git update-index --chmod=+x file1 &&\n            git commit -m b1 &&\n            git checkout a1 &&\n            git merge-recursive master -- a1 b1 &&\n            test -x file1\n\n    Created initial commit bae7a40: initial\n     0 files changed, 0 insertions(+), 0 deletions(-)\n     create mode 100644 file1\n    Switched to a new branch \"a1\"\n    Created commit 1f64f65: a\n     0 files changed, 0 insertions(+), 0 deletions(-)\n     create mode 100644 dummy\n    Switched to a new branch \"b1\"\n    Created commit 98a1cf4: b1\n     0 files changed, 0 insertions(+), 0 deletions(-)\n     mode change 100644 => 100755 file1\n    error: Entry 'file1' not uptodate. Cannot merge.\n    * FAIL 1: mode change in one branch: keep changed version\n\nIf we squash the following on top of your patch, I think we can help\nfilesystems without executable bit without breaking filesystems with one.\nCan you see if it works on your executable-bit-less setup?\n\n-- >8 --\n t/t6031-merge-recursive.sh |    5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\nindex f1c91c8..8073e0c 100755\n--- a/t/t6031-merge-recursive.sh\n+++ b/t/t6031-merge-recursive.sh\n@@ -3,6 +3,9 @@\n test_description='merge-recursive: handle file mode'\n . ./test-lib.sh\n \n+# Note that we follow \"chmod +x F\" with \"update-index --chmod=+x F\" to\n+# help filesystems that do not have the executable bit.\n+\n test_expect_success 'mode change in one branch: keep changed version' '\n \t: >file1 &&\n \tgit add file1 &&\n@@ -12,6 +15,7 @@ test_expect_success 'mode change in one branch: keep changed version' '\n \tgit add dummy &&\n \tgit commit -m a &&\n \tgit checkout -b b1 master &&\n+\tchmod +x file1 &&\n \tgit update-index --chmod=+x file1 &&\n \tgit commit -m b1 &&\n \tgit checkout a1 &&\n@@ -24,6 +28,7 @@ test_expect_success 'mode change in both branches: expect conflict' '\n \tgit checkout -b a2 master &&\n \t: >file2 &&\n \tH=$(git hash-object file2) &&\n+\tchmod +x file2 &&\n \tgit update-index --add --chmod=+x file2 &&\n \tgit commit -m a2 &&\n \tgit checkout -b b2 master &&\n"},{"id":"77445","messageId":"81b0412b0805220116h4e4ff279k2e433a8a9a1cd9fc@mail.gmail.com","threadId":"13566","inReplyTo":"7v8wy360l8.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix t6031 on filesystems without working exec bit","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-05-22T08:16:25Z","receivedAt":"2008-05-22T08:16:25Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/5/21 Junio C Hamano <gitster@pobox.com>:\n> Look at what the \"switch to a1 branch after committing to b1\" does again.\n> You modify \"file1\" and commit to \"b1\" and you now have a local change in\n> file1 (because you update only the index and not the work tree) that is\n> effectively a revert of what you committed just now, and a1 and b1 differs\n> at that path.  You get \"Entry 'file1' not uptodate. Cannot merge.\"  when\n> you try to switch to \"a1\".\n>\n>    $ sh t6031-merge-recursive.sh -i -v\n>    * expecting success:\n>            : >file1 &&\n>            git add file1 &&\n>            git commit -m initial &&\n>            git checkout -b a1 master &&\n>            : >dummy &&\n>            git add dummy &&\n>            git commit -m a &&\n>            git checkout -b b1 master &&\n>            git update-index --chmod=+x file1 &&\n>            git commit -m b1 &&\n>            git checkout a1 &&\n>            git merge-recursive master -- a1 b1 &&\n>            test -x file1\n>\n>    Created initial commit bae7a40: initial\n>     0 files changed, 0 insertions(+), 0 deletions(-)\n>     create mode 100644 file1\n>    Switched to a new branch \"a1\"\n>    Created commit 1f64f65: a\n>     0 files changed, 0 insertions(+), 0 deletions(-)\n>     create mode 100644 dummy\n>    Switched to a new branch \"b1\"\n>    Created commit 98a1cf4: b1\n>     0 files changed, 0 insertions(+), 0 deletions(-)\n>     mode change 100644 => 100755 file1\n>    error: Entry 'file1' not uptodate. Cannot merge.\n>    * FAIL 1: mode change in one branch: keep changed version\n\nIt did work for me when I tried it back then. Maybe I just accidentally\ntested something else.\n\n> If we squash the following on top of your patch, I think we can help\n> filesystems without executable bit without breaking filesystems with one.\n> Can you see if it works on your executable-bit-less setup?\n\nNot in the moment.\n\n> -- >8 --\n>  t/t6031-merge-recursive.sh |    5 +++++\n>  1 files changed, 5 insertions(+), 0 deletions(-)\n>\n> diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh\n> index f1c91c8..8073e0c 100755\n> --- a/t/t6031-merge-recursive.sh\n> +++ b/t/t6031-merge-recursive.sh\n> @@ -3,6 +3,9 @@\n>  test_description='merge-recursive: handle file mode'\n>  . ./test-lib.sh\n>\n> +# Note that we follow \"chmod +x F\" with \"update-index --chmod=+x F\" to\n> +# help filesystems that do not have the executable bit.\n> +\n>  test_expect_success 'mode change in one branch: keep changed version' '\n>        : >file1 &&\n>        git add file1 &&\n> @@ -12,6 +15,7 @@ test_expect_success 'mode change in one branch: keep changed version' '\n>        git add dummy &&\n>        git commit -m a &&\n>        git checkout -b b1 master &&\n> +       chmod +x file1 &&\n\nNow, this is pointless in my setup. Cygwin just ignores the operation\nand decidedddds (presumably according the file _content_ or maybe\nphase of the moon) that is not executable. Working tree is still modified.\n\nMaybe I should just live with it (and disable the test locally). It is just\nvery broken system which hopefully no one besides me ever gets to see.\n"},{"id":"77448","messageId":"7vskwawvzq.fsf@gitster.siamese.dyndns.org","threadId":"13566","inReplyTo":"81b0412b0805220116h4e4ff279k2e433a8a9a1cd9fc@mail.gmail.com","subject":"Re: [PATCH] Fix t6031 on filesystems without working exec bit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-22T09:04:09Z","receivedAt":"2008-05-22T09:04:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" <raa.lkml@gmail.com> writes:\n\n> 2008/5/21 Junio C Hamano <gitster@pobox.com>:\n> ...\n>> @@ -12,6 +15,7 @@ test_expect_success 'mode change in one branch: keep changed version' '\n>>        git add dummy &&\n>>        git commit -m a &&\n>>        git checkout -b b1 master &&\n>> +      chmod +x file1 &&\n>>  \t  git update-index --chmod=+x file1 &&\n>\n> Now, this is pointless in my setup. Cygwin just ignores the operation\n> and decidedddds (presumably according the file _content_ or maybe\n> phase of the moon) that is not executable. Working tree is still modified.\n\nDoesn't it mean you spotted a bug?\n\nIf your repository is marked so that executable bit is untrustworthy\nthere, the check done when switching to branch \"a1\" to compare if \"file1\"\nthat is involved in the switch operation has local changes should ignore\n(apparent and false) executable-bit change, shouldn't it?\n"},{"id":"77469","messageId":"20080522131258.GE3206@steel.home","threadId":"13566","inReplyTo":"7vskwawvzq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix t6031 on filesystems without working exec bit","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-05-22T13:12:58Z","receivedAt":"2008-05-22T13:12:58Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Thu, May 22, 2008 11:04:09 +0200:\n> \"Alex Riesen\" <raa.lkml@gmail.com> writes:\n> \n> > 2008/5/21 Junio C Hamano <gitster@pobox.com>:\n> > ...\n> >> @@ -12,6 +15,7 @@ test_expect_success 'mode change in one branch: keep changed version' '\n> >>        git add dummy &&\n> >>        git commit -m a &&\n> >>        git checkout -b b1 master &&\n> >> +      chmod +x file1 &&\n> >>  \t  git update-index --chmod=+x file1 &&\n\nJust retested this on my normal system, and of course you are right.\nMy patch breaks the t6031 there.\n\n> > Now, this is pointless in my setup. Cygwin just ignores the operation\n> > and decidedddds (presumably according the file _content_ or maybe\n> > phase of the moon) that is not executable. Working tree is still modified.\n> \n> Doesn't it mean you spotted a bug?\n> \n\nLooks like: builtin-merge-recursive.c does not seem to reference\ntrust_executable_bit (aka core.filemode) anywhere. The default\nconfiguration statements are read in alright (there is a call to\ngit_default_config), it is just I am cannot find if the flag is used.\n\nJohannes, in the meantime I lost the track of merge-recursive\ncompletely. In this case, should I look at unpack-trees.c or is\nit still somewhere inside merge-recursive (merge_file)?\n\n> If your repository is marked so that executable bit is untrustworthy\n> there, the check done when switching to branch \"a1\" to compare if \"file1\"\n> that is involved in the switch operation has local changes should ignore\n> (apparent and false) executable-bit change, shouldn't it?\n\nWill see next monday, when I get back to that wretched laptop.\n"},{"id":"77569","messageId":"20080523191209.GA3673@steel.home","threadId":"13566","inReplyTo":"20080522131258.GE3206@steel.home","subject":"Re: [PATCH] Fix t6031 on filesystems without working exec bit","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-05-23T19:12:09Z","receivedAt":"2008-05-23T19:12:09Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Alex Riesen, Thu, May 22, 2008 15:12:58 +0200:\n> Junio C Hamano, Thu, May 22, 2008 11:04:09 +0200:\n> > If your repository is marked so that executable bit is untrustworthy\n> > there, the check done when switching to branch \"a1\" to compare if \"file1\"\n> > that is involved in the switch operation has local changes should ignore\n> > (apparent and false) executable-bit change, shouldn't it?\n> \n> Will see next monday, when I get back to that wretched laptop.\n> \n\nI managed to get my hands on it today and tried the test with your\nmodification. The test succeeds always and it succeeds with or without\nthe chmod. So it seems merge-recursive somehow ignores the x-bit from\nworking tree, I just cannot not find how and where.\n"}]}