# Git blame only current branch 10 messages from 2011-12-12 to 2011-12-13. Participants: Stephen Bash, Jeff King, andreas.t.auer_gtml_37453@ursus.ath.cx, Vijay Lakshminarayanan, Junio C Hamano, Frans Klaver. Thread: https://gitlist.dev/t/29139 ## Stephen Bash, 2011-12-12 15:24 Subject: Git blame only current branch Message-ID: URL: https://gitlist.dev/e/d615954f-bed8-482d-a2e3-e1e741d6dd23%40mail In-Reply-To: ``` 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, 2011-12-12 16:55 Subject: Re: Git blame only current branch Message-ID: <20111212165542.GA4802@sigill.intra.peff.net> URL: https://gitlist.dev/e/20111212165542.GA4802%40sigill.intra.peff.net In-Reply-To: ``` 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, 2011-12-12 17:05 Subject: Re: Git blame only current branch Message-ID: <5e2440c1-8d11-4d92-b42f-14169a62ced1@mail> URL: https://gitlist.dev/e/5e2440c1-8d11-4d92-b42f-14169a62ced1%40mail In-Reply-To: <20111212165542.GA4802@sigill.intra.peff.net> ``` ----- Original Message ----- > From: "Jeff King" > 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, 2011-12-12 17:19 Subject: Re: Git blame only current branch Message-ID: <4EE63799.6020409@ursus.ath.cx> URL: https://gitlist.dev/e/4EE63799.6020409%40ursus.ath.cx In-Reply-To: <5e2440c1-8d11-4d92-b42f-14169a62ced1@mail> ``` On 12.12.2011 18:05 Stephen Bash wrote: > ----- Original Message ----- > > From: "Jeff King" 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, 2011-12-13 02:07 Subject: Re: Git blame only current branch Message-ID: <8739cpteat.fsf@gmail.com> URL: https://gitlist.dev/e/8739cpteat.fsf%40gmail.com In-Reply-To: <20111212165542.GA4802@sigill.intra.peff.net> ``` Jeff King 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. ``` ## Jeff King, 2011-12-13 02:14 Subject: Re: Git blame only current branch Message-ID: <20111213021442.GA4244@sigill.intra.peff.net> URL: https://gitlist.dev/e/20111213021442.GA4244%40sigill.intra.peff.net In-Reply-To: <8739cpteat.fsf@gmail.com> ``` 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 ``` ## Junio C Hamano, 2011-12-13 05:47 Subject: Re: Git blame only current branch Message-ID: <7vobvdvx9c.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vobvdvx9c.fsf%40alter.siamese.dyndns.org In-Reply-To: <8739cpteat.fsf@gmail.com> ``` Vijay Lakshminarayanan 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. ``` ## Vijay Lakshminarayanan, 2011-12-13 14:09 Subject: Re: Git blame only current branch Message-ID: <87y5ugsguj.fsf@gmail.com> URL: https://gitlist.dev/e/87y5ugsguj.fsf%40gmail.com In-Reply-To: <7vobvdvx9c.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > Vijay Lakshminarayanan 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. ``` ## Frans Klaver, 2011-12-13 14:18 Subject: Re: Git blame only current branch Message-ID: URL: https://gitlist.dev/e/op.v6fl05u30aolir%40keputer In-Reply-To: <87y5ugsguj.fsf@gmail.com> ``` On Tue, 13 Dec 2011 15:09:56 +0100, Vijay Lakshminarayanan wrote: > Junio C Hamano writes: > >> Vijay Lakshminarayanan 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, 2011-12-13 17:25 Subject: Re: Git blame only current branch Message-ID: <7vd3bswfhz.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd3bswfhz.fsf%40alter.siamese.dyndns.org In-Reply-To: <87y5ugsguj.fsf@gmail.com> ``` Vijay Lakshminarayanan 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 ''" and "git grep -e ''". ```