[PATCH 0/3] hn/reftable: fixes for building with -DNDEBUG

[PATCH 0/3] hn/reftable: fixes for building with -DNDEBUG

[Carlo Marcelo Arenas Belón]

they are all based against the last reroll for that topic in "seen"

they all introduce the use of a macro from git (MAYBE_UNUSED) that
hopefully is not controversial (ex: when applied to libgit2) as other
git-compat is already in use.

they are named so they could be easily squashed into their corresponding
patches, but at least the third one needs some further evaluation, where
the code in the assert gets removed and with it the whole function
becomes a NOOP, even if the tests still pass.

there is also an additional fixup for supporting OpenBSD 7 (now in beta)
that will be sent independently; sorry for the noise

Carlo Marcelo Arenas Belón (3):
  fixup! refs: RFC: Reftable support for git-core
  fixup! reftable: add merged table view
  fixup! reftable: add a heap-based priority queue for reftable records

 refs/reftable-backend.c | 5 +++--
 reftable/merged_test.c  | 8 ++++----
 reftable/pq.c           | 2 ++
 3 files changed, 9 insertions(+), 6 deletions(-)

-- 
2.33.0.481.g26d3bed244

[PATCH 1/3] fixup! refs: RFC: Reftable support for git-core

[Carlo Marcelo Arenas Belón]

need to reorder the variables to hopefully make it easier to see why
they might not be used since assert will compile out itself with -DNDEBUG.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 refs/reftable-backend.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 61ee144e19..5d733b0496 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -127,10 +127,11 @@ static void clear_reftable_log_record(struct reftable_log_record *log)
 
 static void fill_reftable_log_record(struct reftable_log_record *log)
 {
-	const char *info = git_committer_info(0);
 	struct ident_split split = { NULL };
-	int result = split_ident_line(&split, info, strlen(info));
 	int sign = 1;
+	MAYBE_UNUSED const char *info = git_committer_info(0);
+	MAYBE_UNUSED int result = split_ident_line(&split, info, strlen(info));
+
 	assert(0 == result);
 
 	reftable_log_record_release(log);
-- 
2.33.0.481.g26d3bed244

Re: [PATCH 1/3] fixup! refs: RFC: Reftable support for git-core

[Jeff King]

On Wed, Sep 01, 2021 at 10:30:21PM -0700, Carlo Marcelo Arenas Belón wrote:

> need to reorder the variables to hopefully make it easier to see why
> they might not be used since assert will compile out itself with -DNDEBUG.

This should probably lead with the reason for the patch (avoiding errors
with NDEBUG), and then mention any other bits (like "we also reorder for
clarity"). That makes the point of the patch easier to see.

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 61ee144e19..5d733b0496 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -127,10 +127,11 @@ static void clear_reftable_log_record(struct reftable_log_record *log)
>  
>  static void fill_reftable_log_record(struct reftable_log_record *log)
>  {
> -	const char *info = git_committer_info(0);
>  	struct ident_split split = { NULL };
> -	int result = split_ident_line(&split, info, strlen(info));
>  	int sign = 1;
> +	MAYBE_UNUSED const char *info = git_committer_info(0);
> +	MAYBE_UNUSED int result = split_ident_line(&split, info, strlen(info));

I don't think you need MAYBE_UNUSED on "info"; it is always used, even
if the assert isn't compiled.

>  	assert(0 == result);

IMHO this would be better converted to:

  if (result)
	BUG("unable to parse output of get_committer_info()");

That solves the problem, avoids introducing head-scratching constructs
like MAYBE_UNUSED, and gives readers more of a clue about what we
expected.

-Peff

PS I do think it's pretty ugly that we have to re-split the ident from
   git_committer_info(), which just took the individual pieces and
   stuffed them into a string! But our ident functions are a little
   cumbersome here (we even have fmt_name(), but I think it's too picky
   about IDENT_STRICT). I suspect it could be fixed with some
   refactoring, but that's way out of scope for your series (and for the
   reftable series itself), so let's leave it for now.

Re: [PATCH 1/3] fixup! refs: RFC: Reftable support for git-core

[Han-Wen Nienhuys]

On Thu, Sep 2, 2021 at 11:05 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Sep 01, 2021 at 10:30:21PM -0700, Carlo Marcelo Arenas Belón wrote:
>
> > need to reorder the variables to hopefully make it easier to see why
> > they might not be used since assert will compile out itself with -DNDEBUG.
>
> This should probably lead with the reason for the patch (avoiding errors
> with NDEBUG), and then mention any other bits (like "we also reorder for
> clarity"). That makes the point of the patch easier to see.

Counterpoint: I can see what the problem is (thanks, Carlo!) but would
likely fix it in a different way, so effort spent in polishing commit
messages will be wasted.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

[PATCH 2/3] fixup! reftable: add merged table view

[Carlo Marcelo Arenas Belón]

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Just like in the next patch, having relevant code in the assert might be
a problem, but this is mostly test code and still passes.

 reftable/merged_test.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index f58e44003e..b7be4dcf9f 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -47,8 +47,8 @@ static void write_test_table(struct strbuf *buf,
 	reftable_writer_set_limits(w, min, max);
 
 	for (i = 0; i < n; i++) {
-		uint64_t before = refs[i].update_index;
-		int n = reftable_writer_add_ref(w, &refs[i]);
+		MAYBE_UNUSED uint64_t before = refs[i].update_index;
+		MAYBE_UNUSED int n = reftable_writer_add_ref(w, &refs[i]);
 		assert(n == 0);
 		assert(before == refs[i].update_index);
 	}
@@ -181,7 +181,7 @@ static void test_merged(const char *fn_name)
 		},
 	};
 
-	struct reftable_ref_record want[] = {
+	MAYBE_UNUSED struct reftable_ref_record want[] = {
 		r2[0],
 		r1[1],
 		r3[0],
@@ -251,7 +251,7 @@ static void test_default_write_opts(const char *fn_name)
 	int err;
 	struct reftable_block_source source = { NULL };
 	struct reftable_table *tab = reftable_calloc(sizeof(*tab) * 1);
-	uint32_t hash_id;
+	MAYBE_UNUSED uint32_t hash_id;
 	struct reftable_reader *rd = NULL;
 	struct reftable_merged_table *merged = NULL;
 
-- 
2.33.0.481.g26d3bed244

[PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records

[Carlo Marcelo Arenas Belón]

not a very useful option, but the fact the main logic is done inside
and assert that then gets compiled out when -DNDEBUG might be worth
reconsidering.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 reftable/pq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/reftable/pq.c b/reftable/pq.c
index 8918d158e2..a6acca006b 100644
--- a/reftable/pq.c
+++ b/reftable/pq.c
@@ -43,12 +43,14 @@ int merged_iter_pqueue_is_empty(struct merged_iter_pqueue pq)
 
 void merged_iter_pqueue_check(struct merged_iter_pqueue pq)
 {
+#ifndef NDEBUG
 	int i = 0;
 	for (i = 1; i < pq.len; i++) {
 		int parent = (i - 1) / 2;
 
 		assert(pq_less(pq.heap[parent], pq.heap[i]));
 	}
+#endif
 }
 
 struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)
-- 
2.33.0.481.g26d3bed244

Re: [PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records

[Jeff King]

On Wed, Sep 01, 2021 at 10:30:23PM -0700, Carlo Marcelo Arenas Belón wrote:

> diff --git a/reftable/pq.c b/reftable/pq.c
> index 8918d158e2..a6acca006b 100644
> --- a/reftable/pq.c
> +++ b/reftable/pq.c
> @@ -43,12 +43,14 @@ int merged_iter_pqueue_is_empty(struct merged_iter_pqueue pq)
>  
>  void merged_iter_pqueue_check(struct merged_iter_pqueue pq)
>  {
> +#ifndef NDEBUG
>  	int i = 0;
>  	for (i = 1; i < pq.len; i++) {
>  		int parent = (i - 1) / 2;
>  
>  		assert(pq_less(pq.heap[parent], pq.heap[i]));
>  	}
> +#endif
>  }

This will trigger -Wunused-parameter warnings, since the function body
is now empty when NDEBUG is undefined. Probably switching the assert()
to die() would be better, since the whole point of the function is just
to exit on error.

If there's a problem using die() from the reftable code, it could also
return an error and the caller in the test helper could propagate it.

-Peff

Re: [PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 01, 2021 at 10:30:23PM -0700, Carlo Marcelo Arenas Belón wrote:
>
>> diff --git a/reftable/pq.c b/reftable/pq.c
>> index 8918d158e2..a6acca006b 100644
>> --- a/reftable/pq.c
>> +++ b/reftable/pq.c
>> @@ -43,12 +43,14 @@ int merged_iter_pqueue_is_empty(struct merged_iter_pqueue pq)
>>  
>>  void merged_iter_pqueue_check(struct merged_iter_pqueue pq)
>>  {
>> +#ifndef NDEBUG
>>  	int i = 0;
>>  	for (i = 1; i < pq.len; i++) {
>>  		int parent = (i - 1) / 2;
>>  
>>  		assert(pq_less(pq.heap[parent], pq.heap[i]));
>>  	}
>> +#endif
>>  }
>
> This will trigger -Wunused-parameter warnings, since the function body
> is now empty when NDEBUG is undefined. Probably switching the assert()
> to die() would be better, since the whole point of the function is just
> to exit on error.
>
> If there's a problem using die() from the reftable code, it could also
> return an error and the caller in the test helper could propagate it.

I agree that the patch as posted does not help but if this is
originally an assertion, then it should never trigger in real life,
so BUG() would be more appropriate than an error return, no?

Re: [PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records

[Jeff King]

On Thu, Sep 02, 2021 at 01:08:58PM -0700, Junio C Hamano wrote:

> > This will trigger -Wunused-parameter warnings, since the function body
> > is now empty when NDEBUG is undefined. Probably switching the assert()
> > to die() would be better, since the whole point of the function is just
> > to exit on error.
> >
> > If there's a problem using die() from the reftable code, it could also
> > return an error and the caller in the test helper could propagate it.
> 
> I agree that the patch as posted does not help but if this is
> originally an assertion, then it should never trigger in real life,
> so BUG() would be more appropriate than an error return, no?

My thinking was that it doesn't make much sense as an assertion in the
first place. It is not a side effect of "let's make sure things are as
we expect while we're doing some other operation". The whole point of
the function is: is this data structure properly in order.

But I guess you could argue that calling the function is itself a form
of assertion. I don't really care that much either way, so whatever
Han-Wen prefers is fine with me (but I do think it is worth addressing
the warning Carlo found _somehow_).

-Peff

Re: [PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I agree that the patch as posted does not help but if this is
>> originally an assertion, then it should never trigger in real life,
>> so BUG() would be more appropriate than an error return, no?
>
> My thinking was that it doesn't make much sense as an assertion in the
> first place. It is not a side effect of "let's make sure things are as
> we expect while we're doing some other operation". The whole point of
> the function is: is this data structure properly in order.

Very true.  Ah, so you mean the way this function is supposed to be
used is to _call_ it, like so:

	if (!is_our_data_structure_healthy())
		BUG(...);

It makes it easier to reason about what the function is doing, I
guess.

> But I guess you could argue that calling the function is itself a form
> of assertion. I don't really care that much either way, so whatever
> Han-Wen prefers is fine with me (but I do think it is worth addressing
> the warning Carlo found _somehow_).
>
> -Peff