Re: [PATCH v2] archive: initialize archivers earlier

[Josh Steadmon <steadmon@google.com>]

On 2018.10.22 18:35, Jeff King wrote:
> On Mon, Oct 22, 2018 at 02:48:11PM -0700, steadmon@google.com wrote:
> 
> > Initialize archivers as soon as possible when running git-archive and
> > git-upload-archive. Various non-obvious behavior depends on having the
> > archivers initialized, such as determining the desired archival format
> > from the provided filename.
> > 
> > Since 08716b3c11 ("archive: refactor file extension format-guessing",
> > 2011-06-21), archive_format_from_filename() has used the registered
> > archivers to match filenames (provided via --output) to archival
> > formats. However, when git-archive is executed with --remote, format
> > detection happens before the archivers have been registered. This causes
> > archives from remotes to always be generated as TAR files, regardless of
> > the actual filename (unless an explicit --format is provided).
> > 
> > This patch fixes that behavior; archival format is determined properly
> > from the output filename, even when --remote is used.
> > 
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > Helped-by: Jeff King <peff@peff.net>
> 
> Thanks, this looks good overall.
> 
> A few minor comments (that I'm not even sure are worth re-rolling for):
> 
> > diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> > index 25d9116356..3f35ebcfe8 100644
> > --- a/builtin/upload-archive.c
> > +++ b/builtin/upload-archive.c
> > @@ -43,6 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
> >  	}
> >  
> >  	/* parse all options sent by the client */
> > +	init_archivers();
> >  	return write_archive(sent_argv.argc, sent_argv.argv, prefix,
> >  			     the_repository, NULL, 1);
> >  }
> 
> This seems to separate the comment from what it describes. Any reason
> not to just init_archivers() closer to the top of the function here
> (probably after the enter_repo() call)?

Ack, fixed.


> > diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> > index 2a97b27b0a..3e95fdf660 100755
> > --- a/t/t5000-tar-tree.sh
> > +++ b/t/t5000-tar-tree.sh
> > @@ -206,6 +206,12 @@ test_expect_success 'git archive with --output, override inferred format' '
> >  	test_cmp_bin b.tar d4.zip
> >  '
> >  
> > +test_expect_success GZIP 'git archive with --output and --remote uses expected format' '
> > +	git archive --output=d5.tgz --remote=. HEAD &&
> > +	gzip -d -c < d5.tgz > d5.tar &&
> > +	test_cmp_bin b.tar d5.tar
> > +'
> 
> This nicely tests the more-interesting tgz case. But unfortunately it
> won't run on machines without the GZIP prerequisite. I'd think that
> would really be _most_ machines, but is it worth having a separate zip
> test to cover machines without gzip? I guess that just creates the
> opposite problem: not everybody has ZIP.

Added a test to compare the file lists from the .zip file to the
reference .tar file. I'm not sure if this is the best way to do things,
but it at least verifies that a .zip is produced. However, it's brittle
if the output of "zip -sf" changes. Let me know if you have a better
idea.