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

Enable user to override default diff -M arg #1551

Merged
merged 4 commits into from Feb 2, 2023

Conversation

mellowed100
Copy link

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

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
Author

Let me know if this works for you. thanks!

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.

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
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
Copy link
Member

Byron commented Feb 2, 2023

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
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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants