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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 8, 2019, 18:26 UTC
Message-ID: <xmqqimyz57l9.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190108112742.7878d4cb@gnomeregan.cam.corp.google.com>

Barret Rhoden <brho@google.com> writes:

Show 9 quoted lines

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

Show 7 quoted lines

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

Show 19 quoted lines

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