{"thread":{"id":"58599","subject":"[PATCH] archive: deduplicate verbose printing","startedAt":"2022-10-11T09:29:52Z","lastAt":"2022-10-13T18:41:30Z","messageCount":5,"participants":["René Scharfe","Ævar Arnfjörð Bjarmason","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"464596","messageId":"af5611aa-8662-7508-4f00-7fcf4e9cbcc6@web.de","threadId":"58599","inReplyTo":null,"subject":"[PATCH] archive: deduplicate verbose printing","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-11T09:29:38Z","receivedAt":"2022-10-11T09:29:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"94bc671a1f (Add directory pattern matching to attributes, 2012-12-08)\nmoved the code for adding the trailing slash to names of directories and\nsubmodules up.  This left both branches of the if statement starting\nwith the same conditional fprintf call.  Deduplicate it.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n archive.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 61a79e4a22..cc1087262f 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -166,18 +166,16 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n \t\targs->convert = check_attr_export_subst(check);\n \t}\n\n+\tif (args->verbose)\n+\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n+\n \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\n-\t\tif (args->verbose)\n-\t\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n \t\terr = write_entry(args, oid, path.buf, path.len, mode, NULL, 0);\n \t\tif (err)\n \t\t\treturn err;\n \t\treturn (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n \t}\n\n-\tif (args->verbose)\n-\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n-\n \t/* Stream it? */\n \tif (S_ISREG(mode) && !args->convert &&\n \t    oid_object_info(args->repo, oid, &size) == OBJ_BLOB &&\n--\n2.38.0\n"},{"id":"464603","messageId":"221011.86pmeyo7ch.gmgdl@evledraar.gmail.com","threadId":"58599","inReplyTo":"af5611aa-8662-7508-4f00-7fcf4e9cbcc6@web.de","subject":"Re: [PATCH] archive: deduplicate verbose printing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-11T12:45:11Z","receivedAt":"2022-10-11T12:47:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 11 2022, René Scharfe wrote:\n\n> 94bc671a1f (Add directory pattern matching to attributes, 2012-12-08)\n> moved the code for adding the trailing slash to names of directories and\n> submodules up.  This left both branches of the if statement starting\n> with the same conditional fprintf call.  Deduplicate it.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  archive.c | 8 +++-----\n>  1 file changed, 3 insertions(+), 5 deletions(-)\n>\n> diff --git a/archive.c b/archive.c\n> index 61a79e4a22..cc1087262f 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -166,18 +166,16 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n>  \t\targs->convert = check_attr_export_subst(check);\n>  \t}\n>\n> +\tif (args->verbose)\n> +\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n> +\n>  \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\n> -\t\tif (args->verbose)\n> -\t\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n>  \t\terr = write_entry(args, oid, path.buf, path.len, mode, NULL, 0);\n>  \t\tif (err)\n>  \t\t\treturn err;\n>  \t\treturn (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n>  \t}\n>\n> -\tif (args->verbose)\n> -\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n> -\n>  \t/* Stream it? */\n>  \tif (S_ISREG(mode) && !args->convert &&\n>  \t    oid_object_info(args->repo, oid, &size) == OBJ_BLOB &&\n\nThis looks good, but when trying to validate it with our tests (I added\na BUG(...)) it seems we have no tests. I tried this on top of master:\n\t\n\tdiff --git a/archive.c b/archive.c\n\tindex 61a79e4a227..ed49f6d9106 100644\n\t--- a/archive.c\n\t+++ b/archive.c\n\t@@ -166,18 +166,18 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n\t \t\targs->convert = check_attr_export_subst(check);\n\t \t}\n\t \n\t+\tif (args->verbose) {\n\t+\t\tfputs(path.buf, stderr);\n\t+\t\tfputc('\\n', stderr);\n\t+\t}\n\t+\n\t \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\n\t-\t\tif (args->verbose)\n\t-\t\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n\t \t\terr = write_entry(args, oid, path.buf, path.len, mode, NULL, 0);\n\t \t\tif (err)\n\t \t\t\treturn err;\n\t \t\treturn (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n\t \t}\n\t \n\t-\tif (args->verbose)\n\t-\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n\t-\n\t \t/* Stream it? */\n\t \tif (S_ISREG(mode) && !args->convert &&\n\t \t    oid_object_info(args->repo, oid, &size) == OBJ_BLOB &&\n\tdiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\n\tindex fc499cdff01..3e61ba2f3ca 100755\n\t--- a/t/t5003-archive-zip.sh\n\t+++ b/t/t5003-archive-zip.sh\n\t@@ -153,9 +153,18 @@ test_expect_success \\\n\t     'remove ignored file' \\\n\t     'rm a/ignored'\n\t \n\t-test_expect_success \\\n\t-    'git archive --format=zip' \\\n\t-    'git archive --format=zip HEAD >d.zip'\n\t+test_expect_success 'git archive --format=zip' '\n\t+\tgit archive --format=zip HEAD >d.zip 2>err &&\n\t+\ttest_must_be_empty err &&\n\t+\n\t+\tgit ls-tree -t -r HEAD --format=\"%(path)\" >expect.err.raw &&\n\t+\tgrep -v ignored <expect.err.raw >expect.err &&\n\t+\ttest_when_finished \"rm -f d2.zip\" &&\n\t+\tgit archive --format=zip --verbose HEAD >d2.zip 2>actual.err.raw &&\n\t+\tsed -n -e \"s,/\\$,,\" -e p <actual.err.raw >actual.err &&\n\t+\ttest_cmp expect.err actual.err &&\n\t+\ttest_cmp_bin d.zip d2.zip\n\t+'\n\t \n\t check_zip d\n\t \n\t\nAnd it'll pass the test with/without the C change.\n\nI'm not sure if it's correct, i.e. are there cases where we really need\nthat (int)path.len, it semes that the case in write_archive_entries()\nreally does need it, but adding a BUG() there also reaveals that the\n--verbose version (but not non-verbose) is test-less.\n\n"},{"id":"464816","messageId":"27ec1253-4d34-1de2-caee-12bcf1dac03e@web.de","threadId":"58599","inReplyTo":"221011.86pmeyo7ch.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] archive: deduplicate verbose printing","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-13T10:35:37Z","receivedAt":"2022-10-13T10:35:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.10.22 um 14:45 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, Oct 11 2022, René Scharfe wrote:\n>\n>> 94bc671a1f (Add directory pattern matching to attributes, 2012-12-08)\n>> moved the code for adding the trailing slash to names of directories and\n>> submodules up.  This left both branches of the if statement starting\n>> with the same conditional fprintf call.  Deduplicate it.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  archive.c | 8 +++-----\n>>  1 file changed, 3 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/archive.c b/archive.c\n>> index 61a79e4a22..cc1087262f 100644\n>> --- a/archive.c\n>> +++ b/archive.c\n>> @@ -166,18 +166,16 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n>>  \t\targs->convert = check_attr_export_subst(check);\n>>  \t}\n>>\n>> +\tif (args->verbose)\n>> +\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n>> +\n>>  \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\n>> -\t\tif (args->verbose)\n>> -\t\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n>>  \t\terr = write_entry(args, oid, path.buf, path.len, mode, NULL, 0);\n>>  \t\tif (err)\n>>  \t\t\treturn err;\n>>  \t\treturn (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n>>  \t}\n>>\n>> -\tif (args->verbose)\n>> -\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n>> -\n>>  \t/* Stream it? */\n>>  \tif (S_ISREG(mode) && !args->convert &&\n>>  \t    oid_object_info(args->repo, oid, &size) == OBJ_BLOB &&\n>\n> This looks good, but when trying to validate it with our tests (I added\n> a BUG(...)) it seems we have no tests. I tried this on top of master:\n>\n> \tdiff --git a/archive.c b/archive.c\n> \tindex 61a79e4a227..ed49f6d9106 100644\n> \t--- a/archive.c\n> \t+++ b/archive.c\n> \t@@ -166,18 +166,18 @@ static int write_archive_entry(const struct object_id *oid, const char *base,\n> \t \t\targs->convert = check_attr_export_subst(check);\n> \t \t}\n>\n> \t+\tif (args->verbose) {\n> \t+\t\tfputs(path.buf, stderr);\n> \t+\t\tfputc('\\n', stderr);\n> \t+\t}\n> \t+\n> \t \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\n> \t-\t\tif (args->verbose)\n> \t-\t\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n> \t \t\terr = write_entry(args, oid, path.buf, path.len, mode, NULL, 0);\n> \t \t\tif (err)\n> \t \t\t\treturn err;\n> \t \t\treturn (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n> \t \t}\n>\n> \t-\tif (args->verbose)\n> \t-\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n> \t-\n> \t \t/* Stream it? */\n> \t \tif (S_ISREG(mode) && !args->convert &&\n> \t \t    oid_object_info(args->repo, oid, &size) == OBJ_BLOB &&\n> \tdiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\n> \tindex fc499cdff01..3e61ba2f3ca 100755\n> \t--- a/t/t5003-archive-zip.sh\n> \t+++ b/t/t5003-archive-zip.sh\n> \t@@ -153,9 +153,18 @@ test_expect_success \\\n> \t     'remove ignored file' \\\n> \t     'rm a/ignored'\n>\n> \t-test_expect_success \\\n> \t-    'git archive --format=zip' \\\n> \t-    'git archive --format=zip HEAD >d.zip'\n> \t+test_expect_success 'git archive --format=zip' '\n> \t+\tgit archive --format=zip HEAD >d.zip 2>err &&\n> \t+\ttest_must_be_empty err &&\n> \t+\n> \t+\tgit ls-tree -t -r HEAD --format=\"%(path)\" >expect.err.raw &&\n> \t+\tgrep -v ignored <expect.err.raw >expect.err &&\n> \t+\ttest_when_finished \"rm -f d2.zip\" &&\n> \t+\tgit archive --format=zip --verbose HEAD >d2.zip 2>actual.err.raw &&\n> \t+\tsed -n -e \"s,/\\$,,\" -e p <actual.err.raw >actual.err &&\n> \t+\ttest_cmp expect.err actual.err &&\n> \t+\ttest_cmp_bin d.zip d2.zip\n> \t+'\n\nI'd expect the tar format to be better suited for such a test because\nits lack of compression makes it cheaper.  And I wouldn't want trailing\nslashes to be removed from the output -- tar(1) prints them as well.\nComparing to the output of \"tar t x.tar\" would be nice and easy, but\nwon't work with old versions and long filenames.  Find my minimal\nattempt below.\n\n>\n> \t check_zip d\n>\n>\n> And it'll pass the test with/without the C change.\n>\n> I'm not sure if it's correct, i.e. are there cases where we really need\n> that (int)path.len, it semes that the case in write_archive_entries()\n> really does need it, but adding a BUG() there also reaveals that the\n> --verbose version (but not non-verbose) is test-less.\n>\n\nNot sure what you mean with \"need\".  strbufs are NUL-terminated and I\nthink filenames that contain NUL won't work, neither with archive nor\nthe rest of Git.  So using %s or fputs(3) in write_archive_entry()\nshould be fine.  But you can't prove that with tests, only disprove it.\n\nI'd tend to use fwrite(3) instead if fprintf(3) would have to be\nreplaced, but I don't see any performance differences and the more\ncompact and familiar fprintf(3) seems good enough.\n\n\n---\n t/t5000-tar-tree.sh | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex eaa0b22ece..91593a5de3 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -169,11 +169,22 @@ test_expect_success 'remove ignored file' '\n '\n\n test_expect_success 'git archive' '\n-\tgit archive HEAD >b.tar\n+\tgit archive HEAD >b.tar 2>err &&\n+\ttest_must_be_empty err\n '\n\n check_tar b\n\n+test_expect_success 'git archive --verbose' '\n+\tgit archive --verbose HEAD >verbose.tar 2>err &&\n+\ttest_cmp_bin b.tar verbose.tar &&\n+\tfind a -type d | sed s-\\$-/- >verbose.lst &&\n+\tfind a \\! -type d >>verbose.lst &&\n+\tsort <verbose.lst >expect &&\n+\tsort <err >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git archive --prefix=prefix/' '\n \tgit archive --prefix=prefix/ HEAD >with_prefix.tar\n '\n--\n2.38.0\n\n"},{"id":"464871","messageId":"CAPig+cRuqq=n=CLkCSH=axLO92=F2JGwNnK51aBEeCoMSdJjqg@mail.gmail.com","threadId":"58599","inReplyTo":"27ec1253-4d34-1de2-caee-12bcf1dac03e@web.de","subject":"Re: [PATCH] archive: deduplicate verbose printing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-10-13T17:02:36Z","receivedAt":"2022-10-13T17:02:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 13, 2022 at 6:40 AM René Scharfe <l.s.r@web.de> wrote:\n> +test_expect_success 'git archive --verbose' '\n> +       git archive --verbose HEAD >verbose.tar 2>err &&\n> +       test_cmp_bin b.tar verbose.tar &&\n> +       find a -type d | sed s-\\$-/- >verbose.lst &&\n> +       find a \\! -type d >>verbose.lst &&\n\nAside: I was curious whether or not we care about older `find`\nimplementations which don't print anything at all if `-print` isn't\nspecified, but I see that the test suite already has a mixture of\n`find` invocations -- some with and some without `-print` -- so that\nanswers my question.\n"},{"id":"464879","messageId":"xmqqy1tjfu9b.fsf@gitster.g","threadId":"58599","inReplyTo":"CAPig+cRuqq=n=CLkCSH=axLO92=F2JGwNnK51aBEeCoMSdJjqg@mail.gmail.com","subject":"Re: [PATCH] archive: deduplicate verbose printing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-13T18:33:52Z","receivedAt":"2022-10-13T18:41:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Thu, Oct 13, 2022 at 6:40 AM René Scharfe <l.s.r@web.de> wrote:\n>> +test_expect_success 'git archive --verbose' '\n>> +       git archive --verbose HEAD >verbose.tar 2>err &&\n>> +       test_cmp_bin b.tar verbose.tar &&\n>> +       find a -type d | sed s-\\$-/- >verbose.lst &&\n>> +       find a \\! -type d >>verbose.lst &&\n>\n> Aside: I was curious whether or not we care about older `find`\n> implementations which don't print anything at all if `-print` isn't\n> specified, but I see that the test suite already has a mixture of\n> `find` invocations -- some with and some without `-print` -- so that\n> answers my question.\n\nIt indicates that everybody is POSIX enough these days ;-)\n\nI do think it is better to be more explicit to write \"-print\" when\nwe mean it, and I wouldn't mind a clean-up patch every once in a\nwhile when the area of the tree being affected are quiescent.\n\nThanks.\n"}]}