* [PATCH] blame: drop unused parameter from maybe_changed_path
@ 2020-04-23 21:03 Jeff King
2020-04-23 21:36 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2020-04-23 21:03 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee
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(-)
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),
--
2.26.2.827.g3c1233342b
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] blame: drop unused parameter from maybe_changed_path
2020-04-23 21:03 [PATCH] blame: drop unused parameter from maybe_changed_path Jeff King
@ 2020-04-23 21:36 ` Junio C Hamano
2020-04-24 4:32 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2020-04-23 21:36 UTC (permalink / raw)
To: Jeff King; +Cc: git, Derrick Stolee
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),
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] blame: drop unused parameter from maybe_changed_path
2020-04-23 21:36 ` Junio C Hamano
@ 2020-04-24 4:32 ` Jeff King
2020-04-24 20:18 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2020-04-24 4:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] blame: drop unused parameter from maybe_changed_path
2020-04-24 4:32 ` Jeff King
@ 2020-04-24 20:18 ` Junio C Hamano
0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2020-04-24 20:18 UTC (permalink / raw)
To: Jeff King; +Cc: git, Derrick Stolee
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-24 20:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 21:03 [PATCH] blame: drop unused parameter from maybe_changed_path Jeff King
2020-04-23 21:36 ` Junio C Hamano
2020-04-24 4:32 ` Jeff King
2020-04-24 20:18 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).