Re: [PATCH] archive: make --add-virtual-file honor --prefix

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2024, 15:23 UTC
Message-ID: <xmqqcypn14ld.fsf@gitster.g>
In-Reply-To: <pull.1719.git.git.1715721327429.gitgitgadget@gmail.com>

"Tom Scogland via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

> From: Tom Scogland <scogland1@llnl.gov>
>
> The documentation for archive states:
>
>   The path of the file in the archive is built by concatenating the
>   value of the last `--prefix` moption (if any) before this
>   `--add-virtual-file` and <path>.
>
> This matches the documentation for --add-file and the behavior works for
> that option, but --prefix is ignored for --add-virtual-file.

This first paragraph was a bit hard to parse for me.

You will contrast the quoted paragraph with another one you did not quote later in this paragraph, so it is more helpful to readers if you instead said:

    The documentatation for archive describes its
    "--add-virtual-file" option like so:

        ... excerpt from --add-virtual-file description ...

    This description is the same as "--add-file", and "--add-file"
    does behave the way as described.  "--add-virtual-file" however
    ignores "--prefix".

> This commit modifies archive.c to include the prefix in the path and
> adds a check into the existing add-virtual-file test to ensure that it
> honors both the most recent prefix before the flag.

Style: "This comit modifies" -> "Modify".

An obvious alternative fix is to update the documentation, which would be a much safer thing to do, given that there may be existing scripts written during the two years since --add-virtual-file option was introduced and has been behaving exactly this way. They will all be broken big time once the command starts honoring the "--prefix" option.

Show 5 quoted lines

> In looking for others with this issue, I found message
> a143e25a70b44b82b4ee6fa3bb2bcda4@atlas-elektronik.com on the mailing
> list, where Stefan proposed a basically identical patch to archive.c
> back in February, so the main addition here is the test along with that
> patch.

This pargraph should come _after_ the three-dash lines below.

> Signed-off-by: Tom Scogland <scogland1@llnl.gov>
> ---
>     archive: make --add-virtual-file honor --prefix

The implementation looked obvious, assuming that it is a good idea to change it (I've already talked about a safer alternative fix).

Show 12 quoted lines

> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
> index 961c6aac256..acc8bc4fcd6 100755
> --- a/t/t5003-archive-zip.sh
> +++ b/t/t5003-archive-zip.sh
> @@ -218,14 +218,18 @@ test_expect_success UNZIP 'git archive --format=zip --add-virtual-file' '
>  	fi &&
>  	git archive --format=zip >with_file_with_content.zip \
>  		--add-virtual-file=\""$PATHNAME"\": \
> -		--add-virtual-file=hello:world $EMPTY_TREE &&
> +		--add-virtual-file=hello:world \
> +		--prefix=subdir/ --add-virtual-file=hello:world \
> +		--prefix= $EMPTY_TREE &&

Instead of reusing the exactly the same name and contents, use something different so that it is clear to the later test which of the two "--add-virtual-file" options created these two paths in the unpacked directories. I.e., create something like

	--prefix=subdir/ --add-virtual-file=good:night

here and update the test below to match.

Show 12 quoted lines

>  	test_when_finished "rm -rf tmp-unpack" &&
>  	mkdir tmp-unpack && (
>  		cd tmp-unpack &&
>  		"$GIT_UNZIP" ../with_file_with_content.zip &&
>  		test_path_is_file hello &&
>  		test_path_is_file "$PATHNAME" &&
> -		test world = $(cat hello)
> +		test world = $(cat hello) &&
> +		test_path_is_file subdir/hello &&
> +		test world = $(cat subdir/hello)
>  	)
>  '

Other than that, looks good to me.  Thanks.