Re: [PATCH v6 4/6] blame: add config options to handle output for ignored lines
- From
- Barret Rhoden <brho@google.com>
- Date
- Apr 15, 2019, 13:51 UTC
- Message-ID
- <1a1b3cd1-5f00-37c9-7382-72de000dd925@google.com>
- In-Reply-To
- <CAJDYR9TRk99Kwq5S7udVqYsXnupGD=t3o_Ss8ewvwWuTQOy_YQ@mail.gmail.com>
Hi -
On 4/14/19 7:27 AM, Michael Platings wrote:
Show 44 quoted lines
> On Sun, 14 Apr 2019 at 11:24, Junio C Hamano <gitster@pobox.com> wrote: >>> If you only enable blame.markIgnoredLines then the hash for >>> "unblamable" lines appears as e.g. "*3252488f5" - this doesn't seem >>> right to me because the commit *wasn't* ignored, >> >> I think you misunderstood me. I was merely suggesting to use the >> approach to mark the line in a way other than using the NULLed out >> object name that has been reserved for something totally different, >> and hinting with "the same *idea*". > > Hi Junio, that paragraph wasn't targetted at yourself, more a comment > on the functionality as it exists in the latest patch series. Sorry > for not making that clear. > >> the "^" marker >> that is used to say "the line is attributed to this commit, but that >> may only be because you blamed with commit range A..B and we reached >> the bottom of the range---if you dug further, you might find the >> line originates from another commit" is the origin of the same idea, >> and this topic borrows it and uses a different mark, i.e. '*', for >> the "we are not certain---take this with grain of salt" mark. > > So it sounds like we have many types of blame to consider: > > 1) This commit is truly the last one to touch this line, and you > didn't ask to ignore it. > 2) This commit is truly the last one to touch this line, but you asked > to ignore it (AKA "unblamable"). > 3) This commit is at the bottom of the range of commits (^) > 4) The "true" commit was ignored but we guess this is the one you're > actually interested in (*) > 5) The "true" commit was ignored and we've reached the bottom of the > range of commits (^*)? > 6) This commit is at the bottom of the range of commits, and you asked > to ignore it. > >> If you ended up hitting the commit the user wanted to ignore, >> perhaps you can find another character that is different from '^' or >> '*' and use that, following the same idea. > > I personally don't find the "unblamable" lines interesting enough to > justify giving them a symbol. But if Barret strongly feels that such > lines should get a '*' then I won't fight it - these lines tend to be > as simple as "}".
I'm fine with not zeroing the hash, so long as there's some way to mark it.
We could mark with another *, such that if we mark-ignored and mark-unblamable you get "**hash". You can't have an unblamable that isn't from an ignored commit, so a single '*' has only one meaning, based on your config options.
If that works for you all, I can change this to markIgnoredUnblamables (instead of 'mask') in the next version.
Show 10 quoted lines
>> By the way, a configuration only feature is something we usually do >> not accept. A feature must be guarded with --command-line-option >> and then optionally can have a corresponding configuration once the >> option proves to be useful enough that it becomes useful to be able >> to say "in this repository (or to this user), the feature is on by >> default". > > In that case we definitely need a --mark-ignored-lines option to git > blame, and I would strongly prefer that we also keep the > blame.markIgnoredLines option as I for one will be switching it on.
I'd also keep this set. I think the whole reason for these config options was that everyone has a different preference, but that preference rarely changes. I don't want to have to type --mark-ignored-lines every time I run git blame. If I had to, I'd have to alias git blame or something.
I think having config options for these sorts of things is fine, since we know already that for a given user+repo, we want the feature on (or off). But if I have to remove it, then let me know.
Thanks,
Barret