improve index mode for executable files

[tmzullinger]

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.

GitPython/git/index/fun.py

Line 111 in eae0e37

def stat_mode_to_index_mode(mode):

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?

[tmzullinger]

Indeed, the code I pasted was only to show the method I used to test things, in case someone else ended up picking up the tests later. :)

I'm also a fan of tests so I spent a little time this morning working on adding some. I tested locally with nostests with and without the code change. It fails before and passes after. I had some print statements in the test while working on it to confirm that the expected modes are correct.

Let me know what needs improved with the test. I won't be surprised if I've overlooked some common pattern used in the test suite. If you'd prefer to have the test added in a separate commit (before or after the code fix) that's easy enough. I'm used to the git project where tests and fixes generally go together. When they don't, they'd add a test which is marked as an expected to fail and changed to expected to pass in the following commit with the code fix.

[Byron]

That's perfect, thanks so much for the test! I am feeling much safer now.

When it comes to the way the test is written I really am not picky, and I absolutely trust that the test will fail without the change.

Will you benefit from an immediate release or can it wait?

[tmzullinger]

Excellent, thank you!

This can absolutely wait. It's just a tightening of code that's been in place for 5 years without complaint, so I don't imagine many people run into it. :)