Re: [PATCH v2] archive: make --add-virtual-file honor --prefix
On 17 May 2024, at 16:33, Junio C Hamano wrote:
Show 15 quoted lines
> "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.
Fair, I had no intention to mislead and will reword.
Show 10 quoted lines
>> 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).
Thanks for that. As to the direction, I mentioned earlier adding a different flag, or perhaps marking the filename in some fashion to express that the prefix should be honored, would you prefer that? It would, as you said, be much safer in that there's no reason for it to be a breaking change. If there's a design you prefer that would result in having an opt-in way to get the prefix behavior I wouldn't mind implementing it.
Show 7 quoted lines
> 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 [*].
That's a very good point, and thank you for digging into it.
Show 9 quoted lines
>> 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.
My apologies, this is my unfamiliarity with GitGitGadget, I'll put information like this in the PR description next time, which I think will do that.