Re: [PATCH] archive: deduplicate verbose printing

["René Scharfe" <l.s.r@web.de>]

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