git-update-index loses executable bit for unmerged files when core.filemode is false

git-update-index loses executable bit for unmerged files when core.filemode is false

[Stefan Haller]

There's a bug with the handling of the executable bit when core.filemode
is false: when you have an executable file that has unmerged changes,
and you stage it with "git update-index", the executable bit is lost.
If you stage it with "git add" instead, it works fine.

    git init test
    cd test
    git config core.filemode false
    touch foo
    git add foo
    git update-index --chmod=+x foo
    git commit -m "Initial revision"
    git branch br
    echo bla >foo
    git commit -a -m "Changed foo"
    git checkout br
    echo blubb >foo
    git commit -a -m "Changed foo"
    git merge master
    git update-index foo
    git diff --staged

See how the diff shows that the mode goes from 100755 to 100644.  If you
replace the second-to-last line with "git add foo", it doesn't.

I started to trace this down, but I didn't get very far, as I'm not
familiar enough with the git codebase yet; so any help would be
appreciated.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

[PATCH] Add tests to demonstrate update-index bug with core.symlinks/core.filemode

[Stefan Haller]

When calling update-index on an unmerged file that is executable and
core.filemode is false, or that is a symlink and core.symlink is false,
the executable bit or the symlink property is lost.
---
Stefan Haller <lists@haller-berlin.de> wrote:

> There's a bug with the handling of the executable bit when core.filemode
> is false: when you have an executable file that has unmerged changes,
> and you stage it with "git update-index", the executable bit is lost.
> If you stage it with "git add" instead, it works fine.

It turns out that the same bug exists for symlinks when core.symlink is
false. Here's a patch that adds two tests that demonstrate the problems.
(I suspect both have a similar cause, and/or a similar solution.)

This is the first time I write a git test, so please point out anything
I might have done wrong. Also, I still don't have much of an idea how or
where to fix the problem, so any guidance towards that is much
appreciated.

 t/t2107-update-index-executable-bit-merged.sh |   44 +++++++++++++++++++++++++
 t/t2108-update-index-symlink-merged.sh        |   43 ++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 0 deletions(-)
 create mode 100755 t/t2107-update-index-executable-bit-merged.sh
 create mode 100755 t/t2108-update-index-symlink-merged.sh

diff --git a/t/t2107-update-index-executable-bit-merged.sh b/t/t2107-update-index-executable-bit-merged.sh
new file mode 100755
index 0000000..7a8f740
--- /dev/null
+++ b/t/t2107-update-index-executable-bit-merged.sh
@@ -0,0 +1,44 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Stefan Haller
+#
+
+test_description='git update-index on filesystem w/o symlinks test.
+
+This tests that git update-index keeps the executable bit when staging
+an unmerged file after a merge if core.filemode is false.'
+
+. ./test-lib.sh
+
+test_expect_success \
+'preparation' '
+git config core.filemode false &&
+touch foo &&
+git add foo &&
+git update-index --chmod=+x foo &&
+git commit -m "Create"'
+
+test_expect_success \
+'modify the file on two branches and merge' '
+git branch br &&
+test_commit "Modify_on_master" foo &&
+git checkout br -- &&
+test_commit "Modify_on_branch" foo &&
+test_must_fail git merge master'
+
+test_expect_success \
+'double-check that file is indeed unmerged' '
+git ls-files --unmerged --error-unmatch -- foo'
+
+test_expect_success \
+'stage unmerged file with update-index' '
+git update-index -- foo'
+
+test_expect_failure \
+'check that filemode is still 100755' '
+case "`git ls-files --stage --cached -- foo`" in
+"100755 "*foo) echo pass;;
+*) echo fail; git ls-files --stage --cached -- foo; (exit 1);;
+esac'
+
+test_done
diff --git a/t/t2108-update-index-symlink-merged.sh b/t/t2108-update-index-symlink-merged.sh
new file mode 100755
index 0000000..7e28e91
--- /dev/null
+++ b/t/t2108-update-index-symlink-merged.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Stefan Haller
+#
+
+test_description='git update-index on filesystem w/o symlinks test.
+
+This tests that git update-index keeps the executable bit when staging
+an unmerged file after a merge if core.filemode is false.'
+
+. ./test-lib.sh
+
+test_expect_success \
+'preparation' '
+git config core.symlinks false &&
+l=$(printf file | git hash-object -t blob -w --stdin) &&
+echo "120000 $l    symlink" | git update-index --index-info &&
+git commit -m "Create"'
+
+test_expect_success \
+'modify the symlink on two branches and merge' '
+git branch br &&
+test_commit "Modify_on_master" symlink &&
+git checkout br -- &&
+test_commit "Modify_on_branch" symlink &&
+test_must_fail git merge master'
+
+test_expect_success \
+'double-check that file is indeed unmerged' '
+git ls-files --unmerged --error-unmatch -- symlink'
+
+test_expect_success \
+'stage unmerged file with update-index' '
+git update-index -- symlink'
+
+test_expect_failure \
+'check that file is still a symlink' '
+case "`git ls-files --stage --cached -- symlink`" in
+"120000 "*symlink) echo pass;;
+*) echo fail; git ls-files --stage --cached -- symlink; (exit 1);;
+esac'
+
+test_done
-- 
1.7.3.1.57.gb5d9d

Re: [PATCH] Add tests to demonstrate update-index bug with core.symlinks/core.filemode

[Stefan Haller]

Stefan Haller <lists@haller-berlin.de> wrote:

> Stefan Haller <lists@haller-berlin.de> wrote:
> 
> > There's a bug with the handling of the executable bit when core.filemode
> > is false: when you have an executable file that has unmerged changes,
> > and you stage it with "git update-index", the executable bit is lost.
> > If you stage it with "git add" instead, it works fine.
> 
> It turns out that the same bug exists for symlinks when core.symlink is
> false. Here's a patch that adds two tests that demonstrate the problems.
> (I suspect both have a similar cause, and/or a similar solution.)

OK, so I found commit 2031427 (git add: respect core.filemode with
unmerged entries), and the corresponding email thread at
<http://article.gmane.org/gmane.comp.version-control.git/51182/>, that
fixed the same bug for git add in 2007.

So maybe the fix for update-index is as simple as replacing the
cache_name_pos call in process_path() with index_name_pos_also_unmerged,
but I'm afraid of breaking something else in that area.  Advice welcome.

BTW, I'm not convinced that the logic in index_name_pos_also_unmerged of
always preferring stage 2 over stage 3 is good in all cases.  For the
case where a regular file is changed into a symlink or vice versa it
probably doesn't matter, as the resolution will always require human
intervention, but if the mode just changes from 644 to 755 or back, it
seems wrong to always pick the mode of "ours" over "theirs".  Instead,
it should pick whichever mode differs from stage 1, if the other
doesn't.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

Re: [PATCH] Add tests to demonstrate update-index bug with core.symlinks/core.filemode

[Junio C Hamano]

lists@haller-berlin.de (Stefan Haller) writes:

> OK, so I found commit 2031427 (git add: respect core.filemode with
> unmerged entries), and the corresponding email thread ...

I haven't had a chance to take a look at your issue with update-index, but
the patch you quoted here was the first thing that came to my mind, and my
gut feeling is that the same fix (or at least a fix in the same spirit) is
appropriate for update-index.

Also I agree with you in that we should attempt the three-way merge of
mode bits (not just in update-index but also in add) when the user tries
to add contents from the working tree that does not have trustworthy
executable bit.

 1. If stages 2 and 3 have the same executable bits, we can take that
    result they agree upon, without any warning;

 2. If stages 2 and 3 are different, and if there is stage 1, we should
    take the one that is different from stage 1;  We _might_ want to warn
    in this case, but I am not sure.

 3. If there is no stage 1 present but stages 2 and 3 have different bits,
    we should take the bit from stage 2 (for the sake of backward
    compatibility), but I think we should warn the user that we did so;

 4. If only one of stages 2 or 3 are present, we should take the bit from
    the one that exists (again for the sake of backward compatibility),
    but we should warn in this case as well, I think.

Re: [PATCH] Add tests to demonstrate update-index bug with core.symlinks/core.filemode

[Jakub Narebski]

lists@haller-berlin.de (Stefan Haller) writes:

> This is the first time I write a git test, so please point out anything
> I might have done wrong. Also, I still don't have much of an idea how or
> where to fix the problem, so any guidance towards that is much
> appreciated.
> 
>  t/t2107-update-index-executable-bit-merged.sh |   44 +++++++++++++++++++++++++
>  t/t2108-update-index-symlink-merged.sh        |   43 ++++++++++++++++++++++++
>  2 files changed, 87 insertions(+), 0 deletions(-)
>  create mode 100755 t/t2107-update-index-executable-bit-merged.sh
>  create mode 100755 t/t2108-update-index-symlink-merged.sh

I guess that because those two tests are conceptually about the same
thing, namely errors in git-update-index handling permissions which
cannot be represented on filesystem (core.filemode and/or
core.symlinks is false).
 
> diff --git a/t/t2107-update-index-executable-bit-merged.sh b/t/t2107-update-index-executable-bit-merged.sh
> new file mode 100755
> index 0000000..7a8f740
> --- /dev/null
> +++ b/t/t2107-update-index-executable-bit-merged.sh
> @@ -0,0 +1,44 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2010 Stefan Haller
> +#
> +
> +test_description='git update-index on filesystem w/o symlinks test.
> +
> +This tests that git update-index keeps the executable bit when staging
> +an unmerged file after a merge if core.filemode is false.'

All right.

> +
> +. ./test-lib.sh

All right.

> +
> +test_expect_success \
> +'preparation' '
> +git config core.filemode false &&
> +touch foo &&
> +git add foo &&
> +git update-index --chmod=+x foo &&
> +git commit -m "Create"'

The suggested way of coding in test script looks like the following:

  +test_expect_success 'preparation' '
  +	git config core.filemode false &&
  +	>foo &&
  +	git add foo &&
  +	git update-index --chmod=+x foo &&
  +	git commit -m "Create"
  +'

BTW. does it matter that 'foo' is empty?

[...]
> +test_expect_failure \
> +'check that filemode is still 100755' '
> +case "`git ls-files --stage --cached -- foo`" in
> +"100755 "*foo) echo pass;;
> +*) echo fail; git ls-files --stage --cached -- foo; (exit 1);;
> +esac'

Wouldn't it be better to simply prepare expected output (perhaps with
stubs for hashes), and compare actual with expected output?

Also, weren't you able to use test_tick, test_commit, test_merge
functions from test-lib.sh?

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] Add tests to demonstrate update-index bug with core.symlinks/core.filemode

[Stefan Haller]

Jakub Narebski <jnareb@gmail.com> wrote:

> > +test_expect_success \
> > +'preparation' '
> > +git config core.filemode false &&
> > +touch foo &&
> > +git add foo &&
> > +git update-index --chmod=+x foo &&
> > +git commit -m "Create"'
> 
> The suggested way of coding in test script looks like the following:
> 
>   +test_expect_success 'preparation' '
>   +   git config core.filemode false &&
>   +   >foo &&
>   +   git add foo &&
>   +   git update-index --chmod=+x foo &&
>   +   git commit -m "Create"
>   +'

OK, will use that style next time, thanks.  I copied the style from
t2102...

> BTW. does it matter that 'foo' is empty?

No, it doesn't make a difference.

> [...]
> > +test_expect_failure \
> > +'check that filemode is still 100755' '
> > +case "`git ls-files --stage --cached -- foo`" in
> > +"100755 "*foo) echo pass;;
> > +*) echo fail; git ls-files --stage --cached -- foo; (exit 1);;
> > +esac'
> 
> Wouldn't it be better to simply prepare expected output (perhaps with
> stubs for hashes), and compare actual with expected output?

Maybe; again, I just copied that from t2102.  (That's also why I felt
uncomfortable putting that copyright notice at the top of my files...)

> Also, weren't you able to use test_tick, test_commit, test_merge
> functions from test-lib.sh?

I used test_commit where possible; for the initial commit I couldn't.
As for test_merge and test_tick, I could have used them, yes; what's the
benefit though?


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/