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

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.

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:

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

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

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.

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

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.

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

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).
 

All right.

All right.

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?

[...]

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 <jnareb@gmail.com> wrote:

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

No, it doesn't make a difference.

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...)

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?