diff mbox series

archive: deduplicate verbose printing

Message ID af5611aa-8662-7508-4f00-7fcf4e9cbcc6@web.de (mailing list archive)
State New, archived
Headers show
Series archive: deduplicate verbose printing | expand

Commit Message

René Scharfe Oct. 11, 2022, 9:29 a.m. UTC 
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(-)

--
2.38.0

Comments

Ævar Arnfjörð Bjarmason Oct. 11, 2022, 12:45 p.m. UTC | #1 
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
	+'
	 
	 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.
René Scharfe Oct. 13, 2022, 10:35 a.m. UTC | #2 
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
Eric Sunshine Oct. 13, 2022, 5:02 p.m. UTC | #3 
On Thu, Oct 13, 2022 at 6:40 AM René Scharfe <l.s.r@web.de> wrote:
> +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 &&

Aside: I was curious whether or not we care about older `find`
implementations which don't print anything at all if `-print` isn't
specified, but I see that the test suite already has a mixture of
`find` invocations -- some with and some without `-print` -- so that
answers my question.
Junio C Hamano Oct. 13, 2022, 6:33 p.m. UTC | #4 
Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Oct 13, 2022 at 6:40 AM René Scharfe <l.s.r@web.de> wrote:
>> +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 &&
>
> Aside: I was curious whether or not we care about older `find`
> implementations which don't print anything at all if `-print` isn't
> specified, but I see that the test suite already has a mixture of
> `find` invocations -- some with and some without `-print` -- so that
> answers my question.

It indicates that everybody is POSIX enough these days ;-)

I do think it is better to be more explicit to write "-print" when
we mean it, and I wouldn't mind a clean-up patch every once in a
while when the area of the tree being affected are quiescent.

Thanks.
diff mbox series

Patch

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