Re: [PATCH] blame: add the ability to ignore commits

From: Junio C Hamano <gitster@pobox.com>
To: Barret Rhoden <brho@google.com>
Cc: git@vger.kernel.org, Stefan Beller <stefanbeller@gmail.com>,
	Jeff Smith <whydoubt@gmail.com>, Jeff King <peff@peff.net>
Subject: Re: [PATCH] blame: add the ability to ignore commits
Date: Tue, 08 Jan 2019 10:26:26 -0800	[thread overview]
Message-ID: <xmqqimyz57l9.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190108112742.7878d4cb@gnomeregan.cam.corp.google.com> (Barret Rhoden's message of "Tue, 8 Jan 2019 11:27:42 -0500")

Barret Rhoden <brho@google.com> writes:

>> A policy decision like the above two shouldn't be hardcoded in the
>> feature like this, but should be done as a separate option.  By
>> default, these shouldn't be marked with '*', as the same tools you
>> said you are afraid of breaking would be expecting a word with only
>> digits and no asterisk in the column where line numbers appear and
>> will get broken by this change if done unconditionally.
>
> Since users are already opting-in to the blame-ignore, do you also want
> them to opt-in to the annotation?

Absolutely.

After all, the users of a moral equivalent that is -S
never needed such an extra annotation, which tells us two things.
(1) the claim "It's useful to be alerted to the presence of an
ignored commit" in the proposed log message is merely a personal
preference and universal users' requirement; (2) if it is useful to
mark a blame-source whose parents (used while blaming) do not match
the actual parents, such an annotation would also be useful while
running -S.  So probably it should be a separate option that can be
given when any of the --skip-commit=<rev>, --skip-commits-file=<file>,
r -S<file> option is given.

>> Obviously, an interesting consideration is what happens when a merge
>> commit is skipped.  Is it sufficient to change this description to
>> "...will get passed to its parentS", or would the code do completely
>> nonsensical things without further tweaks (a possible simple tweak
>> could be to refuse skipping merges)?
>
> If we skip a merge commit, it might pick the wrong parent.

Actually after thinking about it a bit more, I no longer think a
merge is so special.  In this topology (as usual, time flows from
left to right), if you are skipping M,

        ---A---M---C---D
              /
         ----B

you'd simply pretend that the ancestry is like this and you'd be
fine, no?

        ---A-------C---D
                  /
         --------B

That is, when inspecting C, pass_blame() would enumerate its true
parents, notice that M to be skipped, which would cause it to
pretend as if C has M's parents instead of M itself as its parents.
If M in the true history is the first parent of C, then M's first
parent in the true history becomes C's first parent, M's second
parent in the true history becomes C's second parent, etc. (if C
were a merge in the true history, such a rewriting would make C an
octopus in the distorted history)

> The wrong commit is blamed.

So... I still suspect that it is merely a bug in your implementation
and there is nothing inherent that forces us to avoid skipping a
merge.

>> Somehow the damage to blame.c codebase looks way too noisy for what
>> the code wants to do.  If all we want is to pretend in a history,
>> e.g.
>> 
>>     ---O---A---B---C---D
>> 
>> that commit B does not exist, i.e. use a distorted view of the
>> history
>> 
>>     ---O---A-------C---D
>> 
>> wouldn't it be sufficient to modify pass_blame(), i.e. the only the
>> caller of the pass_blame_to_parent(), where we find the parent
>> commits of "C" and dig the history to pass blame to "B", and have it
>> pretend as if "B" does not exist and pass blame directly to "A"?
>
> I originally tried to skip 'B' in pass_blame() when B popped up as a
> scapegoat.  That broke the offsets of the blame_entries in the
> parent.

Why?  When inspecting C in order to exonerate it from as many lines
as possible, we'd run "git diff $P C" for each parent of C, but in
the distorted history (i.e. instead of using $P==B, we use $P==A).
As long as the code reads from "git diff A C", I do not see why "the
offsets in the parent" can be broken.  Care to elaborate a bit more?