{"thread":{"id":"39734","subject":"[PATCH] revision.c: Remove unneeded check for NULL","startedAt":"2015-06-26T19:40:19Z","lastAt":"2015-06-30T22:48:14Z","messageCount":3,"participants":["Stefan Beller","Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"264988","messageId":"1435347619-29410-1-git-send-email-sbeller@google.com","threadId":"39734","inReplyTo":null,"subject":"[PATCH] revision.c: Remove unneeded check for NULL","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-26T19:40:19Z","receivedAt":"2015-06-26T19:40:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The function is called only from one place, which makes sure\nto have `interesting_cache` not NULL. Additionally it is a\ndereferenced a few lines before unconditionally, which would\nresult in a segmentation fault.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    > So I think the right solution is just to drop the conditional entirely.\n    > The current code is not wrong (it is always a noop). What you have here\n    > actually misbehaves; it does not update the cache slot when it has\n    > become UNINTERESTING. That does not produce wrong results, but it loses\n    > the benefit of the cache in some cases.\n    \n    After reading the code a bit more, I agree.\n    \n    > I'm having trouble parsing this sentence.  Do you mean that limit_list()\n    > only calls still_interesting() (and thus, indirectly,\n    > everybody_uninteresting()), with the second parameter equal to the\n    > address of the local interesting_cache variable, so it can never be\n    > NULL?\n    \n    I completely reworded the commit message.\n    \n    > Should there be\n    >\n    >        if (!interesting_cache)\n    >                die(\"BUG: &interesting_cache == NULL\");\n    >\n    > checks at the top of still_interesting and everybody_uninteresting to\n    > futureproof this?\n    \n    I don't think this is necessary as these functions are local functions\n    so when somebody wants to use them they will be aware of the limitations.\n    \n    > This code seems to be underdocumented.\n    \n    I am not a expert in this area of the code, so I hoped Peff\n    would document it if he feels like so.\n\n revision.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 3ff8723..ab97ffd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -361,8 +361,8 @@ static int everybody_uninteresting(struct commit_list *orig,\n \t\tlist = list->next;\n \t\tif (commit->object.flags & UNINTERESTING)\n \t\t\tcontinue;\n-\t\tif (interesting_cache)\n-\t\t\t*interesting_cache = commit;\n+\n+\t\t*interesting_cache = commit;\n \t\treturn 0;\n \t}\n \treturn 1;\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"265024","messageId":"20150627060710.GA9353@peff.net","threadId":"39734","inReplyTo":"1435347619-29410-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] revision.c: Remove unneeded check for NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-27T06:07:10Z","receivedAt":"2015-06-27T06:07:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 26, 2015 at 12:40:19PM -0700, Stefan Beller wrote:\n\n> The function is called only from one place, which makes sure\n> to have `interesting_cache` not NULL. Additionally it is a\n> dereferenced a few lines before unconditionally, which would\n> result in a segmentation fault.\n\nYeah, I think this is the right thing to do.\n\nAcked-by: Jeff King <peff@peff.net>\n\n>     > Should there be\n>     >\n>     >        if (!interesting_cache)\n>     >                die(\"BUG: &interesting_cache == NULL\");\n>     >\n>     > checks at the top of still_interesting and everybody_uninteresting to\n>     > futureproof this?\n>     \n>     I don't think this is necessary as these functions are local functions\n>     so when somebody wants to use them they will be aware of the limitations.\n\nAgreed. We do not assert non-NULL for every parameter in our functions,\nand I do not think this is any more special than the others.\n\n>     > This code seems to be underdocumented.\n>     \n>     I am not a expert in this area of the code, so I hoped Peff\n>     would document it if he feels like so.\n\nI kind of thought that the explanation in b6e8a3b covered this code.\nDoes it not, or did people not read it?\n\nThe whole of revision.c is not well documented by comments, but I don't\nthink that is a new thing.\n\n-Peff\n"},{"id":"265279","messageId":"20150630224814.GB4865@google.com","threadId":"39734","inReplyTo":"20150627060710.GA9353@peff.net","subject":"Re: [PATCH] revision.c: Remove unneeded check for NULL","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-06-30T22:48:14Z","receivedAt":"2015-06-30T22:48:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Fri, Jun 26, 2015 at 12:40:19PM -0700, Stefan Beller wrote:\n\n>>> This code seems to be underdocumented.\n>>\n>> I am not a expert in this area of the code, so I hoped Peff\n>> would document it if he feels like so.\n>\n> I kind of thought that the explanation in b6e8a3b covered this code.\n> Does it not, or did people not read it?\n\nI know that \"tl;dr\" is the last thing anyone who has written a clear\ndescription of something wants to read, but I fear it applies here.  I\ntried to skim that commit message to get a gist of what the\nstill_interesting variable is supposed to hold and I failed.\n\nI think part of the problem was that that commit message doesn't give\na specific example early on to motivate the problem and fix[*].\n\nMore to the point, someone interested in that specific variable\ndoesn't need to necessarily understand the optimization that motivated\nit.  Instead, they'd want to know what invariants to expect and\npreserve: what value does it start with, what does its value mean, are\nthere some forbidden values, etc.\n\nIs the idea that it represents a commit from the queue which is still\ninteresting, and that it saves us from looping through the queue to\nfind a still-interesting commit as long as mark_parents_uninteresting\nhas not marked this one uninteresting yet?  What does it mean when it\nis NULL?\n\nThanks,\nJonathan\n\n[*] I.e., what command do I run to get quadratic behavior?\n\nThe message starts with a diagnosis --- \"When we are limiting a\nrev-list traversal due to UNINTERESTING refs, we have to walk down the\ntips\" --- without introducing what problem is being diganosed.\n\nThe problem being solved might have been something like \"When we call\n'git rev-list $commits --not --all' in check_everything_connected\nafter a fetch, if we fetched something much older than most of our\nrefs, and if we have a large number of refs, the operation is slow ---\nquadratic in the number of refs.  This hasn't been a problem in the\npast because people did not use so many refs, but now as the number of\nrefs in a typical repository grows, it is becoming more noticeable.\"\nEven after re-reading the message more carefully, I'm not sure.  I\nassume there was a report motivating the change, which might have been\nuseful for putting the explanation in context for the reader.\nAlas, git://repo.or.cz/git/trast.git branch notes/gmane doesn't have any\nannotations for that commit to find the context.\n\nThe commit message then goes on to explain how the patch solves that\nproblem, but without an example to put that explanation in context, it\nis hard to follow.  What linear search is the explanation talking\nabout?  What is the interesting commit we find?  I couldn't tell without\nlooking at the code.\n"}]}