{"thread":{"id":"53301","subject":"[PATCH] blame: drop unused parameter from maybe_changed_path","startedAt":"2020-04-23T21:03:06Z","lastAt":"2020-04-24T20:19:02Z","messageCount":4,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396039","messageId":"20200423210303.GA1635761@coredump.intra.peff.net","threadId":"53301","inReplyTo":null,"subject":"[PATCH] blame: drop unused parameter from maybe_changed_path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-23T21:03:03Z","receivedAt":"2020-04-23T21:03:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't use the \"parent\" parameter at all (probably because the bloom\nfilter for a commit is always defined against a single parent anyway).\n\nSigned-off-by: Jeff King \n---\nThis is on top of ds/blame-on-bloom, which just made it to next.\n\nI _think_ this is the right solution, but perhaps the function should be\nverifying that we're looking at the right parent?\n\n blame.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/blame.c b/blame.c\nindex 9fbf79e47c..da7e28800e 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -1263,7 +1263,6 @@ struct blame_bloom_data {\n static int bloom_count_queries = 0;\n static int bloom_count_no = 0;\n static int maybe_changed_path(struct repository *r,\n-\t\t\t struct commit *parent,\n \t\t\t struct blame_origin *origin,\n \t\t\t struct blame_bloom_data *bd)\n {\n@@ -1355,8 +1354,7 @@ static struct blame_origin *find_origin(struct repository *r,\n \t\tif (origin->commit->parents &&\n \t\t !oidcmp(&parent->object.oid,\n \t\t\t &origin->commit->parents->item->object.oid))\n-\t\t\tcompute_diff = maybe_changed_path(r, parent,\n-\t\t\t\t\t\t\t origin, bd);\n+\t\t\tcompute_diff = maybe_changed_path(r, origin, bd);\n \n \t\tif (compute_diff)\n \t\t\tdiff_tree_oid(get_commit_tree_oid(parent),\n-- \n2.26.2.827.g3c1233342b\n"},{"id":"396043","messageId":"xmqqk125g9ze.fsf@gitster.c.googlers.com","threadId":"53301","inReplyTo":"20200423210303.GA1635761@coredump.intra.peff.net","subject":"Re: [PATCH] blame: drop unused parameter from maybe_changed_path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-23T21:36:53Z","receivedAt":"2020-04-23T21:37:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We don't use the \"parent\" parameter at all (probably because the bloom\n> filter for a commit is always defined against a single parent anyway).\n>\n> Signed-off-by: Jeff King \n> ---\n> This is on top of ds/blame-on-bloom, which just made it to next.\n>\n> I _think_ this is the right solution, but perhaps the function should be\n> verifying that we're looking at the right parent?\n\nHmph, \"solution\" to what problem? Ah, the fact that parent is an\nunused parameter?\n\nfind_origin() runs a tree-diff over \"parent\" and \"origin->commit\",\nwith literal pathspec limited to the single path.\n\nAnd the Bloom filter addition changed the code so that we first\nconsult the filter when \"origin->commit\"'s first parent *is*\n\"parent\". Presumably, by asking maybe_changed_path about \"origin\",\nas \"origin\" knows what the commit is (i.e. \"origin->commit\") and\nwhat path we are talking about (i.e. \"origin->path\"), it can answer\n\"does origin->commit change origin->path relative to its first\nparent?\" and it can do so only for the first parent?\n\nThe way I read bloom.c::get_bloom_filter(), it only computes a\ndiff-tree between the given commit and its first parent (or an empty\ntree), so I think the above is correct.\n\nThanks.\n\n>\n> blame.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/blame.c b/blame.c\n> index 9fbf79e47c..da7e28800e 100644\n> --- a/blame.c\n> +++ b/blame.c\n> @@ -1263,7 +1263,6 @@ struct blame_bloom_data {\n> static int bloom_count_queries = 0;\n> static int bloom_count_no = 0;\n> static int maybe_changed_path(struct repository *r,\n> -\t\t\t struct commit *parent,\n> \t\t\t struct blame_origin *origin,\n> \t\t\t struct blame_bloom_data *bd)\n> {\n> @@ -1355,8 +1354,7 @@ static struct blame_origin *find_origin(struct repository *r,\n> \t\tif (origin->commit->parents &&\n> \t\t !oidcmp(&parent->object.oid,\n> \t\t\t &origin->commit->parents->item->object.oid))\n> -\t\t\tcompute_diff = maybe_changed_path(r, parent,\n> -\t\t\t\t\t\t\t origin, bd);\n> +\t\t\tcompute_diff = maybe_changed_path(r, origin, bd);\n> \n> \t\tif (compute_diff)\n> \t\t\tdiff_tree_oid(get_commit_tree_oid(parent),\n"},{"id":"396103","messageId":"20200424043225.GB1648190@coredump.intra.peff.net","threadId":"53301","inReplyTo":"xmqqk125g9ze.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] blame: drop unused parameter from maybe_changed_path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-24T04:32:25Z","receivedAt":"2020-04-24T04:32:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 23, 2020 at 02:36:53PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > We don't use the \"parent\" parameter at all (probably because the bloom\n> > filter for a commit is always defined against a single parent anyway).\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > This is on top of ds/blame-on-bloom, which just made it to next.\n> >\n> > I _think_ this is the right solution, but perhaps the function should be\n> > verifying that we're looking at the right parent?\n> \n> Hmph, \"solution\" to what problem? Ah, the fact that parent is an\n> unused parameter?\n\nYes, exactly.\n\n> find_origin() runs a tree-diff over \"parent\" and \"origin->commit\",\n> with literal pathspec limited to the single path.\n> \n> And the Bloom filter addition changed the code so that we first\n> consult the filter when \"origin->commit\"'s first parent *is*\n> \"parent\". Presumably, by asking maybe_changed_path about \"origin\",\n> as \"origin\" knows what the commit is (i.e. \"origin->commit\") and\n> what path we are talking about (i.e. \"origin->path\"), it can answer\n> \"does origin->commit change origin->path relative to its first\n> parent?\" and it can do so only for the first parent?\n> \n> The way I read bloom.c::get_bloom_filter(), it only computes a\n> diff-tree between the given commit and its first parent (or an empty\n> tree), so I think the above is correct.\n\nYeah, the bloom filters are always against the first parent. I think I\njust got lost in this rather long oidcmp(), which is really just \"is\n'parent' the first parent?\"\n\n\tif (origin->commit->parents &&\n\t !oidcmp(&parent->object.oid,\n\t\t &origin->commit->parents->item->object.oid))\n\t\tcompute_diff = maybe_changed_path(r, origin, bd);\n\nIf the bloom filter also computes against an empty tree for root\ncommits (I didn't check, but that would make sense), I think that AND\ncould be an OR:\n\n if (!origin->commit->parents ||\n !oidcmp(...))\n\nthough it probably doesn't matter that much in practice. Root commits\nare rather rare.\n\nBTW, we could also be using oideq() here. I thought coccicheck would\nnote this, but it doesn't seem to. I suspect we could also get away with\na direct pointer comparison of \"parent == origin->commit->parents->item\"\ndue to the way we allocate \"struct commit\", but I'd rather err on the\nsafer and less subtle side. :)\n\n-Peff\n"},{"id":"396173","messageId":"xmqqimhod4cu.fsf@gitster.c.googlers.com","threadId":"53301","inReplyTo":"20200424043225.GB1648190@coredump.intra.peff.net","subject":"Re: [PATCH] blame: drop unused parameter from maybe_changed_path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T20:18:57Z","receivedAt":"2020-04-24T20:19:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If the bloom filter also computes against an empty tree for root\n> commits (I didn't check, but that would make sense), I think that AND\n> could be an OR:\n>\n> if (!origin->commit->parents ||\n> !oidcmp(...))\n>\n> though it probably doesn't matter that much in practice. Root commits\n> are rather rare.\n\nCorrect. I just followed the code from bloom.c::get_bloom_filter()\ndown, and for a root commit, diff_tree_oid() with NULL in the first\nparameter (i.e. old_oid) is called. This NULL pointer eventually\nreaches tree-walk.c::fill_tree_descriptor() and the function just\ngives an empty tree in that case, which is what we want.\n>\n> BTW, we could also be using oideq() here. I thought coccicheck would\n> note this, but it doesn't seem to. I suspect we could also get away with\n> a direct pointer comparison of \"parent == origin->commit->parents->item\"\n> due to the way we allocate \"struct commit\", but I'd rather err on the\n> safer and less subtle side. :)\n\nTrue. oideq() is probably an improvement; I agree that pointer\nequality is taking it too far.\n"}]}