7:01 a.m.
By default, Git remotes are named 'origin', but some pyrpkg-based tools
might be used to interact with several remotes for the same package
module.
This change allows sites to define how remotes should be called by
simply overriding the Commands.remote attribute.
That attribute is set to "origin" though, to not break existing clones.
The rpkg bash completion resource had to be modified accordingly. Since
the fedpkg one already handles this properly, a simple copy-paste of
that specific line was enough.
---
This is important for us at $dayjob.
We have this rpkg-based tool called nbpkg, which we use to interact with
our packaging infrastructure, similar to fedpkg:
https://github.com/network-box/nbpkg
But as I'm also a Fedora packager, I like to be able to interact with
both our internal infrastructure, and the Fedora one, in the same Git
clone, as it allows me to easily flow changes in both directions.
This workflow requires having different names for each remote though,
they can't just be all called 'origin'. This patch makes this possible.
I can imagine a similar use case for RHEL packagers, which could
probably want to interact with both the Fedora and RHEL packaging
infrastructures in the same Git clone, flowing changes both ways.
I know RPMFusion would be interested as well (I added Nicolas as CC).
src/pyrpkg/__init__.py | 21 ++++++++++++++-------
src/rpkg.bash | 3 +--
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
index c5b4ae4..89a84a2 100644
--- a/src/pyrpkg/__init__.py
+++ b/src/pyrpkg/__init__.py
@@ -135,6 +135,9 @@ class Commands(object):
self._ver = None
self.log = log
+ # The local name of the remote (might be overridden by subclasses)
+ self.remote = "origin"
+
# Define properties here
# Properties allow us to "lazy load" various attributes, which also means
# that we can do clone actions without knowing things like the spec
@@ -961,6 +964,11 @@ class Commands(object):
else:
self.log.debug('Cloning %s' % giturl)
cmd.extend([giturl])
+
+ if not bare_dir:
+ # --bare and --origin are incompatible
+ cmd.extend(['--origin', self.remote])
+
self._run_command(cmd, cwd=path)
return
@@ -1009,14 +1017,14 @@ class Commands(object):
for branch in branches:
try:
# Make a local clone for our branch
- top_git.clone("--branch", branch, repo_path, branch)
+ top_git.clone("--branch", branch, '--origin',
self.remote,
+ repo_path, branch)
- # Set the origin correctly
+ # Set the remote correctly
branch_path = os.path.join(top_path, branch)
branch_git = git.Git(branch_path)
- branch_git.config("--replace-all",
"remote.origin.url", giturl)
- # Bad use of "origin" here, need to fix this when more than
one
- # remote is used.
+ branch_git.config(
+ "--replace-all", "remote.%s.url" %
self.remote, giturl)
except (git.GitCommandError, OSError), e:
raise rpkgError('Could not locally clone %s from %s: %s' %
(branch, repo_path, e))
@@ -1452,8 +1460,7 @@ class Commands(object):
self.log.debug('No local branch found, creating a new one')
totrack = None
for remote in remotes:
- # bad use of "origin" here, will have to be fixed
- if remote.replace('origin/', '') == branch:
+ if remote.endswith(branch):
totrack = remote
break
else:
diff --git a/src/rpkg.bash b/src/rpkg.bash
index 047c98d..a2e45b6 100644
--- a/src/rpkg.bash
+++ b/src/rpkg.bash
@@ -283,8 +283,7 @@ _rpkg_branch()
local git_options= format="--format %(refname:short)"
[[ -n $1 ]] && git_options="--git-dir=$1/.git"
- git $git_options for-each-ref $format 'refs/remotes/origin/*' \
- | sed 's,origin/,,'
+ git $git_options for-each-ref $format 'refs/remotes' | sed 's,.*/,,'
git $git_options for-each-ref $format 'refs/heads'
}
--
1.8.5.3
3:47 a.m.
Ping?
On Mon, 2014-03-10 at 20:01 +0800, Mathieu Bridon wrote:
By default, Git remotes are named 'origin', but some
pyrpkg-based tools
might be used to interact with several remotes for the same package
module.
This change allows sites to define how remotes should be called by
simply overriding the Commands.remote attribute.
That attribute is set to "origin" though, to not break existing clones.
The rpkg bash completion resource had to be modified accordingly. Since
the fedpkg one already handles this properly, a simple copy-paste of
that specific line was enough.
---
This is important for us at $dayjob.
We have this rpkg-based tool called nbpkg, which we use to interact with
our packaging infrastructure, similar to fedpkg:
https://github.com/network-box/nbpkg
But as I'm also a Fedora packager, I like to be able to interact with
both our internal infrastructure, and the Fedora one, in the same Git
clone, as it allows me to easily flow changes in both directions.
This workflow requires having different names for each remote though,
they can't just be all called 'origin'. This patch makes this possible.
I can imagine a similar use case for RHEL packagers, which could
probably want to interact with both the Fedora and RHEL packaging
infrastructures in the same Git clone, flowing changes both ways.
I know RPMFusion would be interested as well (I added Nicolas as CC).
src/pyrpkg/__init__.py | 21 ++++++++++++++-------
src/rpkg.bash | 3 +--
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
index c5b4ae4..89a84a2 100644
--- a/src/pyrpkg/__init__.py
+++ b/src/pyrpkg/__init__.py
@@ -135,6 +135,9 @@ class Commands(object):
self._ver = None
self.log = log
+ # The local name of the remote (might be overridden by subclasses)
+ self.remote = "origin"
+
# Define properties here
# Properties allow us to "lazy load" various attributes, which also means
# that we can do clone actions without knowing things like the spec
@@ -961,6 +964,11 @@ class Commands(object):
else:
self.log.debug('Cloning %s' % giturl)
cmd.extend([giturl])
+
+ if not bare_dir:
+ # --bare and --origin are incompatible
+ cmd.extend(['--origin', self.remote])
+
self._run_command(cmd, cwd=path)
return
@@ -1009,14 +1017,14 @@ class Commands(object):
for branch in branches:
try:
# Make a local clone for our branch
- top_git.clone("--branch", branch, repo_path, branch)
+ top_git.clone("--branch", branch, '--origin',
self.remote,
+ repo_path, branch)
- # Set the origin correctly
+ # Set the remote correctly
branch_path = os.path.join(top_path, branch)
branch_git = git.Git(branch_path)
- branch_git.config("--replace-all",
"remote.origin.url", giturl)
- # Bad use of "origin" here, need to fix this when more than
one
- # remote is used.
+ branch_git.config(
+ "--replace-all", "remote.%s.url" %
self.remote, giturl)
except (git.GitCommandError, OSError), e:
raise rpkgError('Could not locally clone %s from %s: %s' %
(branch, repo_path, e))
@@ -1452,8 +1460,7 @@ class Commands(object):
self.log.debug('No local branch found, creating a new one')
totrack = None
for remote in remotes:
- # bad use of "origin" here, will have to be fixed
- if remote.replace('origin/', '') == branch:
+ if remote.endswith(branch):
totrack = remote
break
else:
diff --git a/src/rpkg.bash b/src/rpkg.bash
index 047c98d..a2e45b6 100644
--- a/src/rpkg.bash
+++ b/src/rpkg.bash
@@ -283,8 +283,7 @@ _rpkg_branch()
local git_options= format="--format %(refname:short)"
[[ -n $1 ]] && git_options="--git-dir=$1/.git"
- git $git_options for-each-ref $format 'refs/remotes/origin/*' \
- | sed 's,origin/,,'
+ git $git_options for-each-ref $format 'refs/remotes' | sed
's,.*/,,'
git $git_options for-each-ref $format 'refs/heads'
}
--
Mathieu
11:44 a.m.
On 24/03/14 09:47, Mathieu Bridon wrote:
Ping?
On Mon, 2014-03-10 at 20:01 +0800, Mathieu Bridon wrote:
> By default, Git remotes are named 'origin', but some pyrpkg-based tools
> might be used to interact with several remotes for the same package
> module.
>
> This change allows sites to define how remotes should be called by
> simply overriding the Commands.remote attribute.
>
> That attribute is set to "origin" though, to not break existing clones.
>
> The rpkg bash completion resource had to be modified accordingly. Since
> the fedpkg one already handles this properly, a simple copy-paste of
> that specific line was enough.
> ---
> This is important for us at $dayjob.
>
> We have this rpkg-based tool called nbpkg, which we use to interact with
> our packaging infrastructure, similar to fedpkg:
>
> https://github.com/network-box/nbpkg
>
> But as I'm also a Fedora packager, I like to be able to interact with
> both our internal infrastructure, and the Fedora one, in the same Git
> clone, as it allows me to easily flow changes in both directions.
>
> This workflow requires having different names for each remote though,
> they can't just be all called 'origin'. This patch makes this possible.
>
> I can imagine a similar use case for RHEL packagers, which could
> probably want to interact with both the Fedora and RHEL packaging
> infrastructures in the same Git clone, flowing changes both ways.
>
> I know RPMFusion would be interested as well (I added Nicolas as CC).
>
> src/pyrpkg/__init__.py | 21 ++++++++++++++-------
> src/rpkg.bash | 3 +--
> 2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
> index c5b4ae4..89a84a2 100644
> --- a/src/pyrpkg/__init__.py
> +++ b/src/pyrpkg/__init__.py
> @@ -135,6 +135,9 @@ class Commands(object):
> self._ver = None
> self.log = log
>
> + # The local name of the remote (might be overridden by subclasses)
> + self.remote = "origin"
> +
In the meantime I've introduced self._branch_remote (and corresponding
self.branch_remote getter)
https://git.fedorahosted.org/cgit/rpkg.git/tree/src/pyrpkg/__init__.py#n143
which is used to get remote name from current branch by git track.
Having another remote variable might be confusing. I believe we cannot
easily merge these two member variables together so I suggest to rename
your variable to self.default_remote.
Are you ok with that?
> # Define properties here
> # Properties allow us to "lazy load" various attributes, which also
means
> # that we can do clone actions without knowing things like the spec
> @@ -961,6 +964,11 @@ class Commands(object):
> else:
> self.log.debug('Cloning %s' % giturl)
> cmd.extend([giturl])
> +
> + if not bare_dir:
> + # --bare and --origin are incompatible
> + cmd.extend(['--origin', self.remote])
> +
> self._run_command(cmd, cwd=path)
>
> return
> @@ -1009,14 +1017,14 @@ class Commands(object):
> for branch in branches:
> try:
> # Make a local clone for our branch
> - top_git.clone("--branch", branch, repo_path, branch)
> + top_git.clone("--branch", branch, '--origin',
self.remote,
> + repo_path, branch)
>
> - # Set the origin correctly
> + # Set the remote correctly
> branch_path = os.path.join(top_path, branch)
> branch_git = git.Git(branch_path)
> - branch_git.config("--replace-all",
"remote.origin.url", giturl)
> - # Bad use of "origin" here, need to fix this when more
than one
> - # remote is used.
> + branch_git.config(
> + "--replace-all", "remote.%s.url" %
self.remote, giturl)
> except (git.GitCommandError, OSError), e:
> raise rpkgError('Could not locally clone %s from %s: %s' %
> (branch, repo_path, e))
> @@ -1452,8 +1460,7 @@ class Commands(object):
> self.log.debug('No local branch found, creating a new one')
> totrack = None
> for remote in remotes:
> - # bad use of "origin" here, will have to be fixed
> - if remote.replace('origin/', '') == branch:
> + if remote.endswith(branch):
> totrack = remote
> break
> else:
> diff --git a/src/rpkg.bash b/src/rpkg.bash
> index 047c98d..a2e45b6 100644
> --- a/src/rpkg.bash
> +++ b/src/rpkg.bash
> @@ -283,8 +283,7 @@ _rpkg_branch()
> local git_options= format="--format %(refname:short)"
> [[ -n $1 ]] && git_options="--git-dir=$1/.git"
>
> - git $git_options for-each-ref $format 'refs/remotes/origin/*' \
> - | sed 's,origin/,,'
> + git $git_options for-each-ref $format 'refs/remotes' | sed
's,.*/,,'
> git $git_options for-each-ref $format 'refs/heads'
> }
>
Except of note above your patch seems to work as advertised and I
haven't found a way to break backwards compatibility.
--
Pavol Babincak
Release Engineering, Red Hat
11:56 a.m.
On Mon, 2014-03-24 at 17:44 +0100, Pavol Babincak wrote:
In the meantime I've introduced self._branch_remote (and
corresponding
self.branch_remote getter)
https://git.fedorahosted.org/cgit/rpkg.git/tree/src/pyrpkg/__init__.py#n143
I saw that, yeah.
But that doesn't work, as the code still hardcodes some 'origin' here
and there, which you just left in.
which is used to get remote name from current branch by git track.
Having another remote variable might be confusing. I believe we cannot
easily merge these two member variables together so I suggest to rename
your variable to self.default_remote.
Are you ok with that?
I think a better way would be:
1. your _branch_remote defaults to 'origin', rather than raising an
error when it can't be determined automatically
2. the rest of my patch is merged, using your "branch_remote" instead of
the new "remote" I was introducing.
That way, we have the best of both worlds:
- my change which allows pyrpkg-based tools to set the remote the way
they want, and it is respected without hardcoding 'origin'
- your change which allows detecting the remote when possible
I already reworked my patches to implement the above, but I won't submit
them without testing first, and I can't really test from here, so that
will have to wait until I get to work tomorrow. ;)
Except of note above your patch seems to work as advertised and I
haven't found a way to break backwards compatibility.
Good, I explicitly kept it.
--
Mathieu
8:19 a.m.
New subject: [PATCH 1/2] Fallback the remote on 'origin'
The name 'origin' is the default remote name with Git, so rather than
raising an exception, we should fall back on it.
---
src/pyrpkg/__init__.py | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
index 31e516c..f3b7362 100644
--- a/src/pyrpkg/__init__.py
+++ b/src/pyrpkg/__init__.py
@@ -285,8 +285,11 @@ class Commands(object):
try:
remote = self.repo.git.config('--get', 'branch.%s.remote'
% self.branch_merge)
- except git.GitCommandError, e:
- raise rpkgError('Unable to find remote name: %s' % e)
+ except Exception as e:
+ self.log.debug("Could not determine the remote name, falling back"
+ " to 'origin'")
+ remote = "origin"
+
self._branch_remote = remote
@property
--
1.9.3
8:19 a.m.
New subject: [PATCH 2/2] De-hardcode 'origin' as the remote name
By default, Git remotes are named 'origin', but some pyrpkg-based tools
might be used to interact with several remotes for the same package
module.
This change allows sites to define how remotes should be called by
simply overriding the Commands.branch_remote attribute.
The rpkg bash completion resource had to be modified accordingly. Since
the fedpkg one already handles this properly, a simple copy-paste of
that specific line was enough.
---
src/pyrpkg/__init__.py | 16 ++++++++++------
src/rpkg.bash | 3 +--
2 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
index f3b7362..362bbca 100644
--- a/src/pyrpkg/__init__.py
+++ b/src/pyrpkg/__init__.py
@@ -1057,6 +1057,11 @@ class Commands(object):
else:
self.log.debug('Cloning %s' % giturl)
cmd.extend([giturl])
+
+ if not bare_dir:
+ # --bare and --origin are incompatible
+ cmd.extend(['--origin', self.branch_remote])
+
self._run_command(cmd, cwd=path)
return
@@ -1105,14 +1110,14 @@ class Commands(object):
for branch in branches:
try:
# Make a local clone for our branch
- top_git.clone("--branch", branch, repo_path, branch)
+ top_git.clone("--branch", branch, "--origin",
self.branch_remote,
+ repo_path, branch)
# Set the origin correctly
branch_path = os.path.join(top_path, branch)
branch_git = git.Git(branch_path)
- branch_git.config("--replace-all",
"remote.origin.url", giturl)
- # Bad use of "origin" here, need to fix this when more than
one
- # remote is used.
+ branch_git.config("--replace-all",
+ "remote.%s.url" % self.branch_remote, giturl)
except (git.GitCommandError, OSError), e:
raise rpkgError('Could not locally clone %s from %s: %s' %
(branch, repo_path, e))
@@ -1548,8 +1553,7 @@ class Commands(object):
self.log.debug('No local branch found, creating a new one')
totrack = None
for remote in remotes:
- # bad use of "origin" here, will have to be fixed
- if remote.replace('origin/', '') == branch:
+ if remote.endswith(branch):
totrack = remote
break
else:
diff --git a/src/rpkg.bash b/src/rpkg.bash
index 047c98d..a2e45b6 100644
--- a/src/rpkg.bash
+++ b/src/rpkg.bash
@@ -283,8 +283,7 @@ _rpkg_branch()
local git_options= format="--format %(refname:short)"
[[ -n $1 ]] && git_options="--git-dir=$1/.git"
- git $git_options for-each-ref $format 'refs/remotes/origin/*' \
- | sed 's,origin/,,'
+ git $git_options for-each-ref $format 'refs/remotes' | sed 's,.*/,,'
git $git_options for-each-ref $format 'refs/heads'
}
--
1.9.3
6:52 a.m.
New subject: [PATCH 1/2] Fallback the remote on 'origin'
On 07/01/2014 03:19 PM, Mathieu Bridon wrote:
- except git.GitCommandError, e:
- raise rpkgError('Unable to find remote name: %s' % e)
+ except Exception as e:
Is there a reason to catch general Exception
instead of specific
GitCommandError which is raised by git module on errors?
Rest of the patch & second patch (De-hardcode 'origin' as the remote
name) looks fine with me.
--
Pavol Babincak
Release Engineering, Red Hat
8:15 a.m.
New subject: [PATCH 1/2] Fallback the remote on 'origin'
On Wed, 2014-07-16 at 13:52 +0200, Pavol Babincak wrote:
On 07/01/2014 03:19 PM, Mathieu Bridon wrote:
> - except git.GitCommandError, e:
> - raise rpkgError('Unable to find remote name: %s' % e)
> + except Exception as e:
Is there a reason to catch general Exception instead of specific
GitCommandError which is raised by git module on errors?
Yes:
the code which can raise an exception is the evaluation of the
`branch_merge` property:
def load_branch_merge(self):
[... snip ...]
try:
localbranch = self.repo.active_branch.name
except TypeError, e:
raise rpkgError('Repo in inconsistent state: %s' % e)
try:
merge = self.repo.git.config('--get',
'branch.%s.merge' % localbranch)
except git.GitCommandError, e:
raise rpkgError('Unable to find remote branch. Use --dist')
[... snip ...]
As you can see on the excerpt above, it can raise a rpkgError in some
situations, so the except block needs to catch it too.
In fact, I don't even think there's any way that the raised exception
could be a git.GitCommandError at all, in my testing I only ever got a
rpkgError.
I can understand that silently ignoring **all** potential errors in this
case is not very wise, so how about this instead:
- except git.GitCommandError, e:
+ except (git.GitCommandError, rpkgError) as e:
That avoids catching all errors silently, while still catching the ones
that matter.
(note: this only works with Python >= 2.6)
--
Mathieu
11:18 a.m.
New subject: [PATCH 1/2] Fallback the remote on 'origin'
The name 'origin' is the default remote name with Git, so rather than
raising an exception, we should fall back on it.
---
As discussed on IRC, this is the same patch as before, with the exception
handling improved so it doesn't catch all exceptions silently, only the
ones we expect.
src/pyrpkg/__init__.py | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
index 31e516c..95ce3f2 100644
--- a/src/pyrpkg/__init__.py
+++ b/src/pyrpkg/__init__.py
@@ -285,8 +285,11 @@ class Commands(object):
try:
remote = self.repo.git.config('--get', 'branch.%s.remote'
% self.branch_merge)
- except git.GitCommandError, e:
- raise rpkgError('Unable to find remote name: %s' % e)
+ except (git.GitCommandError, rpkgError) as e:
+ self.log.debug("Could not determine the remote name, falling back"
+ " to 'origin'")
+ remote = "origin"
+
self._branch_remote = remote
@property
--
1.9.3
6:28 a.m.
New subject: [PATCH 1/2] Fallback the remote on 'origin'
Both patches (modified [PATCH 1/2] Fallback the remote on 'origin' and
[PATCH 2/2] De-hardcode 'origin' as the remote name) applied and pushed
to master.
Thanks!
--
Pavol Babincak
Release Engineering, Red Hat
3558
days inactive
3693
days old
rel-eng@lists.fedoraproject.org
9 comments
2 participants
participants (2)
-
Mathieu Bridon -
Pavol Babincak