{"thread":{"id":"56426","subject":"[PATCH 0/3] hn/reftable: fixes for building with -DNDEBUG","startedAt":"2021-09-02T05:30:41Z","lastAt":"2021-09-03T04:42:06Z","messageCount":10,"participants":["Carlo Marcelo Arenas Belón","Jeff King","Han-Wen Nienhuys","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"434485","messageId":"20210902053023.44006-1-carenas@gmail.com","threadId":"56426","inReplyTo":null,"subject":"[PATCH 0/3] hn/reftable: fixes for building with -DNDEBUG","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-02T05:30:20Z","receivedAt":"2021-09-02T05:30:41Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"they are all based against the last reroll for that topic in \"seen\"\n\nthey all introduce the use of a macro from git (MAYBE_UNUSED) that\nhopefully is not controversial (ex: when applied to libgit2) as other\ngit-compat is already in use.\n\nthey are named so they could be easily squashed into their corresponding\npatches, but at least the third one needs some further evaluation, where\nthe code in the assert gets removed and with it the whole function\nbecomes a NOOP, even if the tests still pass.\n\nthere is also an additional fixup for supporting OpenBSD 7 (now in beta)\nthat will be sent independently; sorry for the noise\n\nCarlo Marcelo Arenas Belón (3):\n fixup! refs: RFC: Reftable support for git-core\n fixup! reftable: add merged table view\n fixup! reftable: add a heap-based priority queue for reftable records\n\n refs/reftable-backend.c | 5 +++--\n reftable/merged_test.c | 8 ++++----\n reftable/pq.c | 2 ++\n 3 files changed, 9 insertions(+), 6 deletions(-)\n\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"434486","messageId":"20210902053023.44006-2-carenas@gmail.com","threadId":"56426","inReplyTo":"20210902053023.44006-1-carenas@gmail.com","subject":"[PATCH 1/3] fixup! refs: RFC: Reftable support for git-core","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-02T05:30:21Z","receivedAt":"2021-09-02T05:30:43Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"need to reorder the variables to hopefully make it easier to see why\nthey might not be used since assert will compile out itself with -DNDEBUG.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n refs/reftable-backend.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 61ee144e19..5d733b0496 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -127,10 +127,11 @@ static void clear_reftable_log_record(struct reftable_log_record *log)\n \n static void fill_reftable_log_record(struct reftable_log_record *log)\n {\n-\tconst char *info = git_committer_info(0);\n \tstruct ident_split split = { NULL };\n-\tint result = split_ident_line(&split, info, strlen(info));\n \tint sign = 1;\n+\tMAYBE_UNUSED const char *info = git_committer_info(0);\n+\tMAYBE_UNUSED int result = split_ident_line(&split, info, strlen(info));\n+\n \tassert(0 == result);\n \n \treftable_log_record_release(log);\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"434487","messageId":"20210902053023.44006-3-carenas@gmail.com","threadId":"56426","inReplyTo":"20210902053023.44006-1-carenas@gmail.com","subject":"[PATCH 2/3] fixup! reftable: add merged table view","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-02T05:30:22Z","receivedAt":"2021-09-02T05:30:43Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Signed-off-by: Carlo Marcelo Arenas Belón \n---\nJust like in the next patch, having relevant code in the assert might be\na problem, but this is mostly test code and still passes.\n\n reftable/merged_test.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/reftable/merged_test.c b/reftable/merged_test.c\nindex f58e44003e..b7be4dcf9f 100644\n--- a/reftable/merged_test.c\n+++ b/reftable/merged_test.c\n@@ -47,8 +47,8 @@ static void write_test_table(struct strbuf *buf,\n \treftable_writer_set_limits(w, min, max);\n \n \tfor (i = 0; i < n; i++) {\n-\t\tuint64_t before = refs[i].update_index;\n-\t\tint n = reftable_writer_add_ref(w, &refs[i]);\n+\t\tMAYBE_UNUSED uint64_t before = refs[i].update_index;\n+\t\tMAYBE_UNUSED int n = reftable_writer_add_ref(w, &refs[i]);\n \t\tassert(n == 0);\n \t\tassert(before == refs[i].update_index);\n \t}\n@@ -181,7 +181,7 @@ static void test_merged(const char *fn_name)\n \t\t},\n \t};\n \n-\tstruct reftable_ref_record want[] = {\n+\tMAYBE_UNUSED struct reftable_ref_record want[] = {\n \t\tr2[0],\n \t\tr1[1],\n \t\tr3[0],\n@@ -251,7 +251,7 @@ static void test_default_write_opts(const char *fn_name)\n \tint err;\n \tstruct reftable_block_source source = { NULL };\n \tstruct reftable_table *tab = reftable_calloc(sizeof(*tab) * 1);\n-\tuint32_t hash_id;\n+\tMAYBE_UNUSED uint32_t hash_id;\n \tstruct reftable_reader *rd = NULL;\n \tstruct reftable_merged_table *merged = NULL;\n \n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"434488","messageId":"20210902053023.44006-4-carenas@gmail.com","threadId":"56426","inReplyTo":"20210902053023.44006-1-carenas@gmail.com","subject":"[PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-02T05:30:23Z","receivedAt":"2021-09-02T05:30:45Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"not a very useful option, but the fact the main logic is done inside\nand assert that then gets compiled out when -DNDEBUG might be worth\nreconsidering.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n reftable/pq.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/reftable/pq.c b/reftable/pq.c\nindex 8918d158e2..a6acca006b 100644\n--- a/reftable/pq.c\n+++ b/reftable/pq.c\n@@ -43,12 +43,14 @@ int merged_iter_pqueue_is_empty(struct merged_iter_pqueue pq)\n \n void merged_iter_pqueue_check(struct merged_iter_pqueue pq)\n {\n+#ifndef NDEBUG\n \tint i = 0;\n \tfor (i = 1; i < pq.len; i++) {\n \t\tint parent = (i - 1) / 2;\n \n \t\tassert(pq_less(pq.heap[parent], pq.heap[i]));\n \t}\n+#endif\n }\n \n struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"434495","messageId":"YTCTyzfIQlme4xee@coredump.intra.peff.net","threadId":"56426","inReplyTo":"20210902053023.44006-2-carenas@gmail.com","subject":"Re: [PATCH 1/3] fixup! refs: RFC: Reftable support for git-core","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-02T09:05:15Z","receivedAt":"2021-09-02T09:05:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 01, 2021 at 10:30:21PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> need to reorder the variables to hopefully make it easier to see why\n> they might not be used since assert will compile out itself with -DNDEBUG.\n\nThis should probably lead with the reason for the patch (avoiding errors\nwith NDEBUG), and then mention any other bits (like \"we also reorder for\nclarity\"). That makes the point of the patch easier to see.\n\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 61ee144e19..5d733b0496 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -127,10 +127,11 @@ static void clear_reftable_log_record(struct reftable_log_record *log)\n> \n> static void fill_reftable_log_record(struct reftable_log_record *log)\n> {\n> -\tconst char *info = git_committer_info(0);\n> \tstruct ident_split split = { NULL };\n> -\tint result = split_ident_line(&split, info, strlen(info));\n> \tint sign = 1;\n> +\tMAYBE_UNUSED const char *info = git_committer_info(0);\n> +\tMAYBE_UNUSED int result = split_ident_line(&split, info, strlen(info));\n\nI don't think you need MAYBE_UNUSED on \"info\"; it is always used, even\nif the assert isn't compiled.\n\n> \tassert(0 == result);\n\nIMHO this would be better converted to:\n\n if (result)\n\tBUG(\"unable to parse output of get_committer_info()\");\n\nThat solves the problem, avoids introducing head-scratching constructs\nlike MAYBE_UNUSED, and gives readers more of a clue about what we\nexpected.\n\n-Peff\n\nPS I do think it's pretty ugly that we have to re-split the ident from\n git_committer_info(), which just took the individual pieces and\n stuffed them into a string! But our ident functions are a little\n cumbersome here (we even have fmt_name(), but I think it's too picky\n about IDENT_STRICT). I suspect it could be fixed with some\n refactoring, but that's way out of scope for your series (and for the\n reftable series itself), so let's leave it for now.\n"},{"id":"434503","messageId":"YTCU3F+0sKcrym6F@coredump.intra.peff.net","threadId":"56426","inReplyTo":"20210902053023.44006-4-carenas@gmail.com","subject":"Re: [PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-02T09:09:48Z","receivedAt":"2021-09-02T09:09:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 01, 2021 at 10:30:23PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> diff --git a/reftable/pq.c b/reftable/pq.c\n> index 8918d158e2..a6acca006b 100644\n> --- a/reftable/pq.c\n> +++ b/reftable/pq.c\n> @@ -43,12 +43,14 @@ int merged_iter_pqueue_is_empty(struct merged_iter_pqueue pq)\n> \n> void merged_iter_pqueue_check(struct merged_iter_pqueue pq)\n> {\n> +#ifndef NDEBUG\n> \tint i = 0;\n> \tfor (i = 1; i < pq.len; i++) {\n> \t\tint parent = (i - 1) / 2;\n> \n> \t\tassert(pq_less(pq.heap[parent], pq.heap[i]));\n> \t}\n> +#endif\n> }\n\nThis will trigger -Wunused-parameter warnings, since the function body\nis now empty when NDEBUG is undefined. Probably switching the assert()\nto die() would be better, since the whole point of the function is just\nto exit on error.\n\nIf there's a problem using die() from the reftable code, it could also\nreturn an error and the caller in the test helper could propagate it.\n\n-Peff\n"},{"id":"434507","messageId":"CAFQ2z_PK20PTsHsK8A0-vdmFLNSxRMP3_Kn0rQmZViiGKzGV+w@mail.gmail.com","threadId":"56426","inReplyTo":"YTCTyzfIQlme4xee@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] fixup! refs: RFC: Reftable support for git-core","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-09-02T09:26:01Z","receivedAt":"2021-09-02T09:26:15Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, Sep 2, 2021 at 11:05 AM Jeff King wrote:\n>\n> On Wed, Sep 01, 2021 at 10:30:21PM -0700, Carlo Marcelo Arenas Belón wrote:\n>\n> > need to reorder the variables to hopefully make it easier to see why\n> > they might not be used since assert will compile out itself with -DNDEBUG.\n>\n> This should probably lead with the reason for the patch (avoiding errors\n> with NDEBUG), and then mention any other bits (like \"we also reorder for\n> clarity\"). That makes the point of the patch easier to see.\n\nCounterpoint: I can see what the problem is (thanks, Carlo!) but would\nlikely fix it in a different way, so effort spent in polishing commit\nmessages will be wasted.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"434594","messageId":"xmqqeea6kaed.fsf@gitster.g","threadId":"56426","inReplyTo":"YTCU3F+0sKcrym6F@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-02T20:08:58Z","receivedAt":"2021-09-02T20:09:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Sep 01, 2021 at 10:30:23PM -0700, Carlo Marcelo Arenas Belón wrote:\n>\n>> diff --git a/reftable/pq.c b/reftable/pq.c\n>> index 8918d158e2..a6acca006b 100644\n>> --- a/reftable/pq.c\n>> +++ b/reftable/pq.c\n>> @@ -43,12 +43,14 @@ int merged_iter_pqueue_is_empty(struct merged_iter_pqueue pq)\n>> \n>> void merged_iter_pqueue_check(struct merged_iter_pqueue pq)\n>> {\n>> +#ifndef NDEBUG\n>> \tint i = 0;\n>> \tfor (i = 1; i < pq.len; i++) {\n>> \t\tint parent = (i - 1) / 2;\n>> \n>> \t\tassert(pq_less(pq.heap[parent], pq.heap[i]));\n>> \t}\n>> +#endif\n>> }\n>\n> This will trigger -Wunused-parameter warnings, since the function body\n> is now empty when NDEBUG is undefined. Probably switching the assert()\n> to die() would be better, since the whole point of the function is just\n> to exit on error.\n>\n> If there's a problem using die() from the reftable code, it could also\n> return an error and the caller in the test helper could propagate it.\n\nI agree that the patch as posted does not help but if this is\noriginally an assertion, then it should never trigger in real life,\nso BUG() would be more appropriate than an error return, no?\n\n"},{"id":"434607","messageId":"YTFS2oQz9QEzBe0Q@coredump.intra.peff.net","threadId":"56426","inReplyTo":"xmqqeea6kaed.fsf@gitster.g","subject":"Re: [PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-02T22:40:26Z","receivedAt":"2021-09-02T22:40:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 02, 2021 at 01:08:58PM -0700, Junio C Hamano wrote:\n\n> > This will trigger -Wunused-parameter warnings, since the function body\n> > is now empty when NDEBUG is undefined. Probably switching the assert()\n> > to die() would be better, since the whole point of the function is just\n> > to exit on error.\n> >\n> > If there's a problem using die() from the reftable code, it could also\n> > return an error and the caller in the test helper could propagate it.\n> \n> I agree that the patch as posted does not help but if this is\n> originally an assertion, then it should never trigger in real life,\n> so BUG() would be more appropriate than an error return, no?\n\nMy thinking was that it doesn't make much sense as an assertion in the\nfirst place. It is not a side effect of \"let's make sure things are as\nwe expect while we're doing some other operation\". The whole point of\nthe function is: is this data structure properly in order.\n\nBut I guess you could argue that calling the function is itself a form\nof assertion. I don't really care that much either way, so whatever\nHan-Wen prefers is fine with me (but I do think it is worth addressing\nthe warning Carlo found _somehow_).\n\n-Peff\n"},{"id":"434623","messageId":"xmqqbl5agtid.fsf@gitster.g","threadId":"56426","inReplyTo":"YTFS2oQz9QEzBe0Q@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] fixup! reftable: add a heap-based priority queue for reftable records","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-03T04:42:02Z","receivedAt":"2021-09-03T04:42:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> I agree that the patch as posted does not help but if this is\n>> originally an assertion, then it should never trigger in real life,\n>> so BUG() would be more appropriate than an error return, no?\n>\n> My thinking was that it doesn't make much sense as an assertion in the\n> first place. It is not a side effect of \"let's make sure things are as\n> we expect while we're doing some other operation\". The whole point of\n> the function is: is this data structure properly in order.\n\nVery true. Ah, so you mean the way this function is supposed to be\nused is to _call_ it, like so:\n\n\tif (!is_our_data_structure_healthy())\n\t\tBUG(...);\n\nIt makes it easier to reason about what the function is doing, I\nguess.\n\n> But I guess you could argue that calling the function is itself a form\n> of assertion. I don't really care that much either way, so whatever\n> Han-Wen prefers is fine with me (but I do think it is worth addressing\n> the warning Carlo found _somehow_).\n>\n> -Peff\n"}]}