[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v2] archive: make --add-virtual-file honor --prefix
From:       Tom Scogland <scogland1 () llnl ! gov>
Date:       2024-05-18 0:26:08
Message-ID: 176A8590-61BC-45F2-9B37-FFD8B07ABCA9 () llnl ! gov
[Download RAW message or body]



On 17 May 2024, at 16:33, Junio C Hamano wrote:

> "Tom Scogland via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> Notably both explicitly state that they honor the last `--prefix` opti=
on
>> before the `--add` option in question.  The implementation of
>> `--add-file` seems to have always honored prefix, but the implementati=
on
>> 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.

>> 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 diffe=
rent flag, or perhaps marking the filename in some fashion to express tha=
t the prefix should be honored, would you prefer that? It would, as you s=
aid, be much safer in that there's no reason for it to be a breaking chan=
ge. 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.

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

>> 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-fi=
le
>
> 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 inform=
ation like this in the PR description next time, which I think will do th=
at.

[prev in list] [next in list] [prev in thread] [next in thread]