mock icon indicating copy to clipboard operation
mock copied to clipboard

`rpmautospec`-plugin's git repo check is too strict

Open dennisklein opened this issue 1 year ago • 10 comments

https://github.com/rpm-software-management/mock/blob/4b3fb22e2f74a17bf7879395b6f6418192430163/mock/py/mockbuild/plugins/rpmautospec.py#L61-L62

This check fails for git submodules and multiple git worktrees (in such cases the .git file may not be a directory). Since rpmautospec already depends on pygit2 I propose to improve the git repo check with pygit2.discover_repository() which already supports submodules and worktrees.

Test with python -c "import pygit2; import os; print(pygit2.discover_repository(os.getcwd()))"

Possible implementation:

if not pygit2.discover_repository(host_chroot_sources):

If pygit2 as a new dependency is not desired, one could fall back to the return code of git rev-parse --git-dir.

dennisklein avatar Jan 17 '24 16:01 dennisklein

@nphilipp fyi

praiskup avatar Jan 24 '24 13:01 praiskup

@sgallagher fyi

praiskup avatar Jan 24 '24 13:01 praiskup

@dennisklein thank you for reporting this!

praiskup avatar Jan 24 '24 14:01 praiskup

@dennisklein good catch! In fact, we don’t want to have pygit2 as a dependency in this plugin – we changed it intentionally so that the bulk of the code is executed in the build root and outside has only minimal dependencies.

nphilipp avatar Jan 25 '24 09:01 nphilipp

@praiskup I lean towards implementing this check in rpmautospec_core and adapting the mock plugin to use it. What do you think?

nphilipp avatar Jan 25 '24 10:01 nphilipp

@dennisklein Your check is too lenient, though 😉 (but the error message in the plugin is misleading): it works anywhere in the worktree, but it should only continue there if at the root. I guess I’ll just check for the presence of .git and if it’s a plain file, check that it begins with gitdir: .

nphilipp avatar Jan 25 '24 10:01 nphilipp

but it should only continue there if at the root

true, I didn't realize this aspect.

I guess I’ll just check for the presence of .git and if it’s a plain file, check that it begins with gitdir:

:+1: Thank you!

dennisklein avatar Jan 25 '24 12:01 dennisklein

@praiskup I lean towards implementing this check in rpmautospec_core and adapting the mock plugin to use it. What do you think?

Not sure if the mock plugin needs an update? But sounds good.

praiskup avatar Jan 26 '24 15:01 praiskup

I’ve looked into it more closely and it seems like just checking these couple files or directories doesn’t cover all the options how a git work tree can be defined. So to me the question is if we can live with not supporting these corner cases – which we could by delegating this step to git itself but then it would have to become a dependency (and I’m not sure if it’s worthwhile).

nphilipp avatar Feb 01 '24 12:02 nphilipp

So to me the question is if we can live with not supporting these corner cases

To my current use case (submodules) it would be acceptable.

which we could by delegating this step to git itself

This sounds more future-proof as we would not be relying on implementation details of git.

dennisklein avatar Feb 01 '24 13:02 dennisklein