| Message ID | 20200423210303.GA1635761@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blame: drop unused parameter from maybe_changed_path | expand |
Jeff King <peff@peff.net> writes: > We don't use the "parent" parameter at all (probably because the bloom > filter for a commit is always defined against a single parent anyway). > > Signed-off-by: Jeff King <peff@peff.net> > --- > This is on top of ds/blame-on-bloom, which just made it to next. > > I _think_ this is the right solution, but perhaps the function should be > verifying that we're looking at the right parent? Hmph, "solution" to what problem? Ah, the fact that parent is an unused parameter? find_origin() runs a tree-diff over "parent" and "origin->commit", with literal pathspec limited to the single path. And the Bloom filter addition changed the code so that we first consult the filter when "origin->commit"'s first parent *is* "parent". Presumably, by asking maybe_changed_path about "origin", as "origin" knows what the commit is (i.e. "origin->commit") and what path we are talking about (i.e. "origin->path"), it can answer "does origin->commit change origin->path relative to its first parent?" and it can do so only for the first parent? The way I read bloom.c::get_bloom_filter(), it only computes a diff-tree between the given commit and its first parent (or an empty tree), so I think the above is correct. Thanks. > > blame.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/blame.c b/blame.c > index 9fbf79e47c..da7e28800e 100644 > --- a/blame.c > +++ b/blame.c > @@ -1263,7 +1263,6 @@ struct blame_bloom_data { > static int bloom_count_queries = 0; > static int bloom_count_no = 0; > static int maybe_changed_path(struct repository *r, > - struct commit *parent, > struct blame_origin *origin, > struct blame_bloom_data *bd) > { > @@ -1355,8 +1354,7 @@ static struct blame_origin *find_origin(struct repository *r, > if (origin->commit->parents && > !oidcmp(&parent->object.oid, > &origin->commit->parents->item->object.oid)) > - compute_diff = maybe_changed_path(r, parent, > - origin, bd); > + compute_diff = maybe_changed_path(r, origin, bd); > > if (compute_diff) > diff_tree_oid(get_commit_tree_oid(parent),
On Thu, Apr 23, 2020 at 02:36:53PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > We don't use the "parent" parameter at all (probably because the bloom > > filter for a commit is always defined against a single parent anyway). > > > > Signed-off-by: Jeff King <peff@peff.net> > > --- > > This is on top of ds/blame-on-bloom, which just made it to next. > > > > I _think_ this is the right solution, but perhaps the function should be > > verifying that we're looking at the right parent? > > Hmph, "solution" to what problem? Ah, the fact that parent is an > unused parameter? Yes, exactly. > find_origin() runs a tree-diff over "parent" and "origin->commit", > with literal pathspec limited to the single path. > > And the Bloom filter addition changed the code so that we first > consult the filter when "origin->commit"'s first parent *is* > "parent". Presumably, by asking maybe_changed_path about "origin", > as "origin" knows what the commit is (i.e. "origin->commit") and > what path we are talking about (i.e. "origin->path"), it can answer > "does origin->commit change origin->path relative to its first > parent?" and it can do so only for the first parent? > > The way I read bloom.c::get_bloom_filter(), it only computes a > diff-tree between the given commit and its first parent (or an empty > tree), so I think the above is correct. Yeah, the bloom filters are always against the first parent. I think I just got lost in this rather long oidcmp(), which is really just "is 'parent' the first parent?" if (origin->commit->parents && !oidcmp(&parent->object.oid, &origin->commit->parents->item->object.oid)) compute_diff = maybe_changed_path(r, origin, bd); If the bloom filter also computes against an empty tree for root commits (I didn't check, but that would make sense), I think that AND could be an OR: if (!origin->commit->parents || !oidcmp(...)) though it probably doesn't matter that much in practice. Root commits are rather rare. BTW, we could also be using oideq() here. I thought coccicheck would note this, but it doesn't seem to. I suspect we could also get away with a direct pointer comparison of "parent == origin->commit->parents->item" due to the way we allocate "struct commit", but I'd rather err on the safer and less subtle side. :) -Peff
Jeff King <peff@peff.net> writes: > If the bloom filter also computes against an empty tree for root > commits (I didn't check, but that would make sense), I think that AND > could be an OR: > > if (!origin->commit->parents || > !oidcmp(...)) > > though it probably doesn't matter that much in practice. Root commits > are rather rare. Correct. I just followed the code from bloom.c::get_bloom_filter() down, and for a root commit, diff_tree_oid() with NULL in the first parameter (i.e. old_oid) is called. This NULL pointer eventually reaches tree-walk.c::fill_tree_descriptor() and the function just gives an empty tree in that case, which is what we want. > > BTW, we could also be using oideq() here. I thought coccicheck would > note this, but it doesn't seem to. I suspect we could also get away with > a direct pointer comparison of "parent == origin->commit->parents->item" > due to the way we allocate "struct commit", but I'd rather err on the > safer and less subtle side. :) True. oideq() is probably an improvement; I agree that pointer equality is taking it too far.
diff --git a/blame.c b/blame.c index 9fbf79e47c..da7e28800e 100644 --- a/blame.c +++ b/blame.c @@ -1263,7 +1263,6 @@ struct blame_bloom_data { static int bloom_count_queries = 0; static int bloom_count_no = 0; static int maybe_changed_path(struct repository *r, - struct commit *parent, struct blame_origin *origin, struct blame_bloom_data *bd) { @@ -1355,8 +1354,7 @@ static struct blame_origin *find_origin(struct repository *r, if (origin->commit->parents && !oidcmp(&parent->object.oid, &origin->commit->parents->item->object.oid)) - compute_diff = maybe_changed_path(r, parent, - origin, bd); + compute_diff = maybe_changed_path(r, origin, bd); if (compute_diff) diff_tree_oid(get_commit_tree_oid(parent),
We don't use the "parent" parameter at all (probably because the bloom filter for a commit is always defined against a single parent anyway). Signed-off-by: Jeff King <peff@peff.net> --- This is on top of ds/blame-on-bloom, which just made it to next. I _think_ this is the right solution, but perhaps the function should be verifying that we're looking at the right parent? blame.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)