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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 6, 2023, 16:56 UTC
Message-ID: <xmqqy1o9byye.fsf@gitster.g>
In-Reply-To: <70f10864-2cc7-cb9e-f868-2ac0011cad58@web.de>

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

Show 7 quoted lines

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

Show 7 quoted lines

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

Show 11 quoted lines

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