Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix files list on file rename #1537

Merged
merged 1 commit into from Jan 14, 2023
Merged

Conversation

teknoraver
Copy link
Contributor

@teknoraver teknoraver commented Jan 12, 2023

GitPython parses the output of git diff --no-renames to get the files changed in a commit.
This breaks when a commit contains a file rename, because the output of git diff is different than expected.
This is the output of a normal commit:

$ git diff --numstat 6ab9810cfe6c^ 6ab9810cfe6c
5       2       drivers/clk/zynqmp/divider.c

And this a commit containing a rename:

$ git diff --numstat db401875f438^ db401875f438
1       1       tools/testing/selftests/drivers/net/mlxsw/{spectrum-2 => }/devlink_trap_tunnel_ipip6.sh

This can be triggered by this code:

for commit in repo.iter_commits():
    print(commit.hexsha)
        for file in commit.stats.files:
            print(file)

Which will print for the normal commit:

6ab9810cfe6c8f3d8b8750c827d7870abd3751b9
'drivers/clk/zynqmp/divider.c'

And when there is a rename:

db401875f438168c5804b295b93a28c7730bb57a
('tools/testing/selftests/drivers/net/mlxsw/{spectrum-2 => '
'}/devlink_trap_tunnel_ipip6.sh')

Fix this by pasing the --no-renames option to git diff which ignores renames and print the same output as if the file was deleted from the old path and created in the new one:

$ git diff --numstat --no-renames db401875f438^ db401875f438
250     0       tools/testing/selftests/drivers/net/mlxsw/devlink_trap_tunnel_ipip6.sh
0       250     tools/testing/selftests/drivers/net/mlxsw/spectrum-2/devlink_trap_tunnel_ipip6.sh

@Byron Byron added this to the v3.1.31 - Bugfixes milestone Jan 12, 2023
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

It should be straightforward to add a test that motivates this changes. The test-suite uses fixture files which could be derived from an example repository.

With a test that fails without this change, the fix can be merged.

Thank you

@teknoraver
Copy link
Contributor Author

@Byron done, and I rewrote the commit message using GitPython hashes, so is simpler for anyone to test

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, this looks like the way to go!

Instead of checking what it shouldn't be (which is a huge set of possible values), could you instead check what it should be? How does a rename look like with the current interface, I'd be very interested to see that.

For posterity, here is what git thinks about the commit being used in the test:

commit 185d847ec7647fd2642a82d9205fb3d07ea71715
Author: Dominic <<redacted>@gmail.com>
Date:   Mon Jul 12 17:02:21 2021 +0100

    Update and rename test_pytest.yml to Future.yml

diff --git a/.github/workflows/test_pytest.yml b/.github/workflows/Future.yml
similarity index 92%
rename from .github/workflows/test_pytest.yml
rename to .github/workflows/Future.yml
index 627e720f..39146533 100644
--- a/.github/workflows/test_pytest.yml
+++ b/.github/workflows/Future.yml
@@ -5,7 +5,9 @@ name: Future
 
 on:
   push:
-    branches: [main]
+    branches: [ main ]
+  pull_request:
+    branches: [ main ]
 
 jobs:
   build:

@teknoraver
Copy link
Contributor Author

This is the output of git show, gitpython does git diff --numstat $commit^ $commit which gives:

3       1       .github/workflows/{test_pytest.yml => Future.yml}

And with --no-renames added:

57      0       .github/workflows/Future.yml
0       55      .github/workflows/test_pytest.yml

GitPython parses the output of `git diff --numstat` to get the
files changed in a commit.
This breaks when a commit contains a file rename, because the output
of `git diff` is different than expected.
This is the output of a normal commit:

    $ git diff --numstat 8f41a39^ 8f41a39
    30      5       test/test_repo.py

And this a commit containing a rename:

    $ git diff --numstat 185d847^ 185d847
    3       1       .github/workflows/{test_pytest.yml => Future.yml}

This can be triggered by this code:

    for commit in repo.iter_commits():
        print(commit.hexsha)
            for file in commit.stats.files:
                print(file)

Which will print for the normal commit:

    8f41a39
    'test/test_repo.py'

And when there is a rename:

    185d847
    '.github/workflows/{test_pytest.yml => Future.yml}'

Additionally, when a path member is removed, the file list become
a list of strings, breaking even more the caller. This is in the
Linux kernel tree:

    $ git diff --numstat db401875f438^ db401875f438
    1       1       tools/testing/selftests/drivers/net/mlxsw/{spectrum-2 => }/devlink_trap_tunnel_ipip6.sh

and GitPython parses it as:

    db401875f438168c5804b295b93a28c7730bb57a
    ('tools/testing/selftests/drivers/net/mlxsw/{spectrum-2 => '
    '}/devlink_trap_tunnel_ipip6.sh')

Fix this by pasing the --no-renames option to `git diff` which ignores
renames and print the same output as if the file was deleted from the
old path and created in the new one:

    $ git diff --numstat --no-renames 185d847^ 185d847
    57      0       .github/workflows/Future.yml
    0       55      .github/workflows/test_pytest.yml
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a million, that is what I had in mind.

Thanks again for contributing!

@Byron Byron merged commit 3901d4c into gitpython-developers:main Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants