Re: [PATCH v6 4/6] blame: add config options to handle output for ignored lines

[Barret Rhoden <brho@google.com>]

Hi -

On 4/14/19 7:27 AM, Michael Platings wrote:
> 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.

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