On 15 May 2024, at 8:23, Junio C Hamano wrote: > "Tom Scogland via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> From: Tom Scogland <scogland1@xxxxxxxx> >> >> 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". Ok, I'll update the message with this and the below style. >> 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. I wouldn't mind doing this necessarily, but would want an option that follows the documentation in addition. The current implementation is harder to use consistently with `--add-file` and with `--prefix`, though it's possible to manually prefix each virtual file it's surprising that it produces (as far as I have found) the only files in the archive that don't fall under the prefix. >> In looking for others with this issue, I found message >> a143e25a70b44b82b4ee6fa3bb2bcda4@xxxxxxxxxxxxxxxxxxxx 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. Certainly. >> Signed-off-by: Tom Scogland <scogland1@xxxxxxxx> >> --- >> 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. Thanks for the feedback, I'll get an updated patch posted later today.
