Skip to content

fix files list on file rename - #1537

Merged
Byron merged 1 commit into
gitpython-developers:mainfrom
teknoraver:main
Jan 14, 2023
Merged

fix files list on file rename#1537
Byron merged 1 commit into
gitpython-developers:mainfrom
teknoraver:main

Conversation

@teknoraver

@teknoraver teknoraver commented Jan 12, 2023

Copy link
Copy Markdown
Contributor

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

@Byron Byron left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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

@Byron Byron left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

@Byron Byron left a comment

Copy link
Copy Markdown
Member

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

2 participants