Re: Bug in git archive + .gitattributes + relative path

[Junio C Hamano <gitster@pobox.com>]

René Scharfe <l.s.r@web.de> writes:

>    $ git archive --strip-components=1 HEAD sha1dc | tar tf -
>    .gitattributes
>    LICENSE.txt
>    sha1.c
>    sha1.h
>    ubc_check.c
>    ubc_check.h

What should happen to paths that match the given pathspec that do
not have enough number of components?  E.g. "cache.h" when the
command is "git archive --strip-components=1 HEAD \*.h"?  Should it
be documented?

> The new option does not affect the paths of entries added by --add-file
> and --add-virtual-file because they are handcrafted to their desired
> values already.  Similarly, the value of --prefix is not subject to
> component stripping.

Very sensible.

> diff --git a/archive.c b/archive.c
> index 9aeaf2bd87..8308d4d9c4 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -166,6 +166,18 @@ static int write_archive_entry(const struct object_id *oid, const char *base,
>  		args->convert = check_attr_export_subst(check);
>  	}

We probably could save attribute lookup overhead by moving the new
logic a bit higher in the function?

No, that would invalidate the path_without_prefix variable by using
strbuf_remove() on &path, and will break the attribute look-up.  The
variable is used only once before this point and never used later,
but as an independent future-proofing, we may want to remove the
variable or narrow the scope.  It's totally out of scope of the
patch, though.

> +	if (args->strip_components > 0) {
> +		size_t orig_baselen = baselen;
> +		for (int i = 0; i < args->strip_components; i++) {
> +			const char *slash = memchr(base, '/', baselen);
> +			if (!slash)
> +				return S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0;
> +			baselen -= slash - base + 1;
> +			base = slash + 1;
> +		}
> +		strbuf_remove(&path, args->baselen, orig_baselen - baselen);
> +	}

Nice to see that the core logic of the new feature is surprisingly
small.

>  	if (args->verbose)
>  		fprintf(stderr, "%.*s\n", (int)path.len, path.buf);

By having the verbose output after the path stripping, we won't show
the leading components we stripped, making it similar to what we
would see when we piped the resulting archive to "| tar tf -".  I
guess this makes sense than showing the original path.