Re: [PATCH] archive: fix archive generation for empty trees

[Jeff King <peff@peff.net>]

[+cc René, as this is his code;
 +cc Duy, for pathspec questions]

On Wed, Mar 07, 2012 at 04:09:22PM -0800, Brodie Rao wrote:

> Prior to this change, git-archive would try to verify path arguments -
> even if none were provided. It used get_pathspec("", pathspec), which
> would return a pathspec of "" instead of NULL.
> 
> Then it would try to verify if the tree contained any paths matching
> "". This is fine in the normal case where the tree contains anything
> (every entry would match), but for an empty tree, it wouldn't match,
> and you'd get this error:
> 
>   fatal: path not found:
> 
> Now, instead of "", we use a pathspec prefix of NULL. If no path
> arguments were provided, get_pathspec() will return NULL, and we won't
> try to verify the existence of any paths in the tree.

Yeah, this looks like the right thing to do. The get_pathspec code
treats a NULL prefix specially as "no prefix", and I think that is what
we are trying to say here (i.e., we are interpreting pathspecs from the
root).

Though the main function of get_pathspec seems to be to call
prefix_pathspec on each element of the pathspec. And we have no prefix
here. However, prefix_pathspec does a lot of magic parsing; it's unclear
to me whether this is all in support of properly adding the prefix, or
if its side effects are important. But if it is purely about prefixing,
can't we just get rid of the call to get_pathspec entirely?

There is also a comment above prefix_pathspec regarding moving things
over to the new "struct pathspec" interface. But that leaves me more
confused, since init_pathspec does not handle prefixes at all (so it
looks like you would need to call get_pathspec first to get a
prefixed list, and then feed the result to init_pathspec. Should we be
doing something with "struct pathspec" here? Confused...

-Peff

PS The patch itself is quoted below without further comment for the
   benefit of those who were cc'd.

> ---
>  archive.c           |    2 +-
>  t/t5000-tar-tree.sh |   16 ++++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/archive.c b/archive.c
> index 1ee837d..6e23896 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -236,7 +236,7 @@ static int path_exists(struct tree *tree, const char *path)
>  static void parse_pathspec_arg(const char **pathspec,
>  		struct archiver_args *ar_args)
>  {
> -	ar_args->pathspec = pathspec = get_pathspec("", pathspec);
> +	ar_args->pathspec = pathspec = get_pathspec(NULL, pathspec);
>  	if (pathspec) {
>  		while (*pathspec) {
>  			if (!path_exists(ar_args->tree, *pathspec))
> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 527c9e7..404786f 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -360,4 +360,20 @@ test_expect_success GZIP 'remote tar.gz can be disabled' '
>  		>remote.tar.gz
>  '
>  
> +test_expect_success \
> +    'git archive with an empty tree and a prefix' \
> +    'git rm -r . &&
> +     git commit -m empty &&
> +     git archive --format=tar --prefix=empty/ HEAD > e1.tar &&
> +     "$TAR" tf e1.tar'
> +
> +test_expect_success \
> +    'git archive with an empty tree and no prefix' \
> +    'git archive --format=tar HEAD > e2.tar &&
> +     test_must_fail "$TAR" tf e2.tar'
> +
> +test_expect_success \
> +    'git archive on specific paths with an empty tree' \
> +    'test_must_fail git archive --format=tar --prefix=empty/ HEAD foo'
> +
>  test_done
> -- 
> 1.7.9.2
>