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

From: Stefan Beller <sbeller@google.com>
To: Junio C Hamano <gitster@pobox.com>,
	Jonathan Tan <jonathantanmy@google.com>
Cc: Barret Rhoden <brho@google.com>, git <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: Wed, 9 Jan 2019 13:21:11 -0800	[thread overview]
Message-ID: <CAGZ79kYFC+ws00isp0F1kxmxVx0geg+R0VjjRf0=u2ocbkFUUQ@mail.gmail.com> (raw)
In-Reply-To: <xmqqimyz57l9.fsf@gitster-ct.c.googlers.com>

On Tue, Jan 8, 2019 at 10:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

From a users point of view it may be desirable to express all this
in the grafts file, i.e. -S <file> where the syntax of that file is extended.
For example we could introduce
    !<hash>
to make it exclude that commit.
Of course this could lead to confusion, as this puts 2 very different
concepts into the same option/file.

Speaking of the implementation:
This patch proposes an oid-set that is handled specially
in blame itself. I wonder if this could be generalized.

Jonathan Tan (cc'd) refactored and extended revision walking
for git-fetch and its negotiation leading to
7c85ee6c58 (Merge branch 'jt/fetch-negotiator-skipping',
2018-08-02), and 3390e42adb (fetch-pack:
support negotiation tip whitelist, 2018-07-02)
which implements another revision walking
algorithm that can be used to fine-tune revisions
walked when fetching.

I wonder if that work could be generalized more
to have "generic" revision walking algorithms
and then making use of them in either fetch or
blame.

For git-fetch there is a new algorithm that increases
step size between commits, which would be funny to
try for blame here. It would give the wrong blamed
commit, but would speed up blaming a lot.

Omitting some revisions seems to be applicable to
more than just blame/fetch, too.

Stefan