Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

archive: make --add-virtual-file honor --prefix #1719

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trws
Copy link

@trws trws commented May 13, 2024

cc: René Scharfe l.s.r@web.de

Copy link

Welcome to GitGitGadget

Hi @trws, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that either:

  • Your Pull Request has a good description, if it consists of multiple commits, as it will be used as cover letter.
  • Your Pull Request description is empty, if it consists of a single commit, as the commit message should be descriptive enough by itself.

You can CC potential reviewers by adding a footer to the PR description with the following syntax:

CC: Revi Ewer <revi.ewer@example.com>, Ill Takalook <ill.takalook@example.net>

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

@dscho
Copy link
Member

dscho commented May 14, 2024

/allow

Copy link

User trws is now allowed to use GitGitGadget.

WARNING: trws has no public email address set on GitHub;
GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use.

@trws
Copy link
Author

trws commented May 14, 2024

/submit

Copy link

Submitted as pull.1719.git.git.1715721327429.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1719/trws/honor-prefix-v1

To fetch this version to local tag pr-git-1719/trws/honor-prefix-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1719/trws/honor-prefix-v1

Copy link

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

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

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

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

Copy link

On the Git mailing list, Tom Scogland wrote (reply to this):

On 15 May 2024, at 8:23, Junio C Hamano wrote:

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

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

Certainly.

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

The documentation for archive describes the `--add-virtual-file` option
thusly:

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

The `--add-file` documentation is similar:

  The path of the file in the archive is built by concatenating the
  value of the last --prefix option (if any) before this --add-file and
  the basename of <file>.

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.  Also note that `--add-virtual-file`
explicitly states it will use the full path given, while `--add-file`
uses the basename of the path it is given.

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

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

Signed-off-by: Tom Scogland <scogland1@llnl.gov>
@trws
Copy link
Author

trws commented May 17, 2024

/submit

Copy link

Submitted as pull.1719.v2.git.git.1715967267420.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1719/trws/honor-prefix-v2

To fetch this version to local tag pr-git-1719/trws/honor-prefix-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1719/trws/honor-prefix-v2

Copy link

This branch is now known as ts/archive-prefix-with-add-virtual-file.

Copy link

This patch series was integrated into seen via eb73ba6.

Copy link

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> Also note that `--add-virtual-file`
> explicitly states it will use the full path given, while `--add-file`
> uses the basename of the path it is given.

Yes, this is a very good thing to mention.  It is probably the
reason why the implementation decided not to add prefix to the "full
path" that already can have the leading directories.

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

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 [*].

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

> Signed-off-by: Tom Scogland <scogland1@llnl.gov>
> ---

>  archive.c              | 10 +++++-----
>  t/t5003-archive-zip.sh | 14 ++++++++++++--
>  2 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index 5287fcdd8e0..64777a9870d 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -365,12 +365,11 @@ int write_archive_entries(struct archiver_args *args,
>  
>  		put_be64(fake_oid.hash, i + 1);
>  
> +		strbuf_reset(&path_in_archive);
> +		if (info->base)
> +			strbuf_addstr(&path_in_archive, info->base);
>  		if (!info->content) {
> -			strbuf_reset(&path_in_archive);
> -			if (info->base)
> -				strbuf_addstr(&path_in_archive, info->base);
>  			strbuf_addstr(&path_in_archive, basename(path));
> -
>  			strbuf_reset(&content);
>  			if (strbuf_read_file(&content, path, info->stat.st_size) < 0)
>  				err = error_errno(_("cannot read '%s'"), path);
> @@ -380,8 +379,9 @@ int write_archive_entries(struct archiver_args *args,
>  						  canon_mode(info->stat.st_mode),
>  						  content.buf, content.len);
>  		} else {
> +			strbuf_addstr(&path_in_archive, path);
>  			err = write_entry(args, &fake_oid,
> -					  path, strlen(path),
> +					  path_in_archive.buf, path_in_archive.len,
>  					  canon_mode(info->stat.st_mode),
>  					  info->content, info->stat.st_size);
>  		}
> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
> index 961c6aac256..0cf3aef8ace 100755
> --- a/t/t5003-archive-zip.sh
> +++ b/t/t5003-archive-zip.sh
> @@ -218,14 +218,24 @@ 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 \
> +		--add-virtual-file=with/dir/noprefix:withdirnopre \
> +		--prefix=subdir/ --add-virtual-file=with/dirprefix:withdirprefix \
> +		--prefix=subdir2/ --add-virtual-file=withoutdir:withoutdir \
> +		--prefix= $EMPTY_TREE &&
>  	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 with/dir/noprefix &&
> +		test withdirnopre = $(cat with/dir/noprefix) &&
> +		test_path_is_file subdir/with/dirprefix &&
> +		test withdirprefix = $(cat subdir/with/dirprefix) &&
> +		test_path_is_file subdir2/withoutdir &&
> +		test withoutdir = $(cat subdir2/withoutdir)

OK.  With different payload at different paths, it is easier than
the previous round to see where things are expected to go in the
result.


[Footnote]

 * I got curious and did this part for you.  After calling
   add_directory_to_archiver() that uses "--prefix" to move the
   target directory around in the output hierarchy, the code clears
   with "--prefix="---even a future change to diagnose.c adds more
   uses to --add-virtual-file= after it happens, it will not go to
   deep in the directory hierarchy where the last use of "--prefix"
   happened to be pointing at.

Copy link

There was a status update in the "New Topics" section about the branch ts/archive-prefix-with-add-virtual-file on the Git mailing list:

source: <pull.1719.v2.git.git.1715967267420.gitgitgadget@gmail.com>

Copy link

On the Git mailing list, Tom Scogland wrote (reply to this):

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.

Copy link

On the Git mailing list, René Scharfe wrote (reply to this):

Am 17.05.24 um 19:34 schrieb Tom Scogland via GitGitGadget:
> From: Tom Scogland <scogland1@llnl.gov>
>
> The documentation for archive describes the `--add-virtual-file` option
> thusly:
>
>   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>.

The documentation does not actually misspell "option" as "moption".

> The `--add-file` documentation is similar:
>
>   The path of the file in the archive is built by concatenating the
>   value of the last --prefix option (if any) before this --add-file and
>   the basename of <file>.
>
> 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.  Also note that `--add-virtual-file`
> explicitly states it will use the full path given, while `--add-file`
> uses the basename of the path it is given.
>
> Modify archive.c to include the prefix in the path used by
> `--add-virtual-file`

Aligning code and docs is a good idea.  Have you considered keeping the
code as is and changing the documentation instead, though?

The two options are related in that they both add untracked files, but
they necessarily have different arguments:

   --add-file=<file>
   --add-virtual-file=<path>:<content>

You can already specify any path you want with --add-virtual-file.
What's the advantage of honoring --prefix as well?

René

Copy link

User René Scharfe <l.s.r@web.de> has been added to the cc: list.

Copy link

On the Git mailing list, Tom Scogland wrote (reply to this):

On 19 May 2024, at 6:25, René Scharfe wrote:

>
> Aligning code and docs is a good idea.  Have you considered keeping the
> code as is and changing the documentation instead, though?
>
> The two options are related in that they both add untracked files, but
> they necessarily have different arguments:
>
>    --add-file=<file>
>    --add-virtual-file=<path>:<content>
>
> You can already specify any path you want with --add-virtual-file.
> What's the advantage of honoring --prefix as well?

I came into this after trying to translate an --add-file to an --add-virtual-file and being surprised that the prefix wasn't applied. anything else you add, in the repo or not, it gets the prefix. I can go back and explicitly add the prefix, but it makes the options less naturally composable in my opinion.  The specific case was packaging some software that uses git metadata to set version and some other things when in a repo, and files in a tarball. The tarball's prefix is set in one part of the packaging code, files and exclusions in another, and passing that through was something we didn't have to think about using add-file, but do with add-virtual-file.

That said, it sounds like both you and Junio prefer updating the docs rather than the code, which makes me think I'm in the minority in that opinion.  If that's the case, I can certainly update the docs, and I imagine we can backport that easily wherever it makes sense.  I would really like to have the option to have the prefix apply though, either adding a new flag or an option to the existing one that would be invalid given current syntax or similar to provide the option.

Copy link

On the Git mailing list, René Scharfe wrote (reply to this):

Am 20.05.24 um 18:10 schrieb Tom Scogland:
>
> On 19 May 2024, at 6:25, René Scharfe wrote:
>
>> You can already specify any path you want with --add-virtual-file.
>> What's the advantage of honoring --prefix as well?
>
> I came into this after trying to translate an --add-file to an
> --add-virtual-file and being surprised that the prefix wasn't
> applied.

Understandable, the documentation promised otherwise and the options
have very similar names.

> anything else you add, in the repo or not, it gets the prefix.> I can go back and explicitly add the prefix, but it makes the
> options less naturally composable in my opinion.

True, applying the prefix to all items would be simpler overall.

Speaking of simpler: --add-virtual-file could have been implemented to
only take a single argument -- the content -- and rely on --prefix to
provide the full path.  That's more consistent with other options, as
most of them only take single-valued arguments (or none). :]

> That said, it sounds like both you and Junio prefer updating the docs
> rather than the code, which makes me think I'm in the minority in
> that opinion.

I'm not sure I have an opinion on that topic, yet.  Fixing the
documentation is certainly easier.  Adding the prefix to the path of
virtual files as well is a breaking change.  I feel that the easier
route should at least be mentioned in the commit message and why it
was not taken.

> If that's the case, I can certainly update the docs, and I imagine we
> can backport that easily wherever it makes sense.  I would really
> like to have the option to have the prefix apply though, either
> adding a new flag or an option to the existing one that would be
> invalid given current syntax or similar to provide the option.
You mean like replacing a leading colon in the path with the prefix?

René

Copy link

On the Git mailing list, Junio C Hamano wrote (reply to this):

Tom Scogland <scogland1@llnl.gov> writes:

> That said, it sounds like both you and Junio prefer updating the
> docs rather than the code, which makes me think I'm in the
> minority in that opinion.  If that's the case, I can certainly
> update the docs, and I imagine we can backport that easily
> wherever it makes sense.  I would really like to have the option
> to have the prefix apply though, either adding a new flag or an
> option to the existing one that would be invalid given current
> syntax or similar to provide the option.

[jc: wrapped overly long lines]

Wouldn't

    #!/bin/sh
    prefix=foo/bar
    git archive --prefix="$prefix" --add-file=x --add-file=y \
	--add-virtual-file="$prefix/path:contents"

be an option enough?  You only have to define $prefix once.

Copy link

This patch series was integrated into seen via 41626cd.

Copy link

There was a status update in the "Cooking" section about the branch ts/archive-prefix-with-add-virtual-file on the Git mailing list:

source: <pull.1719.v2.git.git.1715967267420.gitgitgadget@gmail.com>

Copy link

This patch series was integrated into seen via 9cabbaa.

Copy link

This patch series was integrated into seen via 31f91fb.

Copy link

This patch series was integrated into seen via 4b4e694.

Copy link

This patch series was integrated into seen via d77a9d2.

Copy link

There was a status update in the "Cooking" section about the branch ts/archive-prefix-with-add-virtual-file on the Git mailing list:

The "--add-virtual-file" option of "git archive", added primarily
to help "git diagnose", has always ignored the "--prefix", but
been documented to honor it.

Iffy if updating the implementation is the best approach.
cf. <bc3711a7-37d5-46bc-979e-83bd0b2cf900@web.de>
source: <pull.1719.v2.git.git.1715967267420.gitgitgadget@gmail.com>

Copy link

This patch series was integrated into seen via 747ef69.

Copy link

This patch series was integrated into seen via 47ca324.

Copy link

There was a status update in the "Cooking" section about the branch ts/archive-prefix-with-add-virtual-file on the Git mailing list:

The "--add-virtual-file" option of "git archive", added primarily
to help "git diagnose", has always ignored the "--prefix", but
been documented to honor it.

Iffy if updating the implementation is the best approach.
cf. <bc3711a7-37d5-46bc-979e-83bd0b2cf900@web.de>
source: <pull.1719.v2.git.git.1715967267420.gitgitgadget@gmail.com>

Copy link

This patch series was integrated into seen via 7f648bc.

Copy link

This patch series was integrated into seen via 1870259.

Copy link

There was a status update in the "Cooking" section about the branch ts/archive-prefix-with-add-virtual-file on the Git mailing list:

The "--add-virtual-file" option of "git archive", added primarily
to help "git diagnose", has always ignored the "--prefix", but
been documented to honor it.

Iffy if updating the implementation is the best approach.
cf. <bc3711a7-37d5-46bc-979e-83bd0b2cf900@web.de>
source: <pull.1719.v2.git.git.1715967267420.gitgitgadget@gmail.com>

Copy link

This patch series was integrated into seen via e9d718c.

Copy link

There was a status update in the "Cooking" section about the branch ts/archive-prefix-with-add-virtual-file on the Git mailing list:

The "--add-virtual-file" option of "git archive", added primarily
to help "git diagnose", has always ignored the "--prefix", but
been documented to honor it.

Iffy if updating the implementation is the best approach.
cf. <bc3711a7-37d5-46bc-979e-83bd0b2cf900@web.de>
source: <pull.1719.v2.git.git.1715967267420.gitgitgadget@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants