Re: [PATCH 5/8] teach remote-testgit to import non-HEAD refs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 8, 2011, 23:21 UTC
- Message-ID
- <7vipsf29la.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20110607172052.GD22111@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 10 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.
Show 8 quoted lines
> @@ -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.