[PATCH] Clone anonymously, authenticate for pushing

Mathieu Bridon bochecha at fedoraproject.org
Wed Jul 23 12:33:05 UTC 2014


A common source of (minor) frustration for non-packager-savvy people
trying to clone package trees is:

  $ fedpkg clone foobar
  Cloning into 'foobar'...
  Permission denied (publickey).
  fatal: Could not read from remote repository.

  Please make sure you have the correct access rights
  and the repository exists.
  Could not execute clone: Command '['git', 'clone', 'ssh://you@pkgs.fedoraproject.org/foobar']' returned non-zero exit status 128
  ERROR:rpkg:Could not execute clone: Command '['git', 'clone', 'ssh://you@pkgs.fedoraproject.org/foobar']' returned non-zero exit status 128

This is confusing for newcomers (it is not obvious they should have used
the --anonymous/-a option), and it is unneeded: authentication is only
really needed for pushing.

This change makes the --anonymous/-a options completely unnecessary:
- clones are now always done anonymously
- pushes now always require authentication

Using the options now prints a warning that they are unneeded, but keeps
them to avoid breaking existing scripts. The API parameters are also
kept to preserve compatibility with pyrpkg-based tools, but they are now
completely unused.
---
 src/pyrpkg/__init__.py | 53 ++++++++++++++++++++++++++++++--------------------
 src/pyrpkg/cli.py      | 15 +++++++++-----
 2 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/src/pyrpkg/__init__.py b/src/pyrpkg/__init__.py
index 425ba5f..8437c26 100644
--- a/src/pyrpkg/__init__.py
+++ b/src/pyrpkg/__init__.py
@@ -1014,7 +1014,7 @@ class Commands(object):
         self._run_command(cmd, cwd=self.path)
         return
 
-    def clone(self, module, path=None, branch=None, bare_dir=None, anon=False):
+    def clone(self, module, path=None, branch=None, bare_dir=None, anon=None):
         """Clone a repo, optionally check out a specific branch.
 
         module is the name of the module to clone
@@ -1026,19 +1026,22 @@ class Commands(object):
         bare_dir is the name of a directory to make a bare clone to, if this is a
         bare clone. None otherwise.
 
-        anon is whether or not to clone anonymously
+        anon is now unused, but kept for compatibility with older pyrpkg-based
+        tools.
 
         Logs the output and returns nothing.
 
         """
 
+        if anon is not None:
+             self.log.warning("The `anon` argument is now obsolete and "
+                              "unneeded")
+
         if not path:
             path = self.path
         # construct the git url
-        if anon:
-            giturl = self.anongiturl % {'module': module}
-        else:
-            giturl = self.gitbaseurl % {'user': self.user, 'module': module}
+        anon_giturl = self.anongiturl % {'module': module}
+        authed_giturl = self.gitbaseurl % {'user': self.user, 'module': module}
 
         # Create the command
         cmd = ['git', 'clone']
@@ -1049,14 +1052,14 @@ class Commands(object):
             raise rpkgError('Cannot combine bare cloning with a branch')
         elif branch:
             # For now we have to use switch branch
-            self.log.debug('Checking out a specific branch %s' % giturl)
-            cmd.extend(['-b', branch, giturl])
+            self.log.debug('Checking out a specific branch %s' % anon_giturl)
+            cmd.extend(['-b', branch, anon_giturl])
         elif bare_dir:
-            self.log.debug('Cloning %s bare' % giturl)
-            cmd.extend(['--bare', giturl, bare_dir])
+            self.log.debug('Cloning %s bare' % anon_giturl)
+            cmd.extend(['--bare', anon_giturl, bare_dir])
         else:
-            self.log.debug('Cloning %s' % giturl)
-            cmd.extend([giturl])
+            self.log.debug('Cloning %s' % anon_giturl)
+            cmd.extend([anon_giturl])
 
         if not bare_dir:
             # --bare and --origin are incompatible
@@ -1064,27 +1067,33 @@ class Commands(object):
 
         self._run_command(cmd, cwd=path)
 
+        repo = git.Repo(os.path.join(path, module))
+        repo.remotes["origin"].config_writer.set_value("pushurl", authed_giturl)
+
         return
 
-    def clone_with_dirs(self, module, anon=False):
+    def clone_with_dirs(self, module, anon=None):
         """Clone a repo old style with subdirs for each branch.
 
         module is the name of the module to clone
 
-        gitargs is an option list of arguments to git clone
+        anon is now unused, but kept for compatibility with older pyrpkg-based
+        tools.
 
         """
 
+        if anon is not None:
+             self.log.warning("The `anon` argument is now obsolete and "
+                              "unneeded")
+
         # Get the full path of, and git object for, our directory of branches
         top_path = os.path.join(self.path, module)
         top_git = git.Git(top_path)
         repo_path = os.path.join(top_path, 'rpkg.git')
 
-        # construct the git url
-        if anon:
-            giturl = self.anongiturl % {'module': module}
-        else:
-            giturl = self.gitbaseurl % {'user': self.user, 'module': module}
+        # construct the git urls
+        anon_giturl = self.anongiturl % {'module': module}
+        authed_giturl = self.gitbaseurl % {'user': self.user, 'module': module}
 
         # Create our new top directory
         try:
@@ -1095,7 +1104,7 @@ class Commands(object):
 
         # Create a bare clone first. This gives us a good list of branches
         try:
-            self.clone(module, top_path, bare_dir=repo_path, anon=anon)
+            self.clone(module, top_path, bare_dir=repo_path)
         except Exception, e:
             # Clean out our directory
             shutil.rmtree(top_path)
@@ -1117,7 +1126,9 @@ class Commands(object):
                 branch_path = os.path.join(top_path, branch)
                 branch_git = git.Git(branch_path)
                 branch_git.config("--replace-all",
-                        "remote.%s.url" % self.branch_remote, giturl)
+                        "remote.%s.url" % self.branch_remote, anon_giturl)
+                branch_git.config("--replace-all",
+                        "remote.%s.pushurl" % self.branch_remote, authed_giturl)
             except (git.GitCommandError, OSError), e:
                 raise rpkgError('Could not locally clone %s from %s: %s' %
                         (branch, repo_path, e))
diff --git a/src/pyrpkg/cli.py b/src/pyrpkg/cli.py
index 1ed8862..709d183 100755
--- a/src/pyrpkg/cli.py
+++ b/src/pyrpkg/cli.py
@@ -347,7 +347,8 @@ defined, packages will be built sequentially.""" %
         # allow to clone without needing a account on the scm server
         clone_parser.add_argument('--anonymous', '-a',
                                   action = 'store_true',
-                                  help = 'Check out a module anonymously')
+                                  help = 'Obsolete, kept for compatibility '
+                                         'only')
         # store the module to be cloned
         clone_parser.add_argument('module', nargs = 1,
                                   help = 'Name of the module to clone')
@@ -978,11 +979,9 @@ defined, packages will be built sequentially.""" %
 
     def clone(self):
         if self.args.branches:
-            self.cmd.clone_with_dirs(self.args.module[0],
-                                     anon=self.args.anonymous)
+            self.cmd.clone_with_dirs(self.args.module[0])
         else:
-            self.cmd.clone(self.args.module[0], branch=self.args.branch,
-                           anon=self.args.anonymous)
+            self.cmd.clone(self.args.module[0], branch=self.args.branch)
 
     def commit(self):
         if self.args.clog:
@@ -1341,6 +1340,12 @@ Tasks still running. You can continue to watch with the '%s watch-task' command.
 
         # Parse the args
         self.args = self.parser.parse_args()
+
+        if "anonymous" in self.args and self.args.anonymous:
+            # Logging is not setup yet
+            sys.stderr.write('WARNING: The --anonymous/-a option is now '
+                             'obsolete and unneeded\n')
+
         if self.args.user:
             self.user = self.args.user
         else:
-- 
1.9.3



More information about the rel-eng mailing list