threads / discuss / 29139

Git blame only current branch

Subject: Git blame only current branch

## tl;dr

10 messages between Dec 12, 2011 and Dec 13, 2011.

replies: 9people: 6as markdown or json

Stephen Bash· Dec 12, 2011, 15:24 UTC · lore
Hi all-
I'm curious if there's a method to make git blame merge commits that introduce code to the given branch rather than commits on the original (topic) branch?  For example:
  A--B--C---M--D  master
   \       /
    1--2--3       topicA
I'd like a mode where 'git blame master ...' shows commit M for lines changed by topicA so I can easily do 'git blame M^ ...' to see changes (on master) prior to the merge of topicA.  Unfortunately 'git blame master ^topicA ...' blames all the changes of topicA to 3 (which reading the docs appears to be the "correct" behavior).
Thanks!
Stephen
Jeff King· Dec 12, 2011, 16:55 UTC · re: Stephen Bash · lore

Re: Git blame only current branch

On Mon, Dec 12, 2011 at 10:24:47AM -0500, Stephen Bash wrote:
> I'm curious if there's a method to make git blame merge commits that
> introduce code to the given branch rather than commits on the original
> (topic) branch?  For example:

Usually when you are interested in seeing merges like this in git-log, you would use one of "--first-parent" or "--merges". However, though "git blame" takes revision arguments, it does its own traversal of the graph that does not respect those options.

Modifying it to do --first-parent is pretty easy:
diff --git a/builtin/blame.c b/builtin/blame.c
index 80febbe..c19a8cd 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1191,6 +1191,8 @@ static int num_scapegoats(struct rev_info *revs, struct commit *commit)
 {
 	int cnt;
 	struct commit_list *l = first_scapegoat(revs, commit);
+	if (revs->first_parent_only)
+		return l ? 1 : 0;
 	for (cnt = 0; l; l = l->next)
 		cnt++;
 	return cnt;

With that, "git blame --first-parent" produces reasonable results for
me. But of course I didn't do more than 30 seconds of testing, so it is
entirely possible there are corner cases or unforeseen side effects.

Handling --merges is probably a little trickier, as you need to consider
only some commits as scapegoats, but still traverse through everything
to find the merges.

-Peff
Stephen Bash· Dec 12, 2011, 17:05 UTC · re: Jeff King · lore

Re: Git blame only current branch

----- Original Message -----
Show 15 quoted lines
> From: "Jeff King" <peff@peff.net>
> Sent: Monday, December 12, 2011 11:55:42 AM
> Subject: Re: Git blame only current branch
> 
> On Mon, Dec 12, 2011 at 10:24:47AM -0500, Stephen Bash wrote:
> 
> > I'm curious if there's a method to make git blame merge commits
> > that introduce code to the given branch rather than commits on 
> > the original (topic) branch?  For example:
> 
> Usually when you are interested in seeing merges like this in
> git-log, you would use one of "--first-parent" or "--merges". 
> However, though "git blame" takes revision arguments, it does
> its own traversal of the graph that does not respect those 
> options.
My first thought was --first-parent, and was disappointed when I didn't find it in the blame documentation :)  I think for my purposes --first-parent is better than --merges because there are non-merge commits on the branch(es) of interest (and thus I think the problem would become ill-posed in the --merges case).
> Modifying it to do --first-parent is pretty easy:
> ... snip ...
That's pretty simple...  I'll try to do a little testing this afternoon.

Thanks! Stephen

andreas.t.auer_gtml_37453@ursus.ath.cx· Dec 12, 2011, 17:19 UTC · re: Stephen Bash · lore

Re: Git blame only current branch

On 12.12.2011 18:05 Stephen Bash wrote:
Show 21 quoted lines
>  ----- Original Message -----
> > From: "Jeff King" <peff@peff.net> Sent: Monday, December 12, 2011
> > 11:55:42 AM Subject: Re: Git blame only current branch
> >
> > On Mon, Dec 12, 2011 at 10:24:47AM -0500, Stephen Bash wrote:
> >
> > Usually when you are interested in seeing merges like this in
> > git-log, you would use one of "--first-parent" or "--merges".
> > However, though "git blame" takes revision arguments, it does its
> > own traversal of the graph that does not respect those options.
>
>  My first thought was --first-parent, and was disappointed when I
>  didn't find it in the blame documentation :)  I think for my purposes
>  --first-parent is better than --merges because there are non-merge
>  commits on the branch(es) of interest (and thus I think the problem
>  would become ill-posed in the --merges case).
>
> > Modifying it to do --first-parent is pretty easy: ... snip ...
>
>  That's pretty simple...  I'll try to do a little testing this
>  afternoon.

You might need to consider that if the master branch was first merged into topicA before topicA was merged back to the master that the master would only be fast-forwarded and so the first parent of M would be 3 not C. So depending how the developers merged you might get different results.

Vijay Lakshminarayanan· Dec 13, 2011, 02:07 UTC · re: Jeff King · lore

Re: Git blame only current branch

Jeff King <peff@peff.net> writes:
[snip]
Show 13 quoted lines
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 80febbe..c19a8cd 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1191,6 +1191,8 @@ static int num_scapegoats(struct rev_info *revs, struct commit *commit)
>  {
>  	int cnt;
>  	struct commit_list *l = first_scapegoat(revs, commit);
> +	if (revs->first_parent_only)
> +		return l ? 1 : 0;
>  	for (cnt = 0; l; l = l->next)
>  		cnt++;
>  	return cnt;
I just spent 30s staring at this wondering why you needed to do 
    return 1 ? 1 : 0;
which always returns 1 anyway before I realized it was a lowercase L.

The code reads fine when there's no numeral 1 around but now it doesn't read well. I think refactoring

    struct commit_list *l
to 
    struct commit_list *lst
is justified.  Thoughts?
> -Peff
-- 
Cheers
~vijay

Gnus should be more complicated.
Jeff King· Dec 13, 2011, 02:14 UTC · re: Vijay Lakshminarayanan · lore

Re: Git blame only current branch

On Tue, Dec 13, 2011 at 07:37:22AM +0530, Vijay Lakshminarayanan wrote:
Show 30 quoted lines
> > diff --git a/builtin/blame.c b/builtin/blame.c
> > index 80febbe..c19a8cd 100644
> > --- a/builtin/blame.c
> > +++ b/builtin/blame.c
> > @@ -1191,6 +1191,8 @@ static int num_scapegoats(struct rev_info *revs, struct commit *commit)
> >  {
> >  	int cnt;
> >  	struct commit_list *l = first_scapegoat(revs, commit);
> > +	if (revs->first_parent_only)
> > +		return l ? 1 : 0;
> >  	for (cnt = 0; l; l = l->next)
> >  		cnt++;
> >  	return cnt;
> 
> I just spent 30s staring at this wondering why you needed to do 
> 
>     return 1 ? 1 : 0;
> 
> which always returns 1 anyway before I realized it was a lowercase L.
> 
> The code reads fine when there's no numeral 1 around but now it doesn't
> read well.  I think refactoring
> 
>     struct commit_list *l
> 
> to 
> 
>     struct commit_list *lst
> 
> is justified.  Thoughts?

Sure, that would help. I wasn't planning to push this forward as a "real" patch, but if somebody wants to do some testing and, more importantly read through the code to make sure I am not violating some assumptions, then it might be worth including upstream.

-Peff
Junio C Hamano· Dec 13, 2011, 05:47 UTC · re: Vijay Lakshminarayanan · lore

Re: Git blame only current branch

Vijay Lakshminarayanan <laksvij@gmail.com> writes:
Show 10 quoted lines
> The code reads fine when there's no numeral 1 around but now it doesn't
> read well.  I think refactoring
>
>     struct commit_list *l
>
> to 
>
>     struct commit_list *lst
>
> is justified.  Thoughts?
Not justified at all.

What is "lst" and why is it not spelled "list"? It is a disease to drop vowels when you do not have to.

If I were to name a new variable that points at one element of a linked list and is used to walk the list (surprise!) "element" or perhaps "elem" for short, but in the context of that short function I honestly do not see much need for such a naming. The variable is extremely short-lived and there is no room for confusion.

Vijay Lakshminarayanan· Dec 13, 2011, 14:09 UTC · re: Junio C Hamano · lore

Re: Git blame only current branch

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Vijay Lakshminarayanan <laksvij@gmail.com> writes:
>
>> The code reads fine when there's no numeral 1 around but now it doesn't
>> read well.  I think refactoring
>>
>>     struct commit_list *l
>>
>> to 
>>
>>     struct commit_list *lst
>>
>> is justified.  Thoughts?
>
> Not justified at all.
>
> What is "lst" and why is it not spelled "list"?  It is a disease to drop
> vowels when you do not have to.

lst is better than l in this particular context. I think fried_chicken is better than l in this particular context ;-)

Show 5 quoted lines
> If I were to name a new variable that points at one element of a linked
> list and is used to walk the list (surprise!) "element" or perhaps "elem"
> for short, but in the context of that short function I honestly do not see
> much need for such a naming. The variable is extremely short-lived and
> there is no room for confusion.

Before the introduction of the numeral 1, I am in complete agreement with you for the exact reasons you've mentioned above. Post introduction of "l ? 1 : 0" it warrants a refactoring. It's possible you're using a different font so you never encounter the issue, but this definitely isn't a problem I alone face. For instance, it is a sufficiently common problem that it's one of the "Java Puzzlers" in Josh Bloch's book of the same name. (Yes, elem is better than lst.)

My $0.02.
-- 
Cheers
~vijay

Gnus should be more complicated.
Frans Klaver· Dec 13, 2011, 14:18 UTC · re: Vijay Lakshminarayanan · lore

Re: Git blame only current branch

On Tue, 13 Dec 2011 15:09:56 +0100, Vijay Lakshminarayanan <laksvij@gmail.com> wrote:

Show 22 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Vijay Lakshminarayanan <laksvij@gmail.com> writes:
>>
>>> The code reads fine when there's no numeral 1 around but now it doesn't
>>> read well.  I think refactoring
>>>
>>>     struct commit_list *l
>>>
>>> to
>>>
>>>     struct commit_list *lst
>>>
>>> is justified.  Thoughts?
>>
>> Not justified at all.
>>
>> What is "lst" and why is it not spelled "list"?  It is a disease to drop
>> vowels when you do not have to.
>
> lst is better than l in this particular context.  I think fried_chicken
> is better than l in this particular context ;-)

I tend to agree. If you casually look over the code it may look odd, and with several monospace fonts there really isn't a very big difference between 1?1:0 and l?1:0. You shouldn't have to squint to properly see the intention of the code.

If there's going to be a rename, there is no reason to leave out the i though.

Junio C Hamano· Dec 13, 2011, 17:25 UTC · re: Vijay Lakshminarayanan · lore

Re: Git blame only current branch

Vijay Lakshminarayanan <laksvij@gmail.com> writes:
> Before the introduction of the numeral 1, I am in complete agreement
> with you for the exact reasons you've mentioned above.  Post
> introduction of "l ? 1 : 0" it warrants a refactoring.

If your main point is that "return l ? 1 : 0;", then a better thing to do would be to use a well-known idiom to turn anything into a boolean, i.e.

	return !!l;

and your problem is solved without any renaming (we are not talking about any "refactoring" that changes code structure).

I've seen enough bikeshedding, so I'd stop after pointing you in the right direction by mentioning "git grep -e '<lst>'" and "git grep -e '<elem>'".

← back to recent threads