Adding dubious ownership handling

[marioaag]

This is a fix for the ticket -> #1616

This was already validated and catch the dubious ownership from the repo object generation to avoid confusing errors forward.

Validations results:

Catching dubious ownership

[root@ilabinfra-dev BlueGen]# python3
Python 3.9.16 (main, Sep 12 2023, 00:00:00) 
[GCC 11.3.1 20221121 (Red Hat 11.3.1-4)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import git
>>> repo = git .Repo('/tmp/BlueGen')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.9/site-packages/git/repo/base.py", line 303, in __init__
    raise InvalidGitRepositoryError(err_msg) from None
git.exc.InvalidGitRepositoryError: Detected dubious ownership in repository at: /tmp/BlueGen

No dubious ownership

[mario@ilabinfra-dev BlueGen]$ python3
Python 3.9.16 (main, Sep 12 2023, 00:00:00) 
[GCC 11.3.1 20221121 (Red Hat 11.3.1-4)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import git
>>> repo = git .Repo('/tmp/BlueGen')

[Byron]

Thanks a lot for tackling this.

A huge problem I see with the approach, is that it imposes the latency of one additional git invocation directly onto the Repo instantiation code-path, which runs a lot. The slowdown will be substantial, and a high price to pay for a better error message.

I believe the price is too high, but there might be remedies.

This is because GitPython already calls git a couple of times, here is what I recall.

• In cmd.py it obtains _version_info by calling git - this happens lazily, and I wonder if it could fail.
• When accessing objects, it spawns two git commands to stream objects.

Can a nice error message be generated there instead, if these commands are failing? Maybe more generally, is it possible to 'supervise' the stderr of any invoked command to see if the execution failed because of it? In terms of compute power, I presume this is a better deal as it only costs once commands fail, which usually is an error condition anyway.

What do you think?

[Byron]

git status can be a very expensive operation, and even when it's not it's still expensive and can't be happening when creating a new Repository instance. Interestingly, it will only be cheap if it fails due to dubious ownership.

Can you find another invocation that ideally just checks for permissions and nothing else?

[marioaag]

You came to the same dilemma that I get when I was implementing this...
I thought to just keep it as message enhancement when the git command gets executed but then I thought that it could be little late for the user that already generated the repo instance and maybe did some other stuff but I didn't consider that it could be very expensive operation, not sure if do a manual check could be less expensive just like git is doing it, check if the ownership is different to the actual user but then a lot of extra logic will need to be implemented to consider the git config file exceptions, etc. Kind of duplicated logic and more conde to maintain.

Is it an easy way to check how expensive it is to execute this command? it should be just one time at the repo instantiation, maybe change it for another operation less expensive like check the latest commit?

[Byron]

I thought about this and realized that adding something into the instantiation-path would still be unacceptable even if it could happen at no cost. The reason for this is that right now, some operations can be performed without git and safely so, so users shouldn't be held back by something that git would say. Further, doing so would be a breaking change.

Personally I think git was 'adamant' in their implementation as they put defence of a particular issue before usability. It's understandable, but also far from the best possible implementation of such mitigation.

gitoxide was lucky enough as second-mover to have a fresh take on this, as it will simply ignore configuration data from untrusted sources. GitPython should do the same, as much as possible, and prefer to provide better error messages if git fails.

[marioaag]

get it! now I see what is the philosophy of GitPython, let me reformulate this implementation and go with an enhancement of the message in the cases where it apply.

[Byron]

I've noticed this PR contains a rebased version of 359116b as 07f3641 with the same change and message, which seems like it may be unintentional. Depending how this PR is merged, I would expect the duplicate commit to make it into the upstream GitPython history. I don't know that this is really a problem. But if you want to avoid it, running git rebase main on the GitPython-1616 branch should remove it, showing a message like this (after which the rebased GitPython-1616 branch could be force-pushed):

➜ /workspaces/GitPython (GitPython-1616) $ git rebase main
warning: skipped previously applied commit 07f36411
hint: use --reapply-cherry-picks to include skipped commits
hint: Disable this message with "git config advice.skippedCherryPicks false"
Successfully rebased and updated refs/heads/GitPython-1616.

Sorry if there's a reason for this that I missed. I just figured I'd mention it in case it is not intended.

Thanks, @EliahKagan, for the suggestion. That's exactly what I did and it resolved the commit-duplication like anticipated.

[marioaag]

Hi @Byron, the new approach is implemented! I found that all the errors related with dubious ownership comes to the missing SHA so the steep where it is used is the best place to report the issue and all the rest of the actions that can run without this info will be available.

[Byron]

Great, that's it!

Thanks so much for going the last mile with this :)!