Junio C Hamano <gitster@xxxxxxxxx> writes:
> Jeff King <peff@xxxxxxxx> writes:
>
>> I'm not too familiar with the code, but this _seems_ to work for me:
>>
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index 21321be..2e03d47 100644
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -1375,6 +1375,10 @@ static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit
>> static int num_scapegoats(struct rev_info *revs, struct commit *commit)
>> {
>> struct commit_list *l = first_scapegoat(revs, commit);
>> + if (!l)
>> + return 0;
>> + if (revs->first_parent_only)
>> + return 1;
>> return commit_list_count(l);
>> }
>>
>> I suspect it doesn't work at all with `--reverse`. I also have the
>> nagging feeling that this could be handled inside revision.c with
>> parent-rewriting, but I don't really know.
>
> I am not sure how well --children, which is what we use from
> revision.c, works with --first-parent flag passed to the revision
> walking machinery. If it already does the right thing, then the
> revs.children decoration that collects all children of any given
> commit should automatically give its "sole child" (in the world
> limited to the first-parent chain) from first_scapegoat().
>
> And if it does not, perhaps that is what we would want to fix,
> i.e. making sure rev-list --first-parent --children does what
> people would expect.
>
>> But "git blame --first-parent <file>" seems to behave sanely in git.git
>> with this.
>
> It is intresting to see that the above is the only thing necessary,
> as a naive way to try all parents would be to do:
>
> for (sg = first_scapegoat(...); sg; sg = sg->next)
> assign blame to sg;
> take the remainder ourselves;
>
> in which case, a better place to patch would be first_scapegoat(),
> not this function, so that we will always see zero or one element
> parent list returned from the function.
>
> But in reality, the code instead does this:
>
> num_sg = num_scapegoats(...);
> for (i = 0, sg = first_scapegoat(...);
> i < num_sg && sg;
> i++, sg = sg->next)
> assign blame to sg;
> take the remainder ourselves;
>
> so you do not have to touch first_scapegoat() at all.
>
> I do not offhand know if this was a sign of foresight in the
> original, or just an overly redundant programming ;-).
So here is an outline of what I had in mind when I was writing the
above. Instead of trusting that num_scapegoats() is always used to
limit the enumeration of commit->parents, we discard the second and
subsequent parents from the commit objects, and also make sure the
later parents do not participate in the revs.children annotation, so
that the world "blame" sees truly becomes a single strand of pearls.
As usual, not even compile tested, but it feels correct ;-)
builtin/blame.c | 9 ++++++++-
revision.c | 4 +++-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..bc87d9d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1366,8 +1366,15 @@ static void pass_whole_blame(struct scoreboard *sb,
*/
static struct commit_list *first_scapegoat(struct rev_info *revs, struct commit *commit)
{
- if (!reverse)
+ if (!reverse) {
+ if (revs->first_parent_only &&
+ commit->parents &&
+ commit->parents->next) {
+ free_commit_list(commit->parents->next);
+ commit->parents->next = NULL;
+ }
return commit->parents;
+ }
return lookup_decoration(&revs->children, &commit->object);
}
diff --git a/revision.c b/revision.c
index af2a18e..a020a42 100644
--- a/revision.c
+++ b/revision.c
@@ -2765,7 +2765,9 @@ static void set_children(struct rev_info *revs)
struct commit *commit = l->item;
struct commit_list *p;
- for (p = commit->parents; p; p = p->next)
+ for (p = commit->parents;
+ p && !revs->first_parent_only;
+ p = p->next)
add_child(revs, p->item, commit);
}
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html