| Message ID | pull.1719.git.git.1715721327429.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | archive: make --add-virtual-file honor --prefix | expand |
"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.
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.
diff --git a/archive.c b/archive.c index 5287fcdd8e0..90086a1708f 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); + strbuf_addstr(&path_in_archive, basename(path)); 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); @@ -381,7 +380,7 @@ int write_archive_entries(struct archiver_args *args, content.buf, content.len); } else { 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..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 && 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) ) '