Re: [PATCH] archive: deduplicate verbose printing

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 11, 2022, 12:45 UTC
Message-ID: <221011.86pmeyo7ch.gmgdl@evledraar.gmail.com>
In-Reply-To: <af5611aa-8662-7508-4f00-7fcf4e9cbcc6@web.de>

On Tue, Oct 11 2022, René Scharfe wrote:

Show 36 quoted lines

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