Git blame only current branch

Git blame only current branch

[Stephen Bash]

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

Re: Git blame only current branch

[Jeff King]

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

Re: Git blame only current branch

[Stephen Bash]

----- 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:
> 
> > 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

Re: Git blame only current branch

[andreas.t.auer_gtml_37453]

On 12.12.2011 18:05 Stephen Bash wrote:
>  ----- 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.

Re: Git blame only current branch

[Vijay Lakshminarayanan]

Jeff King <peff@peff.net> writes:

[snip]

> 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.

Re: Git blame only current branch

[Jeff King]

On Tue, Dec 13, 2011 at 07:37:22AM +0530, Vijay Lakshminarayanan wrote:

> > 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

Re: Git blame only current branch

[Junio C Hamano]

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.

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.

Re: Git blame only current branch

[Vijay Lakshminarayanan]

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 ;-)

> 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.

Re: Git blame only current branch

[Frans Klaver]

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

> 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.

Re: Git blame only current branch

[Junio C Hamano]

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>'".