{"thread":{"id":"59634","subject":"[PATCH 1/2] notes: clean up confusing NULL checks in init_notes()","startedAt":"2023-04-22T13:55:47Z","lastAt":"2023-04-24T21:57:29Z","messageCount":5,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"475839","messageId":"20230422135543.GA3942829@coredump.intra.peff.net","threadId":"59634","inReplyTo":"20230422135455.GA3942740@coredump.intra.peff.net","subject":"[PATCH 1/2] notes: clean up confusing NULL checks in init_notes()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-22T13:55:43Z","receivedAt":"2023-04-22T13:55:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Coverity complains that we check whether \"notes_ref\" is NULL, but it was\nalready implied to be non-NULL earlier in the function. And this is\ntrue; since b9342b3fd63 (refs: add array of ref namespaces, 2022-08-05),\nwe call xstrdup(notes_ref) unconditionally, which would segfault if it\nwas NULL.\n\nBut that commit is actually doing the right thing. Even if NULL is\npassed into the function, we'll use default_notes_ref() as a fallback,\nwhich will never return NULL (it tries a few options, but its last\nresort is a string literal). Ironically, the \"!notes_ref\" check was\nadded by the same commit that added the fallback: 709f79b0894 (Notes\nAPI: init_notes(): Initialize the notes tree from the given notes ref,\n2010-02-13). So this check never did anything.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n notes.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 45fb7f22d1..cadb435056 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1019,13 +1019,13 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n \tt->root = (struct int_node *) xcalloc(1, sizeof(struct int_node));\n \tt->first_non_note = NULL;\n \tt->prev_non_note = NULL;\n-\tt->ref = xstrdup_or_null(notes_ref);\n+\tt->ref = xstrdup(notes_ref);\n \tt->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;\n \tt->combine_notes = combine_notes;\n \tt->initialized = 1;\n \tt->dirty = 0;\n \n-\tif (flags & NOTES_INIT_EMPTY || !notes_ref ||\n+\tif (flags & NOTES_INIT_EMPTY ||\n \t    repo_get_oid_treeish(the_repository, notes_ref, &object_oid))\n \t\treturn;\n \tif (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, &object_oid))\n-- \n2.40.0.653.g15ca972062\n\n"},{"id":"475840","messageId":"20230422135455.GA3942740@coredump.intra.peff.net","threadId":"59634","inReplyTo":null,"subject":"[PATCH 0/2] coverity null-check fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-22T13:54:55Z","receivedAt":"2023-04-22T13:56:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are two cases I happened to notice via Coverity. Neither produces\na user-visible bug; it's just slightly misleading code which can be\ncleaned up.\n\nThe first one is ancient, but since the nearby code changed, Coverity\nconsidered it new. :)\n\nThe second one is actually recent; I noted it already on the list, but\nit was already in 'next' at that point and is now in 'master'.\n\n-Peff\n"},{"id":"475841","messageId":"20230422135646.GA3942917@coredump.intra.peff.net","threadId":"59634","inReplyTo":"20230422135455.GA3942740@coredump.intra.peff.net","subject":"[PATCH 2/2] fetch_bundle_uri(): drop pointless NULL check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-22T13:56:46Z","receivedAt":"2023-04-22T13:56:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We check if \"uri\" is NULL, but it cannot be since we'd have segfaulted\nearlier in the function when we unconditionally called xstrdup() on it.\n\nIn theory we might want to soften that xstrdup() to handle this case,\nbut even before the code which added it via c23f592117 (bundle-uri:\nfetch a list of bundles, 2022-10-12), we'd have fed NULL to\nfetch_bundle_uri_internal(), which would also segfault.\n\nThe extra check isn't hurting anything, but it does cause Coverity to\ncomplain, and it may mislead somebody reading the code into thinking\nthat a NULL uri is something we're prepared to handle.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n bundle-uri.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex e2b267cc02..f22490a2ca 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -795,10 +795,10 @@ int fetch_bundle_uri(struct repository *r, const char *uri,\n \tinit_bundle_list(&list);\n \n \t/*\n-\t * Do not fetch a NULL or empty bundle URI. An empty bundle URI\n+\t * Do not fetch an empty bundle URI. An empty bundle URI\n \t * could signal that a configured bundle URI has been disabled.\n \t */\n-\tif (!uri || !*uri) {\n+\tif (!*uri) {\n \t\tresult = 0;\n \t\tgoto cleanup;\n \t}\n-- \n2.40.0.653.g15ca972062\n"},{"id":"475954","messageId":"xmqqttx5duki.fsf@gitster.g","threadId":"59634","inReplyTo":"20230422135543.GA3942829@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] notes: clean up confusing NULL checks in init_notes()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-24T18:05:49Z","receivedAt":"2023-04-24T18:06:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Coverity complains that we check whether \"notes_ref\" is NULL, but it was\n> already implied to be non-NULL earlier in the function. And this is\n> true; since b9342b3fd63 (refs: add array of ref namespaces, 2022-08-05),\n> we call xstrdup(notes_ref) unconditionally, which would segfault if it\n> was NULL.\n>\n> But that commit is actually doing the right thing. Even if NULL is\n> passed into the function, we'll use default_notes_ref() as a fallback,\n> which will never return NULL (it tries a few options, but its last\n> resort is a string literal). Ironically, the \"!notes_ref\" check was\n> added by the same commit that added the fallback: 709f79b0894 (Notes\n> API: init_notes(): Initialize the notes tree from the given notes ref,\n> 2010-02-13). So this check never did anything.\n\nI am impressed(?) that Coverity can complain at the \"_or_null\" part\nin xstrdup_or_null().\n\nIt all makes sense.  Thanks.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  notes.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/notes.c b/notes.c\n> index 45fb7f22d1..cadb435056 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -1019,13 +1019,13 @@ void init_notes(struct notes_tree *t, const char *notes_ref,\n>  \tt->root = (struct int_node *) xcalloc(1, sizeof(struct int_node));\n>  \tt->first_non_note = NULL;\n>  \tt->prev_non_note = NULL;\n> -\tt->ref = xstrdup_or_null(notes_ref);\n> +\tt->ref = xstrdup(notes_ref);\n>  \tt->update_ref = (flags & NOTES_INIT_WRITABLE) ? t->ref : NULL;\n>  \tt->combine_notes = combine_notes;\n>  \tt->initialized = 1;\n>  \tt->dirty = 0;\n>  \n> -\tif (flags & NOTES_INIT_EMPTY || !notes_ref ||\n> +\tif (flags & NOTES_INIT_EMPTY ||\n>  \t    repo_get_oid_treeish(the_repository, notes_ref, &object_oid))\n>  \t\treturn;\n>  \tif (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, &object_oid))\n"},{"id":"475979","messageId":"20230424215719.GA3998354@coredump.intra.peff.net","threadId":"59634","inReplyTo":"xmqqttx5duki.fsf@gitster.g","subject":"Re: [PATCH 1/2] notes: clean up confusing NULL checks in init_notes()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-24T21:57:19Z","receivedAt":"2023-04-24T21:57:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 24, 2023 at 11:05:49AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Coverity complains that we check whether \"notes_ref\" is NULL, but it was\n> > already implied to be non-NULL earlier in the function. And this is\n> > true; since b9342b3fd63 (refs: add array of ref namespaces, 2022-08-05),\n> > we call xstrdup(notes_ref) unconditionally, which would segfault if it\n> > was NULL.\n> >\n> > But that commit is actually doing the right thing. Even if NULL is\n> > passed into the function, we'll use default_notes_ref() as a fallback,\n> > which will never return NULL (it tries a few options, but its last\n> > resort is a string literal). Ironically, the \"!notes_ref\" check was\n> > added by the same commit that added the fallback: 709f79b0894 (Notes\n> > API: init_notes(): Initialize the notes tree from the given notes ref,\n> > 2010-02-13). So this check never did anything.\n> \n> I am impressed(?) that Coverity can complain at the \"_or_null\" part\n> in xstrdup_or_null().\n\nNo, my human brain added that part while I was looking at the function.\n\nCoverity is definitely clever enough to realize that the NULL check in\nxstrdup_or_null() is not needed here (it's a static inline, but I think\nCoverity can even look between translation units). But complaining about\nit would yield lots of false positives. It's redundant in this instance,\nbut not in other callers of the function.\n\nSo it would have to realize: we called xstrdup_or_null(), but there is\nanother function xstrdup() which is exactly the same but without the\nNULL check. And I think that is asking too much of even a very clever\nstatic analyzer. :)\n\n-Peff\n"}]}