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