* [PATCH 0/2] minor fixups for gs/commit-graph-path-filter
@ 2020-04-23 20:58 Jeff King
2020-04-23 20:59 ` [PATCH 1/2] test-bloom: fix some whitespace issues Jeff King
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Jeff King @ 2020-04-23 20:58 UTC (permalink / raw)
To: git; +Cc: Garima Singh, Derrick Stolee
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] test-bloom: fix some whitespace issues
2020-04-23 20:58 [PATCH 0/2] minor fixups for gs/commit-graph-path-filter Jeff King
@ 2020-04-23 20:59 ` Jeff King
2020-04-23 21:01 ` Taylor Blau
2020-04-23 20:59 ` [PATCH 2/2] test-bloom: check that we have expected arguments Jeff King
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2020-04-23 20:59 UTC (permalink / raw)
To: git; +Cc: Garima Singh, Derrick Stolee
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] test-bloom: check that we have expected arguments
2020-04-23 20:58 [PATCH 0/2] minor fixups for gs/commit-graph-path-filter Jeff King
2020-04-23 20:59 ` [PATCH 1/2] test-bloom: fix some whitespace issues Jeff King
@ 2020-04-23 20:59 ` Jeff King
2020-04-23 21:02 ` Taylor Blau
2020-04-23 22:14 ` [PATCH 0/2] minor fixups for gs/commit-graph-path-filter Garima Singh
2020-04-24 1:00 ` Danh Doan
3 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2020-04-23 20:59 UTC (permalink / raw)
To: git; +Cc: Garima Singh, Derrick Stolee
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] test-bloom: fix some whitespace issues
2020-04-23 20:59 ` [PATCH 1/2] test-bloom: fix some whitespace issues Jeff King
@ 2020-04-23 21:01 ` Taylor Blau
2020-04-23 21:04 ` Jeff King
0 siblings, 1 reply; 11+ messages in thread
From: Taylor Blau @ 2020-04-23 21:01 UTC (permalink / raw)
To: Jeff King; +Cc: git, Garima Singh, Derrick Stolee
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] test-bloom: check that we have expected arguments
2020-04-23 20:59 ` [PATCH 2/2] test-bloom: check that we have expected arguments Jeff King
@ 2020-04-23 21:02 ` Taylor Blau
0 siblings, 0 replies; 11+ messages in thread
From: Taylor Blau @ 2020-04-23 21:02 UTC (permalink / raw)
To: Jeff King; +Cc: git, Garima Singh, Derrick Stolee
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] test-bloom: fix some whitespace issues
2020-04-23 21:01 ` Taylor Blau
@ 2020-04-23 21:04 ` Jeff King
0 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2020-04-23 21:04 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Garima Singh, Derrick Stolee
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter
2020-04-23 20:58 [PATCH 0/2] minor fixups for gs/commit-graph-path-filter Jeff King
2020-04-23 20:59 ` [PATCH 1/2] test-bloom: fix some whitespace issues Jeff King
2020-04-23 20:59 ` [PATCH 2/2] test-bloom: check that we have expected arguments Jeff King
@ 2020-04-23 22:14 ` Garima Singh
2020-04-24 16:58 ` Taylor Blau
2020-04-24 1:00 ` Danh Doan
3 siblings, 1 reply; 11+ messages in thread
From: Garima Singh @ 2020-04-23 22:14 UTC (permalink / raw)
To: Jeff King, git; +Cc: Garima Singh, Derrick Stolee
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter
2020-04-23 20:58 [PATCH 0/2] minor fixups for gs/commit-graph-path-filter Jeff King
` (2 preceding siblings ...)
2020-04-23 22:14 ` [PATCH 0/2] minor fixups for gs/commit-graph-path-filter Garima Singh
@ 2020-04-24 1:00 ` Danh Doan
2020-04-24 5:25 ` Jeff King
3 siblings, 1 reply; 11+ messages in thread
From: Danh Doan @ 2020-04-24 1:00 UTC (permalink / raw)
To: Jeff King; +Cc: git, Garima Singh, Derrick Stolee
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter
2020-04-24 1:00 ` Danh Doan
@ 2020-04-24 5:25 ` Jeff King
0 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2020-04-24 5:25 UTC (permalink / raw)
To: Danh Doan; +Cc: git, Garima Singh, Derrick Stolee
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter
2020-04-23 22:14 ` [PATCH 0/2] minor fixups for gs/commit-graph-path-filter Garima Singh
@ 2020-04-24 16:58 ` Taylor Blau
2020-04-24 20:00 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Taylor Blau @ 2020-04-24 16:58 UTC (permalink / raw)
To: Garima Singh; +Cc: Jeff King, git, Garima Singh, Derrick Stolee
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] minor fixups for gs/commit-graph-path-filter
2020-04-24 16:58 ` Taylor Blau
@ 2020-04-24 20:00 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2020-04-24 20:00 UTC (permalink / raw)
To: Taylor Blau; +Cc: Garima Singh, Jeff King, git, Garima Singh, Derrick Stolee
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-24 20:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 20:58 [PATCH 0/2] minor fixups for gs/commit-graph-path-filter Jeff King
2020-04-23 20:59 ` [PATCH 1/2] test-bloom: fix some whitespace issues Jeff King
2020-04-23 21:01 ` Taylor Blau
2020-04-23 21:04 ` Jeff King
2020-04-23 20:59 ` [PATCH 2/2] test-bloom: check that we have expected arguments Jeff King
2020-04-23 21:02 ` Taylor Blau
2020-04-23 22:14 ` [PATCH 0/2] minor fixups for gs/commit-graph-path-filter Garima Singh
2020-04-24 16:58 ` Taylor Blau
2020-04-24 20:00 ` Junio C Hamano
2020-04-24 1:00 ` Danh Doan
2020-04-24 5:25 ` Jeff King
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.