PR#268: Avoid empty commit when convert does no do anything - fedora-infra/rpmautospec

fedora-infra / rpmautospec

#268 Avoid empty commit when convert does no do anything

Merged 2 years ago by nphilipp. Opened 2 years ago by oturpe.

fedora-infra/ oturpe/rpmautospec avoid-empty-commit into main

Avoid empty commit when convert does no do anything

Otto Liljalaakso • 2 years ago

c702b79

rpmautospec/subcommands/convert.py

file modified

+7 -2

		`@@ -167,8 +167,13 @@`
		`signature.email,`
		`ref.shorthand,`
		`)`
		`- oid = self.repo.create_commit(ref.name, signature, signature, message, tree, [parent.oid])`
		`- log.debug("Committed %s to repository", oid)`
		`+ if self.repo.diff(tree, parent, cached=True).patch:`
		`+ oid = self.repo.create_commit(`
		`+ ref.name, signature, signature, message, tree, [parent.oid]`
		`+ )`
		`+ log.debug("Committed %s to repository", oid)`
		`+ else:`
		`+ log.warning("Nothing to commit")`


		`def register_subcommand(subparsers):`

tests/rpmautospec/subcommands/test_convert.py

file modified

+5 -1

		`@@ -282,6 +282,10 @@`
		`*(patch.delta.old_file.path for patch in diff),`
		`}`

		`- assert head.message == "Convert to rpmautospec."`
		`+ if changelog_should_change or release_should_change:`
		`+ expected_message = "Convert to rpmautospec."`
		`+ else:`
		`+ expected_message = "Did nothing!"`
		`+ assert head.message == expected_message`
		`assert (specfile.name in fileschanged) == (changelog_should_change or release_should_change)`
		`assert ("changelog" in fileschanged) == changelog_should_change`

oturpe commented 2 years ago

Resolves #264

Signed-off-by: Otto Liljalaakso otto.liljalaakso@iki.fi

oturpe commented 2 years ago

Seems simple enough, and worked correctly on rubygem-sync when I tested.
But a word of warning: this is the first time I do anything with pygit2.

zuul commented 2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

fi-pre-commit : FAILURE in 1m 04s
fi-pytest-rawhide : FAILURE in 2m 25s
fi-pytest-f36 : FAILURE in 2m 26s
fi-pytest-f35 : FAILURE in 2m 26s
rpm-mock-build : FAILURE in 3m 46s

rebased onto a2edb390324c0f2467d645a103c71e4d5e3f0b7a

2 years ago

zuul commented 2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

fi-pre-commit : SUCCESS in 1m 03s
fi-pytest-rawhide : FAILURE in 2m 21s
fi-pytest-f36 : FAILURE in 2m 25s
fi-pytest-f35 : FAILURE in 2m 27s
rpm-mock-build : FAILURE in 4m 30s

oturpe commented 2 years ago

I will take a look at the failing tests when I find some time.

rebased onto 961e96f2c050e892be8c84d5f1edf00c4b45b6bc

2 years ago

rebased onto ee8227426567e685b7234e59ab2df3cce2ac643c

2 years ago

zuul commented 2 years ago

Build succeeded.

fi-pre-commit : SUCCESS in 1m 05s
fi-pytest-rawhide : SUCCESS in 2m 23s
fi-pytest-f36 : SUCCESS in 2m 27s
fi-pytest-f35 : SUCCESS in 2m 28s
rpm-mock-build : SUCCESS in 4m 23s

oturpe commented 2 years ago

Parameterized test case test_commit at tests/rpmautospec/subcommands/test_convert.py
was actually checking that an empty commit is created even if nothing changes.
I added a special case there to match the new behavior.

rebased onto 5fa6c28221ea6de7dc76171f4a93055ea86f0528

2 years ago

zuul commented 2 years ago

Build succeeded.

fi-pre-commit : SUCCESS in 1m 15s
fi-pytest-rawhide : SUCCESS in 4m 32s
fi-pytest-f36 : SUCCESS in 2m 30s
fi-pytest-f35 : SUCCESS in 2m 57s
rpm-mock-build : SUCCESS in 4m 03s

nphilipp commented on line 7 of rpmautospec/subcommands/convert.py 2 years ago

At least here, self.repo.diff(...).patch seems to be an empty string, not None:

(rpmautospec) nils@makake:~/dist-git/fedora/rpms/rubygem-sync (rawhide)> git reset --hard @{u}
HEAD is now at c7f6ece Switch to SPDX license ids
(rpmautospec) nils@makake:~/dist-git/fedora/rpms/rubygem-sync (rawhide)> rpmautospec convert
'/home/nils/dist-git/fedora/rpms/rubygem-sync/rubygem-sync.spec' is already using %autochangelog
'/home/nils/dist-git/fedora/rpms/rubygem-sync/rubygem-sync.spec' uses %autorelease already
(rpmautospec) nils@makake:~/dist-git/fedora/rpms/rubygem-sync (rawhide)> git show
commit 2b31912ba67e13cf509952b34d4f6cbfe261d980 (HEAD -> rawhide)
Author:     Nils Philippsen <nils@redhat.com>
AuthorDate: Fri Aug 19 14:34:41 2022 +0200
Commit:     Nils Philippsen <nils@redhat.com>
CommitDate: Fri Aug 19 14:34:41 2022 +0200

    Convert to rpmautospec
(rpmautospec) nils@makake:~/dist-git/fedora/rpms/rubygem-sync (rawhide)>

Not sure why, maybe you and I have different versions of pygit2/libgit2? Anyway, it works with this change applied:

(rpmautospec) nils@makake:~/src/fedora-infra/rpmautospec ((HEAD detached at oturpe/avoid-empty-commit))> git diff
diff --git a/rpmautospec/subcommands/convert.py b/rpmautospec/subcommands/convert.py
index 5319a69..4c3ee3c 100644
--- a/rpmautospec/subcommands/convert.py
+++ b/rpmautospec/subcommands/convert.py
@@ -167,7 +167,7 @@ class PkgConverter:
             signature.email,
             ref.shorthand,
         )
-        if self.repo.diff(tree, parent, cached=True).patch is not None:
+        if self.repo.diff(tree, parent, cached=True).patch:
             oid = self.repo.create_commit(
                 ref.name, signature, signature, message, tree, [parent.oid]
             )
(rpmautospec) nils@makake:~/src/fedora-infra/rpmautospec ((HEAD detached at oturpe/avoid-empty-commit))>

Would you apply that and re-push? Thanks!

rebased onto c702b79

2 years ago

zuul commented 2 years ago

Build succeeded.

fi-pre-commit : SUCCESS in 1m 07s
fi-pytest-rawhide : SUCCESS in 6m 10s
fi-pytest-f36 : SUCCESS in 2m 34s
fi-pytest-f35 : SUCCESS in 2m 34s
rpm-mock-build : SUCCESS in 4m 21s

oturpe commented 2 years ago

Yes, pygit2 documentation was a bit cryptic regarding what is in patch if the commit is empty.
it only says is "can be" None.
Anyhow, your fix that works for both has now been applied.