[PATCH 0/2] minor fixups for gs/commit-graph-path-filter

[PATCH 0/2] minor fixups for gs/commit-graph-path-filter

[Jeff King]

These are just a few bits I noticed in the test-tool helper when the
topic hit next (my -Wunused-parameter patch complained that we never
looked at argc).

  [1/2]: test-bloom: fix some whitespace issues
  [2/2]: test-bloom: check that we have expected arguments

 t/helper/test-bloom.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

-Peff

[PATCH 1/2] test-bloom: fix some whitespace issues

[Jeff King]

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-bloom.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c
index ce412664ba..f9c0ce2bae 100644
--- a/t/helper/test-bloom.c
+++ b/t/helper/test-bloom.c
@@ -27,7 +27,7 @@ static void print_bloom_filter(struct bloom_filter *filter) {
 	}
 	printf("Filter_Length:%d\n", (int)filter->len);
 	printf("Filter_Data:");
-	for (i = 0; i < filter->len; i++){
+	for (i = 0; i < filter->len; i++) {
 		printf("%02x|", filter->data[i]);
 	}
 	printf("\n");
@@ -50,13 +50,13 @@ int cmd__bloom(int argc, const char **argv)
 		printf("Murmur3 Hash with seed=0:0x%08x\n", hashed);
 	}
 
-    if (!strcmp(argv[1], "generate_filter")) {
+	if (!strcmp(argv[1], "generate_filter")) {
 		struct bloom_filter filter;
 		int i = 2;
 		filter.len =  (settings.bits_per_entry + BITS_PER_WORD - 1) / BITS_PER_WORD;
 		filter.data = xcalloc(filter.len, sizeof(unsigned char));
 
-		if (!argv[2]){
+		if (!argv[2]) {
 			die("at least one input string expected");
 		}
 
@@ -68,7 +68,7 @@ int cmd__bloom(int argc, const char **argv)
 		print_bloom_filter(&filter);
 	}
 
-    if (!strcmp(argv[1], "get_filter_for_commit")) {
+	if (!strcmp(argv[1], "get_filter_for_commit")) {
 		struct object_id oid;
 		const char *end;
 		if (parse_oid_hex(argv[2], &oid, &end))
@@ -78,4 +78,4 @@ int cmd__bloom(int argc, const char **argv)
 	}
 
 	return 0;
-}
\ No newline at end of file
+}
-- 
2.26.2.827.g3c1233342b

Re: [PATCH 1/2] test-bloom: fix some whitespace issues

[Taylor Blau]

On Thu, Apr 23, 2020 at 04:59:07PM -0400, Jeff King wrote:
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/helper/test-bloom.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c
> index ce412664ba..f9c0ce2bae 100644
> --- a/t/helper/test-bloom.c
> +++ b/t/helper/test-bloom.c
> @@ -27,7 +27,7 @@ static void print_bloom_filter(struct bloom_filter *filter) {
>  	}
>  	printf("Filter_Length:%d\n", (int)filter->len);
>  	printf("Filter_Data:");
> -	for (i = 0; i < filter->len; i++){
> +	for (i = 0; i < filter->len; i++) {

Thanks for fixing the spacing, but I wonder if these braces should be
here at all. Since the body is one line long, maybe this should just be:

  for (i = 0; i < filter->len; i++)
    printf("%02x|", filter->data[i]);

>  		printf("%02x|", filter->data[i]);
>  	}
>  	printf("\n");
> @@ -50,13 +50,13 @@ int cmd__bloom(int argc, const char **argv)
>  		printf("Murmur3 Hash with seed=0:0x%08x\n", hashed);
>  	}
>
> -    if (!strcmp(argv[1], "generate_filter")) {
> +	if (!strcmp(argv[1], "generate_filter")) {

This spot looks good, and ditto for fixing the indentation.

>  		struct bloom_filter filter;
>  		int i = 2;
>  		filter.len =  (settings.bits_per_entry + BITS_PER_WORD - 1) / BITS_PER_WORD;
>  		filter.data = xcalloc(filter.len, sizeof(unsigned char));
>
> -		if (!argv[2]){
> +		if (!argv[2]) {

Same note here about the brace.

>  			die("at least one input string expected");
>  		}
>
> @@ -68,7 +68,7 @@ int cmd__bloom(int argc, const char **argv)
>  		print_bloom_filter(&filter);
>  	}
>
> -    if (!strcmp(argv[1], "get_filter_for_commit")) {
> +	if (!strcmp(argv[1], "get_filter_for_commit")) {
>  		struct object_id oid;
>  		const char *end;
>  		if (parse_oid_hex(argv[2], &oid, &end))
> @@ -78,4 +78,4 @@ int cmd__bloom(int argc, const char **argv)
>  	}
>
>  	return 0;
> -}
> \ No newline at end of file
> +}
> --
> 2.26.2.827.g3c1233342b
>

Thanks,
Taylor

Re: [PATCH 1/2] test-bloom: fix some whitespace issues

[Jeff King]

On Thu, Apr 23, 2020 at 03:01:55PM -0600, Taylor Blau wrote:

> On Thu, Apr 23, 2020 at 04:59:07PM -0400, Jeff King wrote:
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> >  t/helper/test-bloom.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c
> > index ce412664ba..f9c0ce2bae 100644
> > --- a/t/helper/test-bloom.c
> > +++ b/t/helper/test-bloom.c
> > @@ -27,7 +27,7 @@ static void print_bloom_filter(struct bloom_filter *filter) {
> >  	}
> >  	printf("Filter_Length:%d\n", (int)filter->len);
> >  	printf("Filter_Data:");
> > -	for (i = 0; i < filter->len; i++){
> > +	for (i = 0; i < filter->len; i++) {
> 
> Thanks for fixing the spacing, but I wonder if these braces should be
> here at all. Since the body is one line long, maybe this should just be:
> 
>   for (i = 0; i < filter->len; i++)
>     printf("%02x|", filter->data[i]);

I have to admit that I don't care either way, and I think we spend too
much time quibbling about braces or not-braces. It was really the bad
indentation that I cared about most.

-Peff

[PATCH 2/2] test-bloom: check that we have expected arguments

[Jeff King]

If "test-tool bloom" is not fed a command, or if arguments are missing
for some commands, it will just segfault. Let's check argc and write a
friendlier usage message.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-bloom.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c
index f9c0ce2bae..77eb27adac 100644
--- a/t/helper/test-bloom.c
+++ b/t/helper/test-bloom.c
@@ -43,10 +43,21 @@ static void get_bloom_filter_for_commit(const struct object_id *commit_oid)
 	print_bloom_filter(filter);
 }
 
+static const char *bloom_usage = "\n"
+"  test-tool bloom get_murmer3 <string>\n"
+"  test-tool bloom generate_filter <string> [<string>...]\n"
+"  test-tool get_filter_for_commit <commit-hex>\n";
+
 int cmd__bloom(int argc, const char **argv)
 {
+	if (argc < 2)
+		usage(bloom_usage);
+
 	if (!strcmp(argv[1], "get_murmur3")) {
-		uint32_t hashed = murmur3_seeded(0, argv[2], strlen(argv[2]));
+		uint32_t hashed;
+		if (argc < 3)
+			usage(bloom_usage);
+		hashed = murmur3_seeded(0, argv[2], strlen(argv[2]));
 		printf("Murmur3 Hash with seed=0:0x%08x\n", hashed);
 	}
 
@@ -56,9 +67,8 @@ int cmd__bloom(int argc, const char **argv)
 		filter.len =  (settings.bits_per_entry + BITS_PER_WORD - 1) / BITS_PER_WORD;
 		filter.data = xcalloc(filter.len, sizeof(unsigned char));
 
-		if (!argv[2]) {
-			die("at least one input string expected");
-		}
+		if (argc - 1 < i)
+			usage(bloom_usage);
 
 		while (argv[i]) {
 			add_string_to_filter(argv[i], &filter);
@@ -71,6 +81,8 @@ int cmd__bloom(int argc, const char **argv)
 	if (!strcmp(argv[1], "get_filter_for_commit")) {
 		struct object_id oid;
 		const char *end;
+		if (argc < 3)
+			usage(bloom_usage);
 		if (parse_oid_hex(argv[2], &oid, &end))
 			die("cannot parse oid '%s'", argv[2]);
 		init_bloom_filters();
-- 
2.26.2.827.g3c1233342b

Re: [PATCH 2/2] test-bloom: check that we have expected arguments

[Taylor Blau]

On Thu, Apr 23, 2020 at 04:59:14PM -0400, Jeff King wrote:
> If "test-tool bloom" is not fed a command, or if arguments are missing
> for some commands, it will just segfault. Let's check argc and write a
> friendlier usage message.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

Looks all good.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter

[Garima Singh]

On 4/23/2020 4:58 PM, Jeff King wrote:
> These are just a few bits I noticed in the test-tool helper when the
> topic hit next (my -Wunused-parameter patch complained that we never
> looked at argc).
> 
>   [1/2]: test-bloom: fix some whitespace issues
>   [2/2]: test-bloom: check that we have expected arguments
> 
>  t/helper/test-bloom.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> -Peff
> 

Thank you for doing this! 
Both patches look good to me. 
I also don't care about the brace/no-brace thing that 
Taylor brought up for 1/2. 

Cheers! 
Garima Singh

Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter

[Taylor Blau]

On Thu, Apr 23, 2020 at 06:14:36PM -0400, Garima Singh wrote:
>
> On 4/23/2020 4:58 PM, Jeff King wrote:
> > These are just a few bits I noticed in the test-tool helper when the
> > topic hit next (my -Wunused-parameter patch complained that we never
> > looked at argc).
> >
> >   [1/2]: test-bloom: fix some whitespace issues
> >   [2/2]: test-bloom: check that we have expected arguments
> >
> >  t/helper/test-bloom.c | 28 ++++++++++++++++++++--------
> >  1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > -Peff
> >
>
> Thank you for doing this!
> Both patches look good to me.
> I also don't care about the brace/no-brace thing that
> Taylor brought up for 1/2.

To be clear, I don't care about them either ;). Maybe it's time that we
relax that rule (if it seems that a good number of us don't mind it
either way)..?

> Cheers!
> Garima Singh

Thanks,
Taylor

Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> To be clear, I don't care about them either ;). Maybe it's time that we
> relax that rule (if it seems that a good number of us don't mind it
> either way)..?

I'd prefer our reviewers (even more preferably the authors) to be
careful about new code, but it probably is not worth reviewing
patches that only fix them and do nothing else.

Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter

[Danh Doan]

On 2020-04-23 16:58:51-0400, Jeff King <peff@peff.net> wrote:
> These are just a few bits I noticed in the test-tool helper when the
> topic hit next (my -Wunused-parameter patch complained that we never
> looked at argc).

I think I'll add this one to those few bits.
Old version of this change was sent here:
<20200423133937.GA1984@danh.dev>

But that version doesn't have the fixup for sh script.

Garima Singh: Could you please change your editor to add final new line?

I've take another look into bloom.h.

I think we should drop BITS_PER_WORD definition and use CHAR_BIT
instead. It's a standard definition.

To me, a WORD is an `int`, at least I was told that when I was still
in university and study about computer science.

-----------------8<---------------------
From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?=
 <congdanhqx@gmail.com>
Date: Thu, 23 Apr 2020 20:24:50 +0700
Subject: [PATCH] bloom: fix `make sparse` warning
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* We need a `final_new_line` to make our source code as text file, per
  POSIX and C specification.
* `bloom_filters` should be limited to interal linkage only


Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
Feel free to fix up to current series
 bloom.c               | 4 ++--
 bloom.h               | 2 +-
 t/helper/test-bloom.c | 2 +-
 t/t0095-bloom.sh      | 2 +-
 t/t4216-log-bloom.sh  | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/bloom.c b/bloom.c
index dd9bab9bbd..ee025e0c61 100644
--- a/bloom.c
+++ b/bloom.c
@@ -9,7 +9,7 @@
 
 define_commit_slab(bloom_filter_slab, struct bloom_filter);
 
-struct bloom_filter_slab bloom_filters;
+static struct bloom_filter_slab bloom_filters;
 
 struct pathmap_hash_entry {
     struct hashmap_entry entry;
@@ -273,4 +273,4 @@ int bloom_filter_contains(const struct bloom_filter *filter,
 	}
 
 	return 1;
-}
\ No newline at end of file
+}
diff --git a/bloom.h b/bloom.h
index b935186425..e0e59e0754 100644
--- a/bloom.h
+++ b/bloom.h
@@ -87,4 +87,4 @@ int bloom_filter_contains(const struct bloom_filter *filter,
 			  const struct bloom_key *key,
 			  const struct bloom_filter_settings *settings);
 
-#endif
\ No newline at end of file
+#endif
diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c
index 77eb27adac..456f5ea7f9 100644
--- a/t/helper/test-bloom.c
+++ b/t/helper/test-bloom.c
@@ -3,7 +3,7 @@
 #include "test-tool.h"
 #include "commit.h"
 
-struct bloom_filter_settings settings = DEFAULT_BLOOM_FILTER_SETTINGS;
+static struct bloom_filter_settings settings = DEFAULT_BLOOM_FILTER_SETTINGS;
 
 static void add_string_to_filter(const char *data, struct bloom_filter *filter) {
 		struct bloom_key key;
diff --git a/t/t0095-bloom.sh b/t/t0095-bloom.sh
index 8f9eef116d..809ec7b0b8 100755
--- a/t/t0095-bloom.sh
+++ b/t/t0095-bloom.sh
@@ -114,4 +114,4 @@ test_expect_success EXPENSIVE 'get bloom filter for commit with 513 changes' '
 	test_cmp expect actual
 '
 
-test_done
\ No newline at end of file
+test_done
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index c7011f33e2..21b68dd6c8 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -152,4 +152,4 @@ test_expect_success 'Use Bloom filters if they exist in the latest but not all c
 	test_bloom_filters_used_when_some_filters_are_missing "-- A/B"
 '
 
-test_done
\ No newline at end of file
+test_done
-- 
2.26.2.384.g435bf60bd5


-- 
Danh

Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter

[Jeff King]

On Fri, Apr 24, 2020 at 08:00:47AM +0700, Danh Doan wrote:

> On 2020-04-23 16:58:51-0400, Jeff King <peff@peff.net> wrote:
> > These are just a few bits I noticed in the test-tool helper when the
> > topic hit next (my -Wunused-parameter patch complained that we never
> > looked at argc).
> 
> I think I'll add this one to those few bits.

Yeah, they all look sensible (I should have looked for more "No newline"
cases.

> I've take another look into bloom.h.
> 
> I think we should drop BITS_PER_WORD definition and use CHAR_BIT
> instead. It's a standard definition.
> 
> To me, a WORD is an `int`, at least I was told that when I was still
> in university and study about computer science.

Yes, I agree it would be more clear as just CHAR_BIT if we are using
single-char words. But I suspect the code could be looking at the bit
patterns using larger word sizes (e.g., all of the ewah code uses 64-bit
words). That might be worth exploring.

-Peff