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

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

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

> Notably both explicitly state that they honor the last `--prefix` option
> before the `--add` option in question.  The implementation of
> `--add-file` seems to have always honored prefix, but the implementation
> of `--add-virtual-file` does not.

The above is misleading.

    The implementation of `--add-file` has always honored the prefix,
    while the implementation of `--add-virtual-file` has always ignored
    the prefix.

would make it easier to assess how long existing users may have been
relying on the current behaviour.

> Also note that `--add-virtual-file`
> explicitly states it will use the full path given, while `--add-file`
> uses the basename of the path it is given.

Yes, this is a very good thing to mention.  It is probably the
reason why the implementation decided not to add prefix to the "full
path" that already can have the leading directories.

> Modify archive.c to include the prefix in the path used by
> `--add-virtual-file` and add checks into
> the existing add-virtual-file test to verify:
>
> * that `--prefix` is honored
> * that leading path components are preserved
> * that both work together and separately

Very nice job explaining the chosen design clearly (even though I do
not necessarily agree with the direction this patch is going).

Also, given that this option was introduced for an explicit purpose
of using it to write out diagnostics archive file, we should mention
that this change does not break it in the proposed log message, at
least.  Of course, we should do so after verifying that is indeed
the case, and better yet, after verifying that it will be hard for
future changes to diagnose.c to trigger an unexpected behaviour
caused by this change [*].

> Changes since v1:
> - Revised the commit message style
> - Added tests for basename/non-basename behavior
> - Fixed archive.c to use full path for virtual and basename for add-file

The "changes since v1" section does not belong to the log message
proper, as v1 never happened as long as readers of "git log" are
concerned.  It is a very good thing to help reviewers to have below
the three-dash lines that comes after your sign-off, though.

> Signed-off-by: Tom Scogland <scogland1@llnl.gov>
> ---

>  archive.c              | 10 +++++-----
>  t/t5003-archive-zip.sh | 14 ++++++++++++--
>  2 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index 5287fcdd8e0..64777a9870d 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -365,12 +365,11 @@ int write_archive_entries(struct archiver_args *args,
>  
>  		put_be64(fake_oid.hash, i + 1);
>  
> +		strbuf_reset(&path_in_archive);
> +		if (info->base)
> +			strbuf_addstr(&path_in_archive, info->base);
>  		if (!info->content) {
> -			strbuf_reset(&path_in_archive);
> -			if (info->base)
> -				strbuf_addstr(&path_in_archive, info->base);
>  			strbuf_addstr(&path_in_archive, basename(path));
> -
>  			strbuf_reset(&content);
>  			if (strbuf_read_file(&content, path, info->stat.st_size) < 0)
>  				err = error_errno(_("cannot read '%s'"), path);
> @@ -380,8 +379,9 @@ int write_archive_entries(struct archiver_args *args,
>  						  canon_mode(info->stat.st_mode),
>  						  content.buf, content.len);
>  		} else {
> +			strbuf_addstr(&path_in_archive, path);
>  			err = write_entry(args, &fake_oid,
> -					  path, strlen(path),
> +					  path_in_archive.buf, path_in_archive.len,
>  					  canon_mode(info->stat.st_mode),
>  					  info->content, info->stat.st_size);
>  		}
> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
> index 961c6aac256..0cf3aef8ace 100755
> --- a/t/t5003-archive-zip.sh
> +++ b/t/t5003-archive-zip.sh
> @@ -218,14 +218,24 @@ 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 \
> +		--add-virtual-file=with/dir/noprefix:withdirnopre \
> +		--prefix=subdir/ --add-virtual-file=with/dirprefix:withdirprefix \
> +		--prefix=subdir2/ --add-virtual-file=withoutdir:withoutdir \
> +		--prefix= $EMPTY_TREE &&
>  	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 with/dir/noprefix &&
> +		test withdirnopre = $(cat with/dir/noprefix) &&
> +		test_path_is_file subdir/with/dirprefix &&
> +		test withdirprefix = $(cat subdir/with/dirprefix) &&
> +		test_path_is_file subdir2/withoutdir &&
> +		test withoutdir = $(cat subdir2/withoutdir)

OK.  With different payload at different paths, it is easier than
the previous round to see where things are expected to go in the
result.


[Footnote]

 * I got curious and did this part for you.  After calling
   add_directory_to_archiver() that uses "--prefix" to move the
   target directory around in the output hierarchy, the code clears
   with "--prefix="---even a future change to diagnose.c adds more
   uses to --add-virtual-file= after it happens, it will not go to
   deep in the directory hierarchy where the last use of "--prefix"
   happened to be pointing at.