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

["Tom Scogland via GitGitGadget" <gitgitgadget@gmail.com>]

From: Tom Scogland <scogland1@llnl.gov>

The documentation for archive describes the `--add-virtual-file` option
thusly:

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

The `--add-file` documentation is similar:

  The path of the file in the archive is built by concatenating the
  value of the last --prefix option (if any) before this --add-file and
  the basename of <file>.

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

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

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

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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1719%2Ftrws%2Fhonor-prefix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1719/trws/honor-prefix-v2
Pull-Request: https://github.com/git/git/pull/1719

Range-diff vs v1:

 1:  1b685d8ca1a ! 1:  b9a0ef282a3 archive: make --add-virtual-file honor --prefix
     @@ Metadata
       ## Commit message ##
          archive: make --add-virtual-file honor --prefix
      
     -    The documentation for archive states:
     +    The documentation for archive describes the `--add-virtual-file` option
     +    thusly:
      
            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.
     +    The `--add-file` documentation is similar:
      
     -    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.
     +      The path of the file in the archive is built by concatenating the
     +      value of the last --prefix option (if any) before this --add-file and
     +      the basename of <file>.
     +
     +    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.  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.
     +
     +    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
      
     -    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.
     +    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
      
          Signed-off-by: Tom Scogland <scogland1@llnl.gov>
      
     @@ archive.c: int write_archive_entries(struct archiver_args *args,
      +		strbuf_reset(&path_in_archive);
      +		if (info->base)
      +			strbuf_addstr(&path_in_archive, info->base);
     -+		strbuf_addstr(&path_in_archive, basename(path));
       		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_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);
      @@ archive.c: 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,
     @@ t/t5003-archive-zip.sh: test_expect_success UNZIP 'git archive --format=zip --ad
       		--add-virtual-file=\""$PATHNAME"\": \
      -		--add-virtual-file=hello:world $EMPTY_TREE &&
      +		--add-virtual-file=hello:world \
     -+		--prefix=subdir/ --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 && (
     @@ t/t5003-archive-zip.sh: test_expect_success UNZIP 'git archive --format=zip --ad
       		test_path_is_file "$PATHNAME" &&
      -		test world = $(cat hello)
      +		test world = $(cat hello) &&
     -+		test_path_is_file subdir/hello &&
     -+		test world = $(cat subdir/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)
       	)
       '
       


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

base-commit: 786a3e4b8d754d2b14b1208b98eeb0a554ef19a8
-- 
gitgitgadget