First batch of GitPython pruning

[kyleam]

This eliminates some of the GitPython API calls mentioned in #2879.

• Eliminates nearly all repo.git.rev_parse calls through use and extension of GitRepo.get_hexsha and through new GitRepo.commit_exists. (ece5229 explains the remaining two.)
• Removes all repo.git.show calls with use of new GitRepo.format_commit.
• Removes all repo.git_dir uses.
• Add GitRepo.is_ancestor and drops repo.git.merge_base.

---

• This change is complete

[codecov]

Codecov Report

Merging #2902 into master will increase coverage by <.01%.
The diff coverage is 97.61%.

@@            Coverage Diff             @@
##           master    #2902      +/-   ##
==========================================
+ Coverage   90.28%   90.29%   +<.01%     
==========================================
  Files         246      246              
  Lines       31877    31887      +10     
==========================================
+ Hits        28781    28791      +10     
  Misses       3096     3096

Impacted Files	Coverage Δ
datalad/support/tests/test_repodates.py	100% <ø> (ø)	⬆️
datalad/interface/run.py	100% <100%> (ø)	⬆️
datalad/interface/rerun.py	96.26% <100%> (-0.19%)	⬇️
datalad/interface/tests/test_save.py	100% <100%> (ø)	⬆️
datalad/interface/tests/test_run.py	99.83% <100%> (ø)	⬆️
datalad/support/tests/test_gitrepo.py	100% <100%> (ø)	⬆️
datalad/support/gitrepo.py	88.93% <95.83%> (+0.2%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a98a016...703552c. Read the comment docs.

[yarikoptic]

I wonder if there is a performance impact/benefit. I expect that previous implementation used in memory db

[kyleam]

I wonder if there is a performance impact/benefit. I expect that previous implementation used in memory db

Not sure. I'm not familiar with how gitpython uses the db internally and am not sure when it comes into play. (Though this seems like a more general concern for #2879.)

The rev-parse call is probably the most relevant one for this pr. A crude test:

$ python -m timeit -s "from datalad.support.gitrepo import GitRepo; gr = GitRepo('.')" "gr.get_hexsha()"
100 loops, best of 3: 8.79 msec per loop

$ python -m timeit -s "from datalad.support.gitrepo import GitRepo; gr = GitRepo('.')" "gr._git_custom_command('', ['git', 'rev-parse', 'HEAD'])"
100 loops, best of 3: 8.12 msec per loop

$ python -m timeit -s "from datalad.support.gitrepo import GitRepo; gr = GitRepo('.')" "gr.repo.git.rev_parse('HEAD')"
100 loops, best of 3: 7.71 msec per loop

[mih]

@kyleam Thanks for the test. Personally, I would be willing to pay a higher price for not having that DB. This is pretty much the only state information that we are carrying around (besides whether a repo has an annex or not). From my POV we are not benefitting from it much, and we actually had more then one issue with leakage/destructor calls in the past.

My vote is on improving the statelessness of Dataset/Repo.

[mih]

Apart from the general attitude towards gitpython DB vs. straight git calls (see prev comment), this LGTM.

[mih]

Ah, another one without -z. Thx for catching that!

[yarikoptic]

Re generic "From my POV we are not benefitting from it much,", hopefully @bpoldrack could support my memory of an early analysis where we compared against straight git calls and concluded that performance benefit was notable. But that was in the past, and even git could have become faster, and the way we use it might have changed not requiring as much of git commits structure analysis

[kyleam]

Stepping back, my current understanding is that there is general agreement that GitPython should not be used outside of GitRepo, and whether we should remove GitPython altogether is a separate issue. If that is true, I fail to see how this peformance discussion pertains to this PR since converting repo.git calls to GitRepo method calls would seem the clear direction to go. Instead, the question would be whether the relevant GitRepo method (here GitRepo.get_hexsha) should use GitPython.

[yarikoptic]

Stepping back, my current understanding is that there is general agreement that GitPython should not be used outside of GitRepo, and whether we should remove GitPython altogether is a separate issue.

True

If that is true, I fail to see how this peformance discussion pertains to this PR since converting repo.git calls to GitRepo method calls would seem the clear direction to go. Instead, the question would be whether the relevant GitRepo method (here GitRepo.get_hexsha) should use GitPython.

yeah, I just wondered if we would have any performance impact since GitPython's .git.rev_parse was replaced with get_hexsha which does git call... FWIW here is timing from my laptop on datalad code repo

$> python -m timeit -s "from datalad.support.gitrepo import GitRepo; gr = GitRepo('.')" "gr._git_custom_command('', ['git', 'rev-parse', 'HEAD'])"                                                                              100 loops, best of 3: 16.1 msec per loop
$> python -m timeit -s "from datalad.support.gitrepo import GitRepo; gr = GitRepo('.')" "gr.repo.git.rev_parse('HEAD')"
100 loops, best of 3: 16.5 msec per loop

so , it even gets better a tiny bit (should I finally get a new laptop? ;)), so seems to be a non-issue for this particular change, so we should go ahead with this one.

[bpoldrack]

FTR: First of all, I'm happy to see this change. Second I do remember some performance testing in the past, but the benefit from GitPython was notable only for a very small set of queries. I doubt it will notably affect our actual commands, since we do a lot more than querying for a reference. And most things just lead to direct git calls done by GitPython if you dig into it. Plus: Even where do use features that benefit from in memory DB, I think it's mostly just a single call that hardly compensates for the offset of creating that DB in the first place.

Edit:
To underline the last point: Remember how we benefited from making the instantiation of GitPython Repo lazy?