Repository: gitpython-developers/GitPython
Pull request #1254 (2021-05-24 22:00:27)

improve index mode for executable files
As noted in #1253, this area could use some tests.  Any help is appreciated there.

Byron: Thanks a lot! If the tests remain green we could certainly merge this one right away.

Otherwise it should be possible to add a unit-tests without much fuzz which tests the function in question directly.

https://github.com/gitpython-developers/GitPython/blob/eae0e37c88a71a3b8ca816b820eed71fd1590f11/git/index/fun.py#L111

It could be fed a few modes along with the desired output.

Tests can be run locally with `nose`, and these days it became a little harder as one has to install `tox` to get the dependencies, and from `nose` should become available in `.tox/…/bin` somewhere. I am not purposefully vague here, I just don't exactly know.

If you don't want to try a test just yet it could be merged without it, often I have unreasonable trust in the existing test suite.
tmzullinger: Thanks!  FWIW, I tested manually via the following:

```
$ for mode in 0600 0641 0644 0650 0700 0744 0751 0755; do touch file-$mode && chmod $mode file-$mode; done

$ ls -l
total 0
-rw-------. 1 user user 0 May 24 23:36 file-0600
-rw-r----x. 1 user user 0 May 24 23:36 file-0641
-rw-r--r--. 1 user user 0 May 24 23:36 file-0644
-rw-r-x---. 1 user user 0 May 24 23:36 file-0650
-rwx------. 1 user user 0 May 24 23:36 file-0700
-rwxr--r--. 1 user user 0 May 24 23:36 file-0744
-rwxr-x--x. 1 user user 0 May 24 23:36 file-0751
-rwxr-xr-x. 1 user user 0 May 24 23:36 file-0755

$ python -c 'import git, glob; repo = git.Repo(); repo.index.add(glob.glob("file-*"))'

$ git ls-files -s
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       file-0600
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       file-0641
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       file-0644
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       file-0650
100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       file-0700
100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       file-0744
100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       file-0751
100755 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       file-0755
```

I goofed off a bit more today than I should have and didn't leave myself time to poke around with adding proper tests.  If I can make some time in the next day or two to do that, I'll push them here.  If you're okay to merge this before then, that's fine with me too.  And maybe I'll still get around to adding tests later (though I can be bad about getting back to things).
Byron: Something like that could be reproduced in the test system, but it's harder than it should be since shell execution isn't natively supported there. Instead I recommend calling the function directly on plain file modes that are created as literals in python itself.

Let's see if you can get to it till the weekend, which is when I will merge this PR anyway :D. If it helps, adding it would prevent regression, something my shoddy memory won't prevent for sure 😅.

Would you benefit from a new release after merge?

Patch:
diff --git a/git/index/fun.py b/git/index/fun.py
index f40928c3..1012f480 100644
--- a/git/index/fun.py
+++ b/git/index/fun.py
@@ -11,6 +11,7 @@ from stat import (
     S_ISDIR,
     S_IFMT,
     S_IFREG,
+    S_IXUSR,
 )
 import subprocess
 
@@ -115,7 +116,7 @@ def stat_mode_to_index_mode(mode):
         return S_IFLNK
     if S_ISDIR(mode) or S_IFMT(mode) == S_IFGITLINK:    # submodules
         return S_IFGITLINK
-    return S_IFREG | 0o644 | (mode & 0o111)       # blobs with or without executable bit
+    return S_IFREG | (mode & S_IXUSR and 0o755 or 0o644)  # blobs with or without executable bit
 
 
 def write_cache(entries: Sequence[Union[BaseIndexEntry, 'IndexEntry']], stream: IO[bytes],


Test patch:
diff --git a/test/test_fun.py b/test/test_fun.py
index a7fb8f8b..e3d07194 100644
--- a/test/test_fun.py
+++ b/test/test_fun.py
@@ -1,5 +1,5 @@
 from io import BytesIO
-from stat import S_IFDIR, S_IFREG, S_IFLNK
+from stat import S_IFDIR, S_IFREG, S_IFLNK, S_IXUSR
 from os import stat
 import os.path as osp
 from unittest import SkipTest
@@ -7,7 +7,8 @@ from unittest import SkipTest
 from git import Git
 from git.index import IndexFile
 from git.index.fun import (
-    aggressive_tree_merge
+    aggressive_tree_merge,
+    stat_mode_to_index_mode,
 )
 from git.objects.fun import (
     traverse_tree_recursive,
@@ -206,6 +207,16 @@ class TestFun(TestBase):
             assert_entries(aggressive_tree_merge(odb, trees), 2, True)
         # END handle ours, theirs
 
+    def test_stat_mode_to_index_mode(self):
+        modes = (
+            0o600, 0o611, 0o640, 0o641, 0o644, 0o650, 0o651,
+            0o700, 0o711, 0o740, 0o744, 0o750, 0o751, 0o755,
+        )
+        for mode in modes:
+            expected_mode = S_IFREG | (mode & S_IXUSR and 0o755 or 0o644)
+            assert stat_mode_to_index_mode(mode) == expected_mode
+        # END for each mode
+
     def _assert_tree_entries(self, entries, num_trees):
         for entry in entries:
             assert len(entry) == num_trees