Re: [PATCH 5/8] teach remote-testgit to import non-HEAD refs

Jeff King [off-list ref] writes:

This seems like an accident waiting to happen, even though it is Ok with
the current code (because this method does not modify refs in any way), to
specify a mutable object as the default value for an optional parameter.

Hmm, am I looking at the right version of this file?

I see args.append("HEAD") after this --import-marks in the existing code,
which this patch does not remove.

An existing script that does not pass the new argument would end up
feeding HEAD twice, and if a new script does pass the new argument that
does not have "HEAD", it would still feed HEAD, to the underlying
fast-export.

I suspect that duplicated HEAD would not hurt for existing callers, but I
am not sure feeding HEAD unconditionally is a right thing to do.

On Wed, Jun 08, 2011 at 04:21:21PM -0700, Junio C Hamano wrote:

quoted

diff --git a/git_remote_helpers/git/exporter.py b/git_remote_helpers/git/exporter.py
index f40f9d6..1855c6a 100644
--- a/git_remote_helpers/git/exporter.py
+++ b/git_remote_helpers/git/exporter.py

@@ -15,7 +15,7 @@ class GitExporter(object):
 
         self.repo = repo
 
-    def export_repo(self, base):
+    def export_repo(self, base, refs = ["HEAD"]):

This seems like an accident waiting to happen, even though it is Ok with
the current code (because this method does not modify refs in any way), to
specify a mutable object as the default value for an optional parameter.

Yeah, I didn't realize that python assigns default values by reference,
unlike other languages (this is actually the first python I've ever
written).

At any rate, I think this patch is obsoleted by Sverre's new series
(though I haven't read it in detail yet).

quoted

         sys.stdout.flush()
 
         args = ["git", "--git-dir=" + self.repo.gitpath, "fast-export", "--export-marks=" + path]
+	args.extend(refs)
 
         if os.path.exists(path):
             args.append("--import-marks=" + path)

Hmm, am I looking at the right version of this file?

I see args.append("HEAD") after this --import-marks in the existing code,
which this patch does not remove.

Ugh, yeah. I missed that "HEAD". So this version is buggy in that it
exports everything you asked for, _plus_ HEAD. Which isn't a problem in
practice, but it's inefficient. That "HEAD" should definitely go away.

I'll check for it in Sverre's series and make sure he did it right. :)

-Peff