fix: treeNotSorted issue #1799

ghost · 2024-01-12T06:59:07Z

Byron

Thanks a lot for providing a fix!

I wonder if there was a way to assure the ordering is correct by looking at what git would do.

I assume the files_in_desired_order is from a repository, could you show a git cat-file <tree>? Alternatively, maybe there is a way to use git to build a (somewhat) similar tree, and have the test compare the outcome of GitPython with the outcome of git itself - it's much more effort though.

Finally, here is a good tree for testing ordering, and you might be able create a test-case with this one as well.

Something I'd appreciate is to re-order the commits so that the test, whatever you come up with, first, and the fix later so it's clear how it fails first.

Thanks a lot for bearing with me, but I'd like to gain confidence that this is now indeed what git would do, and not just different but subtly wrong (I have been bitten by this more than once).

test/test_tree.py

EliahKagan · 2024-01-12T15:46:36Z

Would this also fix #1771?

doc/requirements.txt

ghost · 2024-01-15T07:09:06Z

Thanks a lot for providing a fix!

I wonder if there was a way to assure the ordering is correct by looking at what git would do.

I assume the files_in_desired_order is from a repository, could you show a git cat-file <tree>? Alternatively, maybe there is a way to use git to build a (somewhat) similar tree, and have the test compare the outcome of GitPython with the outcome of git itself - it's much more effort though.

Finally, here is a good tree for testing ordering, and you might be able create a test-case with this one as well.

Something I'd appreciate is to re-order the commits so that the test, whatever you come up with, first, and the fix later so it's clear how it fails first.

Thanks a lot for bearing with me, but I'd like to gain confidence that this is now indeed what git would do, and not just different but subtly wrong (I have been bitten by this more than once).

BTW, the test i provide before is here fyi~

ghost · 2024-01-15T07:14:31Z

Would this also fix #1771?

i think it's related

This fixes version incompatibilities with major version 4 of Sphinx, which GitPython is still using for the time being. Some plugins that previously had depended back on Sphinx have had those dependencies removed to avoid dependency cycles, but the effect is that `pip` no longer is aware of or able to enforce version compatibility, and newer plugin versions than can actually run with Sphinx 4 are installed instead of older, usable versions. This fixes the problem by pinning each of the affected Sphinx plugins' latest actually compatible versions, i.e., latest versions that do not need Sphinx 5 or higher. The alternative of moving to Sphinx 5 or higher should probably be done eventually, but will require addressing a cross-reference ambiguity in type annotations for the `Actor` class. For details on the bug, see: - gitpython-developers#1799 (comment) - gitpython-developers#1802

Byron

Thanks a lot for bearing with us for changes and the turbulent CI.

EliahKagan · 2024-01-16T20:30:27Z

Would this also fix #1771?

i think it's related

If this change has made the feature usable as originally intended and supersedes the advice in #369 (comment), then I think think also fixes #1771.

If not, then I think #1771 should be modified accordingly, and any non-obvious limitations of the affected code documented in its docstrings (which would close #1771).

Byron · 2024-01-16T21:03:07Z

I'd still lean towards rather removing it as it's deemed unfit, but that might be overly generalising and unnecessarily dismissive of the work of 'young' me. Since @et-repositories has had interest in fixing this, maybe they also have interest in investigating this matter?

This fixes a test bug in TestTree.test_tree_modifier_ordering where: - A `tmp` subdirectory of the directory the tests are run from (almost always the root of the GitPython source tree) was created, instead of creating it in /tmp or elsewhere configured. - That directory was never deleted afterward, creating new untracked files in GitPython's own working tree, and also making it so running the tests twice would always fail the second time, unless a manual intervening deletion was performed. (This had not broken CI, since CI checks clone the repository anew each time.) - The directory was changed into to set up the test, but not deterministically changed back out of. It would typically be exited, but this was not guaranteed to occur if some subprocess commands were to fail unexpectedly. In addition to fixing all three aspects of that bug, this also: - Remains in the temporary directory only as long as necessary to execute the subprocesses in it that produce the expected value for the test (including not entering it until running them). - Deletes the temporary directory immediately after exiting it, since it is only used to produce a file list to compare to. - Avoids interleaving file creation and git subprocess commands, since doing so made it harder to understand what was being set up and since the difference is not relevant to the test. - Does the work in that temporary directory before performing any operations with the code under test, because it is conceptually an "arrange" step, and because doing so makes its limited purpose clearer on reading the tests. - Some other refactoring to support the fixes and accompanying changes, including extracting the temporary directory logic to a helper method. This deliberately does not change the order in which any files are created. It also keeps the approach of using subprocess functions directly to operate on the temporary git repository (and changing directory while doing so, keeping each of the commands the same), since there might be good reasons to do this, such as to make very clear that the file order being obtained to compare to is really coming from git itself. Even if this is to be changed, it seems outside the scope of this bugfix. The test still fails if the fix to git/objects/tree.py from 365d44f (gitpython-developers#1799) is absent. This was verified by running: git revert --no-commit 365d44f pytest --no-cov -vv test/test_tree.py git revert --abort

Byron requested changes Jan 12, 2024

View reviewed changes

Byron reviewed Jan 12, 2024

View reviewed changes

test/test_tree.py Outdated Show resolved Hide resolved

Byron reviewed Jan 15, 2024

View reviewed changes

doc/requirements.txt Outdated Show resolved Hide resolved

EliahKagan mentioned this pull request Jan 15, 2024

Sphinx doc builds recently fail on Python 3.9 and higher #1802

Closed

EliahKagan mentioned this pull request Jan 15, 2024

Pin Sphinx plugins to compatible versions #1803

Merged

Ｅthan added 3 commits January 16, 2024 16:05

fix: add treeNotSorted test

08a819c

fix: treeNotSorted issue

365d44f

chore: update AUTHORS

bda5a17

Byron approved these changes Jan 16, 2024

View reviewed changes

Byron merged commit d28c20b into gitpython-developers:main Jan 16, 2024
22 checks passed

EliahKagan mentioned this pull request Jan 24, 2024

Status of Tree.cache and TreeModifier is unclear #1771

Open

This was referenced Feb 15, 2024

Tests create tmp dir in GitPython dir and fail on second run #1824

Closed

Keep temp files out of project dir and improve cleanup #1825

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: treeNotSorted issue #1799

fix: treeNotSorted issue #1799

ghost commented Jan 12, 2024

Byron left a comment

EliahKagan commented Jan 12, 2024

ghost commented Jan 15, 2024

ghost commented Jan 15, 2024

Byron left a comment

EliahKagan commented Jan 16, 2024

Byron commented Jan 16, 2024

fix: treeNotSorted issue #1799

fix: treeNotSorted issue #1799

Conversation

ghost commented Jan 12, 2024

Byron left a comment

Choose a reason for hiding this comment

EliahKagan commented Jan 12, 2024

ghost commented Jan 15, 2024

ghost commented Jan 15, 2024

Byron left a comment

Choose a reason for hiding this comment

EliahKagan commented Jan 16, 2024

Byron commented Jan 16, 2024