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

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

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

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

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

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

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