{"thread":{"id":"53300","subject":"[PATCH 0/2] minor fixups for gs/commit-graph-path-filter","startedAt":"2020-04-23T20:58:54Z","lastAt":"2020-04-24T20:00:28Z","messageCount":11,"participants":["Jeff King","Taylor Blau","Garima Singh","Danh Doan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"396034","messageId":"20200423205851.GA1633985@coredump.intra.peff.net","threadId":"53300","inReplyTo":null,"subject":"[PATCH 0/2] minor fixups for gs/commit-graph-path-filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-23T20:58:51Z","receivedAt":"2020-04-23T20:58:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These are just a few bits I noticed in the test-tool helper when the\ntopic hit next (my -Wunused-parameter patch complained that we never\nlooked at argc).\n\n [1/2]: test-bloom: fix some whitespace issues\n [2/2]: test-bloom: check that we have expected arguments\n\n t/helper/test-bloom.c | 28 ++++++++++++++++++++--------\n 1 file changed, 20 insertions(+), 8 deletions(-)\n\n-Peff\n"},{"id":"396035","messageId":"20200423205907.GA1634224@coredump.intra.peff.net","threadId":"53300","inReplyTo":"20200423205851.GA1633985@coredump.intra.peff.net","subject":"[PATCH 1/2] test-bloom: fix some whitespace issues","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-23T20:59:07Z","receivedAt":"2020-04-23T20:59:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Signed-off-by: Jeff King \n---\n t/helper/test-bloom.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c\nindex ce412664ba..f9c0ce2bae 100644\n--- a/t/helper/test-bloom.c\n+++ b/t/helper/test-bloom.c\n@@ -27,7 +27,7 @@ static void print_bloom_filter(struct bloom_filter *filter) {\n \t}\n \tprintf(\"Filter_Length:%d\\n\", (int)filter->len);\n \tprintf(\"Filter_Data:\");\n-\tfor (i = 0; i < filter->len; i++){\n+\tfor (i = 0; i < filter->len; i++) {\n \t\tprintf(\"%02x|\", filter->data[i]);\n \t}\n \tprintf(\"\\n\");\n@@ -50,13 +50,13 @@ int cmd__bloom(int argc, const char **argv)\n \t\tprintf(\"Murmur3 Hash with seed=0:0x%08x\\n\", hashed);\n \t}\n \n- if (!strcmp(argv[1], \"generate_filter\")) {\n+\tif (!strcmp(argv[1], \"generate_filter\")) {\n \t\tstruct bloom_filter filter;\n \t\tint i = 2;\n \t\tfilter.len = (settings.bits_per_entry + BITS_PER_WORD - 1) / BITS_PER_WORD;\n \t\tfilter.data = xcalloc(filter.len, sizeof(unsigned char));\n \n-\t\tif (!argv[2]){\n+\t\tif (!argv[2]) {\n \t\t\tdie(\"at least one input string expected\");\n \t\t}\n \n@@ -68,7 +68,7 @@ int cmd__bloom(int argc, const char **argv)\n \t\tprint_bloom_filter(&filter);\n \t}\n \n- if (!strcmp(argv[1], \"get_filter_for_commit\")) {\n+\tif (!strcmp(argv[1], \"get_filter_for_commit\")) {\n \t\tstruct object_id oid;\n \t\tconst char *end;\n \t\tif (parse_oid_hex(argv[2], &oid, &end))\n@@ -78,4 +78,4 @@ int cmd__bloom(int argc, const char **argv)\n \t}\n \n \treturn 0;\n-}\n\\ No newline at end of file\n+}\n-- \n2.26.2.827.g3c1233342b\n\n"},{"id":"396036","messageId":"20200423205914.GB1634224@coredump.intra.peff.net","threadId":"53300","inReplyTo":"20200423205851.GA1633985@coredump.intra.peff.net","subject":"[PATCH 2/2] test-bloom: check that we have expected arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-23T20:59:14Z","receivedAt":"2020-04-23T20:59:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If \"test-tool bloom\" is not fed a command, or if arguments are missing\nfor some commands, it will just segfault. Let's check argc and write a\nfriendlier usage message.\n\nSigned-off-by: Jeff King \n---\n t/helper/test-bloom.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c\nindex f9c0ce2bae..77eb27adac 100644\n--- a/t/helper/test-bloom.c\n+++ b/t/helper/test-bloom.c\n@@ -43,10 +43,21 @@ static void get_bloom_filter_for_commit(const struct object_id *commit_oid)\n \tprint_bloom_filter(filter);\n }\n \n+static const char *bloom_usage = \"\\n\"\n+\" test-tool bloom get_murmer3 \\n\"\n+\" test-tool bloom generate_filter [...]\\n\"\n+\" test-tool get_filter_for_commit \\n\";\n+\n int cmd__bloom(int argc, const char **argv)\n {\n+\tif (argc < 2)\n+\t\tusage(bloom_usage);\n+\n \tif (!strcmp(argv[1], \"get_murmur3\")) {\n-\t\tuint32_t hashed = murmur3_seeded(0, argv[2], strlen(argv[2]));\n+\t\tuint32_t hashed;\n+\t\tif (argc < 3)\n+\t\t\tusage(bloom_usage);\n+\t\thashed = murmur3_seeded(0, argv[2], strlen(argv[2]));\n \t\tprintf(\"Murmur3 Hash with seed=0:0x%08x\\n\", hashed);\n \t}\n \n@@ -56,9 +67,8 @@ int cmd__bloom(int argc, const char **argv)\n \t\tfilter.len = (settings.bits_per_entry + BITS_PER_WORD - 1) / BITS_PER_WORD;\n \t\tfilter.data = xcalloc(filter.len, sizeof(unsigned char));\n \n-\t\tif (!argv[2]) {\n-\t\t\tdie(\"at least one input string expected\");\n-\t\t}\n+\t\tif (argc - 1 < i)\n+\t\t\tusage(bloom_usage);\n \n \t\twhile (argv[i]) {\n \t\t\tadd_string_to_filter(argv[i], &filter);\n@@ -71,6 +81,8 @@ int cmd__bloom(int argc, const char **argv)\n \tif (!strcmp(argv[1], \"get_filter_for_commit\")) {\n \t\tstruct object_id oid;\n \t\tconst char *end;\n+\t\tif (argc < 3)\n+\t\t\tusage(bloom_usage);\n \t\tif (parse_oid_hex(argv[2], &oid, &end))\n \t\t\tdie(\"cannot parse oid '%s'\", argv[2]);\n \t\tinit_bloom_filters();\n-- \n2.26.2.827.g3c1233342b\n"},{"id":"396037","messageId":"20200423210155.GA46862@syl.local","threadId":"53300","inReplyTo":"20200423205907.GA1634224@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] test-bloom: fix some whitespace issues","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T21:01:55Z","receivedAt":"2020-04-23T21:02:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 23, 2020 at 04:59:07PM -0400, Jeff King wrote:\n> Signed-off-by: Jeff King \n> ---\n> t/helper/test-bloom.c | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c\n> index ce412664ba..f9c0ce2bae 100644\n> --- a/t/helper/test-bloom.c\n> +++ b/t/helper/test-bloom.c\n> @@ -27,7 +27,7 @@ static void print_bloom_filter(struct bloom_filter *filter) {\n> \t}\n> \tprintf(\"Filter_Length:%d\\n\", (int)filter->len);\n> \tprintf(\"Filter_Data:\");\n> -\tfor (i = 0; i < filter->len; i++){\n> +\tfor (i = 0; i < filter->len; i++) {\n\nThanks for fixing the spacing, but I wonder if these braces should be\nhere at all. Since the body is one line long, maybe this should just be:\n\n for (i = 0; i < filter->len; i++)\n printf(\"%02x|\", filter->data[i]);\n\n> \t\tprintf(\"%02x|\", filter->data[i]);\n> \t}\n> \tprintf(\"\\n\");\n> @@ -50,13 +50,13 @@ int cmd__bloom(int argc, const char **argv)\n> \t\tprintf(\"Murmur3 Hash with seed=0:0x%08x\\n\", hashed);\n> \t}\n>\n> - if (!strcmp(argv[1], \"generate_filter\")) {\n> +\tif (!strcmp(argv[1], \"generate_filter\")) {\n\nThis spot looks good, and ditto for fixing the indentation.\n\n> \t\tstruct bloom_filter filter;\n> \t\tint i = 2;\n> \t\tfilter.len = (settings.bits_per_entry + BITS_PER_WORD - 1) / BITS_PER_WORD;\n> \t\tfilter.data = xcalloc(filter.len, sizeof(unsigned char));\n>\n> -\t\tif (!argv[2]){\n> +\t\tif (!argv[2]) {\n\nSame note here about the brace.\n\n> \t\t\tdie(\"at least one input string expected\");\n> \t\t}\n>\n> @@ -68,7 +68,7 @@ int cmd__bloom(int argc, const char **argv)\n> \t\tprint_bloom_filter(&filter);\n> \t}\n>\n> - if (!strcmp(argv[1], \"get_filter_for_commit\")) {\n> +\tif (!strcmp(argv[1], \"get_filter_for_commit\")) {\n> \t\tstruct object_id oid;\n> \t\tconst char *end;\n> \t\tif (parse_oid_hex(argv[2], &oid, &end))\n> @@ -78,4 +78,4 @@ int cmd__bloom(int argc, const char **argv)\n> \t}\n>\n> \treturn 0;\n> -}\n> \\ No newline at end of file\n> +}\n> --\n> 2.26.2.827.g3c1233342b\n>\n\nThanks,\nTaylor\n"},{"id":"396038","messageId":"20200423210248.GB46862@syl.local","threadId":"53300","inReplyTo":"20200423205914.GB1634224@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-bloom: check that we have expected arguments","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-23T21:02:48Z","receivedAt":"2020-04-23T21:02:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 23, 2020 at 04:59:14PM -0400, Jeff King wrote:\n> If \"test-tool bloom\" is not fed a command, or if arguments are missing\n> for some commands, it will just segfault. Let's check argc and write a\n> friendlier usage message.\n>\n> Signed-off-by: Jeff King \n> ---\n\nLooks all good.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"396040","messageId":"20200423210409.GB1635761@coredump.intra.peff.net","threadId":"53300","inReplyTo":"20200423210155.GA46862@syl.local","subject":"Re: [PATCH 1/2] test-bloom: fix some whitespace issues","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-23T21:04:09Z","receivedAt":"2020-04-23T21:04:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 23, 2020 at 03:01:55PM -0600, Taylor Blau wrote:\n\n> On Thu, Apr 23, 2020 at 04:59:07PM -0400, Jeff King wrote:\n> > Signed-off-by: Jeff King \n> > ---\n> > t/helper/test-bloom.c | 10 +++++-----\n> > 1 file changed, 5 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c\n> > index ce412664ba..f9c0ce2bae 100644\n> > --- a/t/helper/test-bloom.c\n> > +++ b/t/helper/test-bloom.c\n> > @@ -27,7 +27,7 @@ static void print_bloom_filter(struct bloom_filter *filter) {\n> > \t}\n> > \tprintf(\"Filter_Length:%d\\n\", (int)filter->len);\n> > \tprintf(\"Filter_Data:\");\n> > -\tfor (i = 0; i < filter->len; i++){\n> > +\tfor (i = 0; i < filter->len; i++) {\n> \n> Thanks for fixing the spacing, but I wonder if these braces should be\n> here at all. Since the body is one line long, maybe this should just be:\n> \n> for (i = 0; i < filter->len; i++)\n> printf(\"%02x|\", filter->data[i]);\n\nI have to admit that I don't care either way, and I think we spend too\nmuch time quibbling about braces or not-braces. It was really the bad\nindentation that I cared about most.\n\n-Peff\n"},{"id":"396058","messageId":"dde036bb-303c-a9d1-6bf2-880428cfb7c0@gmail.com","threadId":"53300","inReplyTo":"20200423205851.GA1633985@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter","fromName":"Garima Singh","fromEmail":"garimasigit@gmail.com","sentAt":"2020-04-23T22:14:36Z","receivedAt":"2020-04-23T22:14:40Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"\nOn 4/23/2020 4:58 PM, Jeff King wrote:\n> These are just a few bits I noticed in the test-tool helper when the\n> topic hit next (my -Wunused-parameter patch complained that we never\n> looked at argc).\n> \n> [1/2]: test-bloom: fix some whitespace issues\n> [2/2]: test-bloom: check that we have expected arguments\n> \n> t/helper/test-bloom.c | 28 ++++++++++++++++++++--------\n> 1 file changed, 20 insertions(+), 8 deletions(-)\n> \n> -Peff\n> \n\nThank you for doing this! \nBoth patches look good to me. \nI also don't care about the brace/no-brace thing that \nTaylor brought up for 1/2. \n\nCheers! \nGarima Singh\n"},{"id":"396088","messageId":"20200424010047.GD1949@danh.dev","threadId":"53300","inReplyTo":"20200423205851.GA1633985@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-04-24T01:00:47Z","receivedAt":"2020-04-24T01:00:53Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-04-23 16:58:51-0400, Jeff King wrote:\n> These are just a few bits I noticed in the test-tool helper when the\n> topic hit next (my -Wunused-parameter patch complained that we never\n> looked at argc).\n\nI think I'll add this one to those few bits.\nOld version of this change was sent here:\n<20200423133937.GA1984@danh.dev>\n\nBut that version doesn't have the fixup for sh script.\n\nGarima Singh: Could you please change your editor to add final new line?\n\nI've take another look into bloom.h.\n\nI think we should drop BITS_PER_WORD definition and use CHAR_BIT\ninstead. It's a standard definition.\n\nTo me, a WORD is an `int`, at least I was told that when I was still\nin university and study about computer science.\n\n-----------------8<---------------------\nFrom: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?=\n \nDate: Thu, 23 Apr 2020 20:24:50 +0700\nSubject: [PATCH] bloom: fix `make sparse` warning\nMIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\n\n* We need a `final_new_line` to make our source code as text file, per\n POSIX and C specification.\n* `bloom_filters` should be limited to interal linkage only\n\n\nSigned-off-by: Đoàn Trần Công Danh \n---\nFeel free to fix up to current series\n bloom.c | 4 ++--\n bloom.h | 2 +-\n t/helper/test-bloom.c | 2 +-\n t/t0095-bloom.sh | 2 +-\n t/t4216-log-bloom.sh | 2 +-\n 5 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/bloom.c b/bloom.c\nindex dd9bab9bbd..ee025e0c61 100644\n--- a/bloom.c\n+++ b/bloom.c\n@@ -9,7 +9,7 @@\n \n define_commit_slab(bloom_filter_slab, struct bloom_filter);\n \n-struct bloom_filter_slab bloom_filters;\n+static struct bloom_filter_slab bloom_filters;\n \n struct pathmap_hash_entry {\n struct hashmap_entry entry;\n@@ -273,4 +273,4 @@ int bloom_filter_contains(const struct bloom_filter *filter,\n \t}\n \n \treturn 1;\n-}\n\\ No newline at end of file\n+}\ndiff --git a/bloom.h b/bloom.h\nindex b935186425..e0e59e0754 100644\n--- a/bloom.h\n+++ b/bloom.h\n@@ -87,4 +87,4 @@ int bloom_filter_contains(const struct bloom_filter *filter,\n \t\t\t const struct bloom_key *key,\n \t\t\t const struct bloom_filter_settings *settings);\n \n-#endif\n\\ No newline at end of file\n+#endif\ndiff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c\nindex 77eb27adac..456f5ea7f9 100644\n--- a/t/helper/test-bloom.c\n+++ b/t/helper/test-bloom.c\n@@ -3,7 +3,7 @@\n #include \"test-tool.h\"\n #include \"commit.h\"\n \n-struct bloom_filter_settings settings = DEFAULT_BLOOM_FILTER_SETTINGS;\n+static struct bloom_filter_settings settings = DEFAULT_BLOOM_FILTER_SETTINGS;\n \n static void add_string_to_filter(const char *data, struct bloom_filter *filter) {\n \t\tstruct bloom_key key;\ndiff --git a/t/t0095-bloom.sh b/t/t0095-bloom.sh\nindex 8f9eef116d..809ec7b0b8 100755\n--- a/t/t0095-bloom.sh\n+++ b/t/t0095-bloom.sh\n@@ -114,4 +114,4 @@ test_expect_success EXPENSIVE 'get bloom filter for commit with 513 changes' '\n \ttest_cmp expect actual\n '\n \n-test_done\n\\ No newline at end of file\n+test_done\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex c7011f33e2..21b68dd6c8 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -152,4 +152,4 @@ test_expect_success 'Use Bloom filters if they exist in the latest but not all c\n \ttest_bloom_filters_used_when_some_filters_are_missing \"-- A/B\"\n '\n \n-test_done\n\\ No newline at end of file\n+test_done\n-- \n2.26.2.384.g435bf60bd5\n\n\n-- \nDanh\n"},{"id":"396107","messageId":"20200424052508.GC1648190@coredump.intra.peff.net","threadId":"53300","inReplyTo":"20200424010047.GD1949@danh.dev","subject":"Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-24T05:25:08Z","receivedAt":"2020-04-24T05:25:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 24, 2020 at 08:00:47AM +0700, Danh Doan wrote:\n\n> On 2020-04-23 16:58:51-0400, Jeff King wrote:\n> > These are just a few bits I noticed in the test-tool helper when the\n> > topic hit next (my -Wunused-parameter patch complained that we never\n> > looked at argc).\n> \n> I think I'll add this one to those few bits.\n\nYeah, they all look sensible (I should have looked for more \"No newline\"\ncases.\n\n> I've take another look into bloom.h.\n> \n> I think we should drop BITS_PER_WORD definition and use CHAR_BIT\n> instead. It's a standard definition.\n> \n> To me, a WORD is an `int`, at least I was told that when I was still\n> in university and study about computer science.\n\nYes, I agree it would be more clear as just CHAR_BIT if we are using\nsingle-char words. But I suspect the code could be looking at the bit\npatterns using larger word sizes (e.g., all of the ewah code uses 64-bit\nwords). That might be worth exploring.\n\n-Peff\n"},{"id":"396151","messageId":"20200424165805.GC58621@syl.local","threadId":"53300","inReplyTo":"dde036bb-303c-a9d1-6bf2-880428cfb7c0@gmail.com","subject":"Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-24T16:58:05Z","receivedAt":"2020-04-24T16:58:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 23, 2020 at 06:14:36PM -0400, Garima Singh wrote:\n>\n> On 4/23/2020 4:58 PM, Jeff King wrote:\n> > These are just a few bits I noticed in the test-tool helper when the\n> > topic hit next (my -Wunused-parameter patch complained that we never\n> > looked at argc).\n> >\n> > [1/2]: test-bloom: fix some whitespace issues\n> > [2/2]: test-bloom: check that we have expected arguments\n> >\n> > t/helper/test-bloom.c | 28 ++++++++++++++++++++--------\n> > 1 file changed, 20 insertions(+), 8 deletions(-)\n> >\n> > -Peff\n> >\n>\n> Thank you for doing this!\n> Both patches look good to me.\n> I also don't care about the brace/no-brace thing that\n> Taylor brought up for 1/2.\n\nTo be clear, I don't care about them either ;). Maybe it's time that we\nrelax that rule (if it seems that a good number of us don't mind it\neither way)..?\n\n> Cheers!\n> Garima Singh\n\nThanks,\nTaylor\n"},{"id":"396171","messageId":"xmqqr1wcd57s.fsf@gitster.c.googlers.com","threadId":"53300","inReplyTo":"20200424165805.GC58621@syl.local","subject":"Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T20:00:23Z","receivedAt":"2020-04-24T20:00:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> To be clear, I don't care about them either ;). Maybe it's time that we\n> relax that rule (if it seems that a good number of us don't mind it\n> either way)..?\n\nI'd prefer our reviewers (even more preferably the authors) to be\ncareful about new code, but it probably is not worth reviewing\npatches that only fix them and do nothing else.\n\n"}]}