Am 11.10.22 um 14:45 schrieb Ævar Arnfjörð Bjarmason:
>
> On Tue, Oct 11 2022, René Scharfe wrote:
>
>> 94bc671a1f (Add directory pattern matching to attributes, 2012-12-08)
>> moved the code for adding the trailing slash to names of directories and
>> submodules up. This left both branches of the if statement starting
>> with the same conditional fprintf call. Deduplicate it.
>>
>> Signed-off-by: René Scharfe <l.s.r@xxxxxx>
>> ---
>> archive.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/archive.c b/archive.c
>> index 61a79e4a22..cc1087262f 100644
>> --- a/archive.c
>> +++ b/archive.c
>> @@ -166,18 +166,16 @@ static int write_archive_entry(const struct object_id *oid, const char *base,
>> args->convert = check_attr_export_subst(check);
>> }
>>
>> + if (args->verbose)
>> + fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
>> +
>> if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
>> - if (args->verbose)
>> - fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
>> err = write_entry(args, oid, path.buf, path.len, mode, NULL, 0);
>> if (err)
>> return err;
>> return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
>> }
>>
>> - if (args->verbose)
>> - fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
>> -
>> /* Stream it? */
>> if (S_ISREG(mode) && !args->convert &&
>> oid_object_info(args->repo, oid, &size) == OBJ_BLOB &&
>
> This looks good, but when trying to validate it with our tests (I added
> a BUG(...)) it seems we have no tests. I tried this on top of master:
>
> diff --git a/archive.c b/archive.c
> index 61a79e4a227..ed49f6d9106 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -166,18 +166,18 @@ static int write_archive_entry(const struct object_id *oid, const char *base,
> args->convert = check_attr_export_subst(check);
> }
>
> + if (args->verbose) {
> + fputs(path.buf, stderr);
> + fputc('\n', stderr);
> + }
> +
> if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
> - if (args->verbose)
> - fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
> err = write_entry(args, oid, path.buf, path.len, mode, NULL, 0);
> if (err)
> return err;
> return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
> }
>
> - if (args->verbose)
> - fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
> -
> /* Stream it? */
> if (S_ISREG(mode) && !args->convert &&
> oid_object_info(args->repo, oid, &size) == OBJ_BLOB &&
> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
> index fc499cdff01..3e61ba2f3ca 100755
> --- a/t/t5003-archive-zip.sh
> +++ b/t/t5003-archive-zip.sh
> @@ -153,9 +153,18 @@ test_expect_success \
> 'remove ignored file' \
> 'rm a/ignored'
>
> -test_expect_success \
> - 'git archive --format=zip' \
> - 'git archive --format=zip HEAD >d.zip'
> +test_expect_success 'git archive --format=zip' '
> + git archive --format=zip HEAD >d.zip 2>err &&
> + test_must_be_empty err &&
> +
> + git ls-tree -t -r HEAD --format="%(path)" >expect.err.raw &&
> + grep -v ignored <expect.err.raw >expect.err &&
> + test_when_finished "rm -f d2.zip" &&
> + git archive --format=zip --verbose HEAD >d2.zip 2>actual.err.raw &&
> + sed -n -e "s,/\$,," -e p <actual.err.raw >actual.err &&
> + test_cmp expect.err actual.err &&
> + test_cmp_bin d.zip d2.zip
> +'
I'd expect the tar format to be better suited for such a test because
its lack of compression makes it cheaper. And I wouldn't want trailing
slashes to be removed from the output -- tar(1) prints them as well.
Comparing to the output of "tar t x.tar" would be nice and easy, but
won't work with old versions and long filenames. Find my minimal
attempt below.
>
> check_zip d
>
>
> And it'll pass the test with/without the C change.
>
> I'm not sure if it's correct, i.e. are there cases where we really need
> that (int)path.len, it semes that the case in write_archive_entries()
> really does need it, but adding a BUG() there also reaveals that the
> --verbose version (but not non-verbose) is test-less.
>
Not sure what you mean with "need". strbufs are NUL-terminated and I
think filenames that contain NUL won't work, neither with archive nor
the rest of Git. So using %s or fputs(3) in write_archive_entry()
should be fine. But you can't prove that with tests, only disprove it.
I'd tend to use fwrite(3) instead if fprintf(3) would have to be
replaced, but I don't see any performance differences and the more
compact and familiar fprintf(3) seems good enough.
---
t/t5000-tar-tree.sh | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index eaa0b22ece..91593a5de3 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -169,11 +169,22 @@ test_expect_success 'remove ignored file' '
'
test_expect_success 'git archive' '
- git archive HEAD >b.tar
+ git archive HEAD >b.tar 2>err &&
+ test_must_be_empty err
'
check_tar b
+test_expect_success 'git archive --verbose' '
+ git archive --verbose HEAD >verbose.tar 2>err &&
+ test_cmp_bin b.tar verbose.tar &&
+ find a -type d | sed s-\$-/- >verbose.lst &&
+ find a \! -type d >>verbose.lst &&
+ sort <verbose.lst >expect &&
+ sort <err >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'git archive --prefix=prefix/' '
git archive --prefix=prefix/ HEAD >with_prefix.tar
'
--
2.38.0
