Skip to content

Enable user to override default diff -M arg - #1551

Merged
Byron merged 4 commits into
gitpython-developers:mainfrom
mellowed100:main
Feb 2, 2023
Merged

Enable user to override default diff -M arg#1551
Byron merged 4 commits into
gitpython-developers:mainfrom
mellowed100:main

Conversation

@mellowed100

Copy link
Copy Markdown

Small fix to allow overriding of default diff -M arg. Using --find-renames= or -M or --no-renames will disable -M.

Full discussion is here:
#1195

@mellowed100
mellowed100 marked this pull request as draft January 31, 2023 00:57
@mellowed100
mellowed100 marked this pull request as ready for review January 31, 2023 01:04

@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.

That looks simple enough, thanks a lot!

Is it possible to have a test leverage this newly gained capability?
Now it's possible to do that, previously it would generate an invalid command-line, so a test can probably check for a successful diff call.

@mellowed100

Copy link
Copy Markdown
Author

Let me know if this works for you. thanks!

Comment thread test/test_diff.py

@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.

This looks great already!

I left a comment with a tiny last request to complete the M journey with a show case on how callers can now use it themselves :).

@mellowed100

Copy link
Copy Markdown
Author

Hey Byron!
So I updated the test to address those changes you asked for. The initial commits of file_a.txt and file_b.txt have been modified to have a similarity index of 52%. So the default diff -M (50%) should still flag it as a renamed file. I added diff -M75% and diff -M40% to check for both added/deleted files and renamed files. Let me know if this works for you. Thanks!

@Byron Byron added this to the v3.1.31 - Bugfixes milestone Feb 2, 2023
@Byron

Byron commented Feb 2, 2023

Copy link
Copy Markdown
Member

Thanks a lot - doing it like this is fantastic and I couldn't imagine a way to better show the effect of the M flag.

I will merge once CI runs through. Silly me aborted the existing Cygwin run because it seemed stuck, but history shows it's taking between 13 and 19 minutes to run there - incredible especially since the linux hosts are finished in less than 3 minutes.

@Byron
Byron merged commit 6ed2285 into gitpython-developers:main Feb 2, 2023
@mellowed100

Copy link
Copy Markdown
Author

You're welcome! Thanks for your guidance and patience (This was my first PR). I really appreciate it! -Take care!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants