Re: [PATCH] archive: make --add-virtual-file honor --prefix
On 15 May 2024, at 8:23, Junio C Hamano wrote:
Show 27 quoted lines
> "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".
Ok, I'll update the message with this and the below style.
Show 12 quoted lines
>> 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.
Show 7 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.
Show 43 quoted lines
>> 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.
Thanks for the feedback, I'll get an updated patch posted later today.