Fix iter_change_type diff renamed property to prevent warning

[kamilkrzyskow]

Hello 👋,
in #1886 and e7dec7d a proper warning message was introduced for the usage of Diff.renamed pointing to use Diff.renamed_file.
However, the usage of this property wasn't changed in the iter_change_type (since 3 years at that), so the internals use deprecated code 😞

GitPython/git/diff.py

Lines 328 to 329 in 9fbfb71

	elif change_type == "R" and diffidx.renamed:
	yield diffidx

Given that the code wasn't reported yet, perhaps I'm doing something wrong using the iter_change_type and there are better ways 👀 I'm using a custom MkDocs hook to run GitPython and check for renames to automatically create redirects mappings for paths, and another plugin handles the redirect creation for the static pages.
Here is the line which triggered the warning

• https://github.com/Gothic-Modding-Community/gmc/blob/b15fc318827d18ae1c2b67a4a73bc3d74c08b0ea/overrides/.hooks/git_redirects.py#L128

And the CI:

• https://github.com/Gothic-Modding-Community/gmc/actions/runs/9248064690/job/25437812921#step:8:21

Warning

As this is a one line change, I took the liberty to omit setting up an environment and just used the GitHub GUI to make a small change.
I also didn't investigate deeper to check if the iter_change_type has any tests.

Thanks for your time ✌️

[Byron]

Thanks a for investigating this issue!

Unfortunately, no existing test fails, showing wrong expectations, which means that it's entirely unclear what this change does. Since the diff-code is confusing enough as it is, any change must be accompanied by a test that protects it from regression and makes clear what the purpose of the change is.

Of course I understand that producing a test that fails without this change, ideally pushed in a commit before this one, is additional work, and it's understandable this request might go beyond what you can provide. If so, please feel free to close this PR, but otherwise, please do take your time, it's appreciated.

[kamilkrzyskow]

OK, I expected this turn of events 😅, I'll see what I can do over the week.

EDIT:
I see that you allow for testing many things in one test function:

GitPython/test/test_diff.py

Lines 102 to 128 in 14066e2

	def test_diff_with_rename(self):
	output = StringProcessAdapter(fixture("diff_rename"))
	diffs = Diff._index_from_patch_format(self.rorepo, output)
	self._assert_diff_format(diffs)
	self.assertEqual(1, len(diffs))
	diff = diffs[0]
	self.assertTrue(diff.renamed_file)
	self.assertTrue(diff.renamed)
	self.assertEqual(diff.rename_from, "Jérôme")
	self.assertEqual(diff.rename_to, "müller")
	self.assertEqual(diff.raw_rename_from, b"J\xc3\xa9r\xc3\xb4me")
	self.assertEqual(diff.raw_rename_to, b"m\xc3\xbcller")
	assert isinstance(str(diff), str)
	output = StringProcessAdapter(to_raw(fixture("diff_rename_raw")))
	diffs = Diff._index_from_raw_format(self.rorepo, output)
	self.assertEqual(len(diffs), 1)
	diff = diffs[0]
	self.assertIsNotNone(diff.renamed_file)
	self.assertIsNotNone(diff.renamed)
	self.assertEqual(diff.rename_from, "this")
	self.assertEqual(diff.rename_to, "that")
	self.assertEqual(diff.change_type, "R")
	self.assertEqual(diff.score, 100)
	self.assertEqual(len(list(diffs.iter_change_type("R"))), 1)

I'll hijack this loose rule and just add a test for lack of warnings when running the iter_change_type inside those functions 👌

EDIT2:

Or in this file https://github.com/gitpython-developers/GitPython/blob/main/test/deprecation/test_basic.py 👌

[Byron]

Thanks a lot, and please do!

And maybe after doing that, putting matters into their own test won't be hard anymore, but if not, also no problem at all.

[kamilkrzyskow]

There you go, I added the test to the other deprecation tests in test_basic
I hope this will get released soon ;]

[Byron]

My apologies, I realize my mistake now! rename is deprecated and was used internally, even though renamed_file should be used.
I thought this change is a affecting the logic of the diff, even though it's not doing that at all.
🤦‍♂️

Oh well, thanks again for bearing with me here.

[Byron]

There must have been a misunderstanding. It's not about deprecation warnings, it's about adding a test that fails without the change presented here, while making clear what the issue truly is.