From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 08 Jun 2011 23:21:21 GMT
Subject: Re: [PATCH 5/8] teach remote-testgit to import non-HEAD refs
Message-ID: <7vipsf29la.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110607172052.GD22111@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

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

> @@ -38,6 +38,7 @@ class GitExporter(object):
>          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.

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.