Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: Tom Scogland <scogland1@llnl.gov>
To: Junio C Hamano <gitster@pobox.com>
Cc: Tom Scogland via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2] archive: make --add-virtual-file honor --prefix
Date: Fri, 17 May 2024 17:26:08 -0700	[thread overview]
Message-ID: <176A8590-61BC-45F2-9B37-FFD8B07ABCA9@llnl.gov> (raw)
In-Reply-To: <xmqqv83cdneo.fsf@gitster.g>



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

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

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

  reply	other threads:[~2024-05-18  0:26 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-14 21:15 [PATCH] archive: make --add-virtual-file honor --prefix Tom Scogland via GitGitGadget
2024-05-15 15:23 ` Junio C Hamano
2024-05-15 16:27   ` Tom Scogland
2024-05-17 17:34 ` [PATCH v2] " Tom Scogland via GitGitGadget
2024-05-17 23:33   ` Junio C Hamano
2024-05-18  0:26     ` Tom Scogland [this message]
2024-05-19 13:25   ` René Scharfe
2024-05-20 16:10     ` Tom Scogland
2024-05-20 17:07       ` René Scharfe
2024-06-14 18:07         ` Junio C Hamano
2024-06-14 18:40           ` [PATCH] archive: document that --add-virtual-file takes full path Junio C Hamano
2024-05-20 17:55       ` [PATCH v2] archive: make --add-virtual-file honor --prefix Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=176A8590-61BC-45F2-9B37-FFD8B07ABCA9@llnl.gov \
    --to=scogland1@llnl.gov \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).