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

From: Jeff King <peff@peff.net>
Date: Jun 9, 2011, 00:17 UTC
Message-ID: <20110609001723.GB19715@sigill.intra.peff.net>
In-Reply-To: <7vipsf29la.fsf@alter.siamese.dyndns.org>

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

Show 14 quoted lines

> > 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).

Show 12 quoted lines

> >          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