blame: add the ability to ignore commits
diff mbox series

Message ID 20190107213013.231514-1-brho@google.com
State New
Headers show
Series
  • blame: add the ability to ignore commits
Related show

Commit Message

Barret Rhoden Jan. 7, 2019, 9:30 p.m. UTC
Commits that make formatting changes or renames are often not
interesting when blaming a file.  This commit, similar to
git-hyper-blame, allows one to specify certain revisions to ignore during
the blame process.

To ignore a revision, put its committish in a file specified by
--ignore-file=<file> or use -i <rev>, which can be repeated.  The file
.git-blame-ignore-revs is checked by default.

It's useful to be alerted to the presence of an ignored commit in the
history of a line.  Those lines will be marked with '*' in the
non-porcelain output.  The '*' is attached to the line number to keep
from breaking tools that rely on the whitespace between columns.

A blame_entry attributed to an ignored commit will get passed to its
parent.  If an ignored commit changed a line, an ancestor that changed
the line will get blamed.  However, if an ignored commit added lines, a
commit changing a nearby line may get blamed.  If no commit is found,
the original commit for the file will get blamed.

Signed-off-by: Barret Rhoden <brho@google.com>
---
 Documentation/git-blame.txt | 15 +++++++++
 blame.c                     | 38 +++++++++++++++++----
 blame.h                     |  3 ++
 builtin/blame.c             | 66 +++++++++++++++++++++++++++++++++++--
 4 files changed, 112 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Jan. 7, 2019, 11:13 p.m. UTC | #1
Barret Rhoden <brho@google.com> writes:

> Commits that make formatting changes or renames are often not
> interesting when blaming a file.  This commit, similar to
> git-hyper-blame, allows one to specify certain revisions to ignore during
> the blame process.
>
> To ignore a revision, put its committish in a file specified by
> --ignore-file=<file> or use -i <rev>, which can be repeated.

If I read it correctly, this gives a very limited form of -S, in the
sense that anything this can do can be expressed by using -S but the
reverse is not true, but is designed to be easier to use, in the
sense that unlike -S, this does not have to describe the part of the
history you do not have to lie about.  The documentation should say
something about these pros-and-cons to help readers decide which
feature to use.

I somehow feel that this is rare enough that it should not squat on
short-and-sweet '-i'.  We would want to reserve it to something like
"--ignore-case", for example.

> The file .git-blame-ignore-revs is checked by default.

Giving the projects a way to easily help participants to share the
same distorted view of the history is a good idea, but I do not
think we should allow projects to do so to those who merely clone it
without their consent.  IOW, "by default" is a terrible idea.

Perhaps paying attention to blame.skipRevsFile configuration
variable that is set by the end user would be sufficient----the
project can ship .blame-skip-revs (or any random filename of their
choice) in-tree as tracked contents and tell its users that they can
optionally use it.

> It's useful to be alerted to the presence of an ignored commit in the
> history of a line.  Those lines will be marked with '*' in the
> non-porcelain output.  The '*' is attached to the line number to keep
> from breaking tools that rely on the whitespace between columns.

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.

In general, I find this patch trying to change too many things at
the same time, without sufficient justification.  Perhaps do these
different things as separate steps in a single series?

> A blame_entry attributed to an ignored commit will get passed to its
> parent.

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 an ignored commit changed a line, an ancestor that changed
> the line will get blamed.  However, if an ignored commit added lines, a
> commit changing a nearby line may get blamed.  If no commit is found,
> the original commit for the file will get blamed.

The above somehow does not read as describing a carefully designed
behaviour; rather, it sounds as if it is saying "the code does
whatever random things it happens to do".  For example, when there
is a newly added line how is "A" commit changing a nearby line
chosen (a line has lines before it and after it---they may be
touched by different commits, and before and after that skipped
commit, so there are multiple commits to choose from)?

> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
> index 16323eb80e31..e41375374892 100644
> --- a/Documentation/git-blame.txt
> +++ b/Documentation/git-blame.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>  [verse]
>  'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]
>  	    [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]
> +	    [-i <rev>] [--no-default-ignores] [--ignore-file=<file>]
>  	    [--progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>..<rev>]
>  	    [--] <file>
>  
> @@ -84,6 +85,20 @@ include::blame-options.txt[]
>  	Ignore whitespace when comparing the parent's version and
>  	the child's to find where the lines came from.
>  
> +-i <rev>::
> +	Ignore revision when assigning blame.  Lines that were changed by an
> +	ignored commit will be marked with a `*` in the blame output.  Lines
> +	that were added by an ignored commit may be attributed commits making
> +	nearby changes or to the first commit touching the file.

It probably deserves to be told that this option can be given
multiple times and used cumulatively (unlike usual "last one wins"
rule).

> +--no-default-ignores::
> +	Do not automatically ignore revisions in the file
> +	`.git-blame-ignore-revs`.

This should not be "opt-out" like this.

> +--ignore-file=<file>::
> +	Ignore revisions listed in `file`, one revision per line.  Whitespace
> +	and comments beginning with `#` are ignored.

Should it be capable to take two or more ignore-files?  Or should we
use the usual "the last one wins" rule?

I think we should support blame.skipRevFile configuration variable
so that the users do not have to constantly give the option from the
command line.  And with that, there is no need to have a hardcoded
filename .git-blame-ignore-revs or anything like that.

If we are to use configuration variable, however, we'd need a way to
override its use from the command line, too.  Perhaps a sane
arrangement would be

    - if one or more --skip-revs-file=<file> are given from the
      command line, use all of them and ignore blame.skipRevsFile
      configuration variable.

    - if no --skip-revs-file=<file> is given from the command line,
      use blame.skipRevsFile configuration variable.

    - regardless of the above two, pay attention to --skip-rev=<rev>
      command line option(s).


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

Thanks.  I am personally not all that interested in this yet.
Ævar Arnfjörð Bjarmason Jan. 8, 2019, 1:12 p.m. UTC | #2
On Mon, Jan 07 2019, Barret Rhoden wrote:

> +static int handle_ignore_file(const char *path, struct string_list *ignores)
> +{
> +	FILE *fp = fopen(path, "r");
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	if (!fp)
> +		return -1;
> +	while (!strbuf_getline(&sb, fp)) {
> +		const char *hash;
> +
> +		hash = strchr(sb.buf, '#');
> +		if (hash)
> +			strbuf_setlen(&sb, hash - sb.buf);
> +		strbuf_trim(&sb);
> +		if (!sb.len)
> +			continue;
> +		string_list_append(ignores, sb.buf);
> +	}
> +	fclose(fp);
> +	strbuf_release(&sb);
> +	return 0;
> +}

Aside from other comments on this patch that Junio had either you mostly
copy-pasted this from init_skiplist() or you've come up with almost the
same code on your own.

In any case, if we're going to integrate something like this patch let's
split this "parse file with SHA-1s or comments/whitespace" into a
utility function that both this and init_skiplist() can call.

Then we could split up the description for the fsck.skipList config
variable to reference that format, and say that both it and this new
thing should consult those docs for how it's parsed.
Barret Rhoden Jan. 8, 2019, 4:27 p.m. UTC | #3
On 2019-01-07 at 15:13 Junio C Hamano <gitster@pobox.com> wrote:
> If I read it correctly, this gives a very limited form of -S, in the
> sense that anything this can do can be expressed by using -S but the
> reverse is not true, but is designed to be easier to use, in the
> sense that unlike -S, this does not have to describe the part of the
> history you do not have to lie about.  The documentation should say
> something about these pros-and-cons to help readers decide which
> feature to use.

Yeah, -S lists the revs to use, this lists the revs to *not* use.  I'll
add a note.

> I somehow feel that this is rare enough that it should not squat on
> short-and-sweet '-i'.  We would want to reserve it to something like
> "--ignore-case", for example.

Can do.  I'll change the interface to your suggestion from down below.

> > The file .git-blame-ignore-revs is checked by default.  
> 
> Giving the projects a way to easily help participants to share the
> same distorted view of the history is a good idea, but I do not
> think we should allow projects to do so to those who merely clone it
> without their consent.  IOW, "by default" is a terrible idea.
> 
> Perhaps paying attention to blame.skipRevsFile configuration
> variable that is set by the end user would be sufficient----the
> project can ship .blame-skip-revs (or any random filename of their
> choice) in-tree as tracked contents and tell its users that they can
> optionally use it.

A blame config option works for me.  I'd like the users/cloners of a
repo to not need to do anything extravagant, but a one-time config
would be fine.

> > It's useful to be alerted to the presence of an ignored commit in the
> > history of a line.  Those lines will be marked with '*' in the
> > non-porcelain output.  The '*' is attached to the line number to keep
> > from breaking tools that rely on the whitespace between columns.  
> 
> 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?  I can make a separate config option
to turn on the annotation.  Any preference for how it is marked?

> In general, I find this patch trying to change too many things at
> the same time, without sufficient justification.  Perhaps do these
> different things as separate steps in a single series?
> 
> > A blame_entry attributed to an ignored commit will get passed to its
> > parent.  
> 
> 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.  For
example, this line was brought in via a merge:

$ ~/src/git/git-blame include/linux/mm.h | grep VM_SYNC
b6fb293f2497a (Jan Kara 2017-11-01 16:36:41 +0100  204) #define VM_SYNC

It's from merge: a3841f94c7ec ("Merge tag 'libnvdimm-for-4.15', and if
we ignore it:

$ ~/src/git/git-blame -i a3841f94c7ecb include/linux/mm.h | grep VM_SYNC
cc2383ec06be0 (Konstantin Khlebnikov 2012-10-08 16:28:37 -0700  204*) #define VM_SYNC

The wrong commit is blamed.

I can put a note in the doc about it and die if we're given a merge
commit.  Is there a convenient helper for detecting a merge, or can I
just check for multiple parents?  (something like commit->parents &&
commit->parents->next)
 
> > If an ignored commit changed a line, an ancestor that changed
> > the line will get blamed.  However, if an ignored commit added lines, a
> > commit changing a nearby line may get blamed.  If no commit is found,
> > the original commit for the file will get blamed.  
> 
> The above somehow does not read as describing a carefully designed
> behaviour; rather, it sounds as if it is saying "the code does
> whatever random things it happens to do".  For example, when there
> is a newly added line how is "A" commit changing a nearby line
> chosen (a line has lines before it and after it---they may be
> touched by different commits, and before and after that skipped
> commit, so there are multiple commits to choose from)?

This was more of a commentary about its behavior.  If you ignore a
commit that added lines, it'd be nice to get a hint of what might have
caused it, and picking a commit that affected an adjacent line seemed
fine.  But yeah, it's not doing anything crazy.

> > diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
> > index 16323eb80e31..e41375374892 100644
> > --- a/Documentation/git-blame.txt
> > +++ b/Documentation/git-blame.txt
> > @@ -10,6 +10,7 @@ SYNOPSIS
> >  [verse]
> >  'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]
> >  	    [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]
> > +	    [-i <rev>] [--no-default-ignores] [--ignore-file=<file>]
> >  	    [--progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>..<rev>]
> >  	    [--] <file>
> >  
> > @@ -84,6 +85,20 @@ include::blame-options.txt[]
> >  	Ignore whitespace when comparing the parent's version and
> >  	the child's to find where the lines came from.
> >  
> > +-i <rev>::
> > +	Ignore revision when assigning blame.  Lines that were changed by an
> > +	ignored commit will be marked with a `*` in the blame output.  Lines
> > +	that were added by an ignored commit may be attributed commits making
> > +	nearby changes or to the first commit touching the file.  
> 
> It probably deserves to be told that this option can be given
> multiple times and used cumulatively (unlike usual "last one wins"
> rule).
> 
> > +--no-default-ignores::
> > +	Do not automatically ignore revisions in the file
> > +	`.git-blame-ignore-revs`.  
> 
> This should not be "opt-out" like this.
> 
> > +--ignore-file=<file>::
> > +	Ignore revisions listed in `file`, one revision per line.  Whitespace
> > +	and comments beginning with `#` are ignored.  
> 
> Should it be capable to take two or more ignore-files?  Or should we
> use the usual "the last one wins" rule?
> 
> I think we should support blame.skipRevFile configuration variable
> so that the users do not have to constantly give the option from the
> command line.  And with that, there is no need to have a hardcoded
> filename .git-blame-ignore-revs or anything like that.
> 
> If we are to use configuration variable, however, we'd need a way to
> override its use from the command line, too.  Perhaps a sane
> arrangement would be
> 
>     - if one or more --skip-revs-file=<file> are given from the
>       command line, use all of them and ignore blame.skipRevsFile
>       configuration variable.
> 
>     - if no --skip-revs-file=<file> is given from the command line,
>       use blame.skipRevsFile configuration variable.
> 
>     - regardless of the above two, pay attention to --skip-rev=<rev>
>       command line option(s).

Sounds fine to me.

> 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.  By running diff_hunks(), we get the opportunity to adjust the
offsets.  Also, when it comes to marking the blame_entries for marking
the output, we want to know the specific diffs and to break them up at
the boundaries of [tlno,same) in blame_chunk().

> Thanks.  I am personally not all that interested in this yet.

Thanks for taking a look.

Barret
Barret Rhoden Jan. 8, 2019, 4:41 p.m. UTC | #4
On 2019-01-08 at 14:12 Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Mon, Jan 07 2019, Barret Rhoden wrote:
> 
> > +static int handle_ignore_file(const char *path, struct string_list *ignores)
> > +{
> > +	FILE *fp = fopen(path, "r");
> > +	struct strbuf sb = STRBUF_INIT;
> > +
> > +	if (!fp)
> > +		return -1;
> > +	while (!strbuf_getline(&sb, fp)) {
> > +		const char *hash;
> > +
> > +		hash = strchr(sb.buf, '#');
> > +		if (hash)
> > +			strbuf_setlen(&sb, hash - sb.buf);
> > +		strbuf_trim(&sb);
> > +		if (!sb.len)
> > +			continue;
> > +		string_list_append(ignores, sb.buf);
> > +	}
> > +	fclose(fp);
> > +	strbuf_release(&sb);
> > +	return 0;
> > +}  
> 
> Aside from other comments on this patch that Junio had either you mostly
> copy-pasted this from init_skiplist() or you've come up with almost the
> same code on your own.
> 
> In any case, if we're going to integrate something like this patch let's
> split this "parse file with SHA-1s or comments/whitespace" into a
> utility function that both this and init_skiplist() can call.

One minor difference is that fsck wants an unabbreviated SHA-1, using
parse_oid_hex() instead of get_oid_committish().  Would you be OK with
also changing fsck to take a committish instead of a full SHA-1?

Is there a good place for the common helper?  Since it's an oidset, I
could put it in oidset.c.  oidset_parse_file() or something.

> Then we could split up the description for the fsck.skipList config
> variable to reference that format, and say that both it and this new
> thing should consult those docs for how it's parsed.

Is there a good spot for the generic skipList documentation?  The only
common text would be: 

	... comments ('#'), empty lines, and any leading and trailing
	whitespace is ignored

Thanks,

Barret
Barret Rhoden Jan. 8, 2019, 6:04 p.m. UTC | #5
On 2019-01-08 at 11:41 Barret Rhoden <brho@google.com> wrote:
> Would you be OK with
> also changing fsck to take a committish instead of a full SHA-1?

Actually, in retrospect, I can keep the unabbreviated SHA-1 for the
file inputs and use get_oid_committish() for the one-off --skip-rev=
cases.

Thanks,

Barret
Junio C Hamano Jan. 8, 2019, 6:26 p.m. UTC | #6
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?
Barret Rhoden Jan. 9, 2019, 8:48 p.m. UTC | #7
Hi -

On 2019-01-08 at 10:26 Junio C Hamano <gitster@pobox.com> wrote:
> >> 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.

Probably!  =)  I tried a bunch of things initially, and might have had
things broken for other reasons.  One reason was not calling
parse_commit() at the right time.

I took another look and had first_scapegoat() scan the parent list, and
for any parent in the list, replace it with its own parent list.  e.g.
when removing 'A':

         ---X---A---B---C
               /
              Y 
              
(B's parent was A, A is the skipped commit, and A has parents X and Y)

becomes:

         ---X---B---C
               /
              Y

But just yanking skipped commits from the scapegoat list doesn't help,
since B gets blamed for the changes made by A, since the file B and
{X,Y} differ, specifically due to the changes A made.  

You'd think that would have worked, since it sounds similar to what -S
revs-file does, but it turns out I want different behavior than -S for
the ignore/skip.  With -S, you blame a nearby commit (at least based on
my testing - what is the desired behavior there?).  With ignore/skip, I
want to blame the last commit that modified the line.

For instance, commit X does this:

-foo(x,y);
+foo(x,y,z);

Then commit Y comes along to reformat it:

-foo(x,y,z);
+foo(x, y, z);

And the history / rev-list for the file looks like:

---O---A---X---B---C---D---Y---E---F

I want to ignore/skip Y and see X in the blame output.  

Sorry that I wasn't clear about that from the beginning; I can see how
one could have different expectations for what happens when a commit is
skipped.

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

When we run "git diff $P C" with $P == A, the diffs from B (the one we
want to remove) will pop up and be attributed to C.  This is what
happened when I tried the "yank B from the history" approach from
above.  If we ever diff over a skipped commit like that, we'll blame
the wrong one. (C, here).

The first thing I tried was to simply move the blame_entry list from C
to B to A without calling diff_hunks().  That's where the "file offsets
were broken," since e->s_lno should be adjusted when passing an entry
from the target blame_origin to its parent blame_origin.  An example of
this is in blame_chunk() when we hand every blame_entry before tlno to
the parent in the "e->s_lno < tlno" block.

Without diffing commit B, we can't propagate the blame_entries.  And
if we just diff A and C, we'll get the "wrong" commit.

The other reason to run the diffs was to identify and mark the
blame_entry that would have been blamed for the skipped commit.  That's
for the annotations during the output phase.  

If we don't track and taint the blame entries, you could just call
blame_chunk() from blame_chunk_cb() with tlno = same, which in essence
tells the blame code that "this diff belongs to the parent."  But that
loses the blame_entry boundaries, and is almost nearly as much of a
change to blame.c.

When it comes to skipping merges, my existing code couldn't do it, but
I found a way to make it work:

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

Say the commit changing the line/blame_entry was X, and we try to skip
M.  'A' made no changes to the line.

If B is the first scapegoat, pass_blame_to_parent() will see no diff
between B and M for that line, and the blame will get passed to B, and
we're done.  This is the same both with my change and without.

However, if A is the first scapegoat, the line will have a diff, and
from the perspective of the ---A---M---C graph, it looks like M
introduced the change.  (In fact it did - it was via B though).  The
existing code would keep the blame on M and then check the next
scapegoat: B.  

With my change, it couldn't tell the difference between M introducing
a change that we wanted to skip and M having another parent that
introduced the change.  Both look like a diff from A..M.

I changed my version to handle that case.  In pass_blame(), if it fails
to find a scapegoat in the loop that calls pass_blame_to_parent(),
it'll make another loop through the scapegoats, where it will pass the
blame to the first scapegoat that has a diff.

This way, we get a first attempt to pass the blame for real (e.g. to
B), and then when we failed, we skip the commit.  In other words, we
only skip the commit when we need to.

With that change, from my earlier example:

This was wrong:
$ git-blame --skip-rev a3841f94c7ecb include/linux/mm.h | grep VM_SYNC
cc2383ec06be0 (Konstantin Khlebnikov 2012-10-08 16:28:37 -0700  204*) #define VM_SYNC

This is right:
$ git-blame --skip-rev a3841f94c7ecb include/linux/mm.h | grep VM_SYNC
b6fb293f2497a (Jan Kara              2017-11-01 16:36:41 +0100  204) #define VM_SYNC

Also note that in the 'right' case it didn't annotate it with the *.
That's because we didn't need to ignore the commit.  b6fb29 was the
correct one, and we didn't need to ignore M.

Anyway, thanks for reading and for the feedback.  I'll clean things up
and explain it better in another patchset.

Thanks,

Barret
Stefan Beller Jan. 9, 2019, 9:21 p.m. UTC | #8
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
Junio C Hamano Jan. 10, 2019, 10:29 p.m. UTC | #9
Barret Rhoden <brho@google.com> writes:

> For instance, commit X does this:
>
> -foo(x,y);
> +foo(x,y,z);
>
> Then commit Y comes along to reformat it:
>
> -foo(x,y,z);
> +foo(x, y, z);
>
> And the history / rev-list for the file looks like:
>
> ---O---A---X---B---C---D---Y---E---F
>
> I want to ignore/skip Y and see X in the blame output.  

If you skip Y, the altered history would have "foo(x, y, z)" in E,
"foo(x,y,z)" in X, and "foo(x,y)" in A.  If you start blaming from
F, you'd get E as the commit that explains the latest state.  If you
do not skip Y, you'd get Y.  I am not sure how you'd get X in either
case.
Barret Rhoden Jan. 14, 2019, 3:19 p.m. UTC | #10
On 2019-01-10 at 14:29 Junio C Hamano <gitster@pobox.com> wrote:
> > For instance, commit X does this:
> >
> > -foo(x,y);
> > +foo(x,y,z);
> >
> > Then commit Y comes along to reformat it:
> >
> > -foo(x,y,z);
> > +foo(x, y, z);
> >
> > And the history / rev-list for the file looks like:
> >
> > ---O---A---X---B---C---D---Y---E---F
> >
> > I want to ignore/skip Y and see X in the blame output.    
> 
> If you skip Y, the altered history would have "foo(x, y, z)" in E,
> "foo(x,y,z)" in X, and "foo(x,y)" in A.  If you start blaming from
> F, you'd get E as the commit that explains the latest state.  If you
> do not skip Y, you'd get Y.  I am not sure how you'd get X in either
> case.  

The way to do it is to let the blames get passed to Y - don't yank it
from the graph.  Then when trying to pass the blames from Y to its
parent, when we get a diff chunk that Y is responsible for, instead of
keeping it on Y's suspect list, we hand the blame_entry to D.  That
blame_entry will get passed all the way back to X, which also has a
diff that touches that line.

Basically we do the same blame processing as usual, but just don't let
any blames stick to Y.

Barret
Junio C Hamano Jan. 14, 2019, 5:45 p.m. UTC | #11
Barret Rhoden <brho@google.com> writes:

> On 2019-01-10 at 14:29 Junio C Hamano <gitster@pobox.com> wrote:
>> > For instance, commit X does this:
>> >
>> > -foo(x,y);
>> > +foo(x,y,z);
>> >
>> > Then commit Y comes along to reformat it:
>> >
>> > -foo(x,y,z);
>> > +foo(x, y, z);
>> >
>> > And the history / rev-list for the file looks like:
>> >
>> > ---O---A---X---B---C---D---Y---E---F
>> >
>> > I want to ignore/skip Y and see X in the blame output.    
>> 
>> If you skip Y, the altered history would have "foo(x, y, z)" in E,
>> "foo(x,y,z)" in X, and "foo(x,y)" in A.  If you start blaming from
>> F, you'd get E as the commit that explains the latest state.  If you
>> do not skip Y, you'd get Y.  I am not sure how you'd get X in either
>> case.  
>
> The way to do it is ...

Sorry, I made a too-fuzzy statement.  What I meant was, that unless
you are ignoring E, I do not know why you "would want to" attribute
a line "foo(x, y, z)" that appears in F to X.  Starting from X up to
D (and to Y in real history, but you are ignoring Y), the line was
"foo(x,y,z)", after E, it is "foo(x, y, z)".  I didn't mean to ask
how you "would show" such a result---as I do not yet understand why
you would want such a result to begin with.
Randall S. Becker Jan. 14, 2019, 6:26 p.m. UTC | #12
On January 14, 2019 12:46, Junio C Hamano wrote:
> Barret Rhoden <brho@google.com> writes:
> 
> > On 2019-01-10 at 14:29 Junio C Hamano <gitster@pobox.com> wrote:
> >> > For instance, commit X does this:
> >> >
> >> > -foo(x,y);
> >> > +foo(x,y,z);
> >> >
> >> > Then commit Y comes along to reformat it:
> >> >
> >> > -foo(x,y,z);
> >> > +foo(x, y, z);
> >> >
> >> > And the history / rev-list for the file looks like:
> >> >
> >> > ---O---A---X---B---C---D---Y---E---F
> >> >
> >> > I want to ignore/skip Y and see X in the blame output.
> >>
> >> If you skip Y, the altered history would have "foo(x, y, z)" in E,
> >> "foo(x,y,z)" in X, and "foo(x,y)" in A.  If you start blaming from F,
> >> you'd get E as the commit that explains the latest state.  If you do
> >> not skip Y, you'd get Y.  I am not sure how you'd get X in either
> >> case.
> >
> > The way to do it is ...
> 
> Sorry, I made a too-fuzzy statement.  What I meant was, that unless you
are
> ignoring E, I do not know why you "would want to" attribute a line "foo(x,
y,
> z)" that appears in F to X.  Starting from X up to D (and to Y in real
history, but
> you are ignoring Y), the line was "foo(x,y,z)", after E, it is "foo(x, y,
z)".  I
> didn't mean to ask how you "would show" such a result---as I do not yet
> understand why you would want such a result to begin with.

From my own community, this came up also. The intent was to show everyone
who touched a particular line, throughout history, not just the current one.
Perhaps that is what Barret is going for.

Regards,
Randall
Barret Rhoden Jan. 14, 2019, 7:03 p.m. UTC | #13
On 2019-01-14 at 13:26 "Randall S. Becker" <rsbecker@nexbridge.com>
wrote:
> > 
> > Sorry, I made a too-fuzzy statement.  What I meant was, that unless you  
> are
> > ignoring E, I do not know why you "would want to" attribute a line "foo(x,  
> y,
> > z)" that appears in F to X.  Starting from X up to D (and to Y in real  
> history, but
> > you are ignoring Y), the line was "foo(x,y,z)", after E, it is "foo(x, y,  
> z)".  I
> > didn't mean to ask how you "would show" such a result---as I do not yet
> > understand why you would want such a result to begin with.  
> 
> From my own community, this came up also. The intent was to show everyone
> who touched a particular line, throughout history, not just the current one.
> Perhaps that is what Barret is going for.

Yeah.  I want to find the most recent commit that changed a line, minus
a set of commits that are deemed 'not interesting' by the user.  

The primary reason for this is to not see a blame attributed to a
commit that does nothing but reformatting the code base.

One could also use this feature for one-off investigation with the
command-line switch.  Imagine a manually driven git-blame + git-log,
where you do a git-blame, check the commit, decide you want to see the
next one back, and rerun git-blame with --skip-rev=SHA1.

The Chromium depot_tools has a python tool that can do this, called
hyper-blame: 
(https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html).

I thought it was a useful feature for all of git's users.  And I also
didn't want people to have to install depot_tools.

Barret
Junio C Hamano Jan. 15, 2019, 6:08 a.m. UTC | #14
"Randall S. Becker" <rsbecker@nexbridge.com> writes:

> On January 14, 2019 12:46, Junio C Hamano wrote:
>> Barret Rhoden <brho@google.com> writes:
>> 
>> > On 2019-01-10 at 14:29 Junio C Hamano <gitster@pobox.com> wrote:
>> >> > For instance, commit X does this:
>> >> >
>> >> > -foo(x,y);
>> >> > +foo(x,y,z);
>> >> >
>> >> > Then commit Y comes along to reformat it:
>> >> >
>> >> > -foo(x,y,z);
>> >> > +foo(x, y, z);
>> >> >
>> >> > And the history / rev-list for the file looks like:
>> >> >
>> >> > ---O---A---X---B---C---D---Y---E---F
>> >> >
>> >> > I want to ignore/skip Y and see X in the blame output.
>> >>
>> >> If you skip Y, the altered history would have "foo(x, y, z)" in E,
>> >> "foo(x,y,z)" in X, and "foo(x,y)" in A.  If you start blaming from F,
>> >> you'd get E as the commit that explains the latest state.  If you do
>> >> not skip Y, you'd get Y.  I am not sure how you'd get X in either
>> >> case.
>> >
>> ...
>
> From my own community, this came up also. The intent was to show everyone
> who touched a particular line, throughout history, not just the current one.
> Perhaps that is what Barret is going for.

I think I now understand what is going on.  In short, what Barret
wanted is way more ambitious than "blame in a hypothetical history
that lacks certain commits".  There is no similiarity with existing
"-S <revs>" option that I asked about in my initial review.  This is
quite a different beast (and I kind-a like it ;-).

Instead of "skipping" Y, Barret wants to pretend that the effect of
commit Y, in addition to the commit Y itself, never existed in the
history.  Even when blaming back from F (where it has "foo(x, y,
z)", which is the same as E and down to Y), the algorithm wants to
pretend as if E and F had that line in "foo(x,y,z)" form, i.e.
before Y touched it, and also pretend that the user started blaming
a line in that shape.

As there is no general solution [*1*] to "how did this line looked
like before this commit", such a blame may not have a meaningful
answer.  When Y added a block of text without removing anything
(i.e. "git show Y" has no line that begins with '-'), and when the
user asked to blame that block of text in F, there is no sensible
answer to the question: "what got replaced by Y in this text?", so a
blame that "ignores" Y would not be able to answer "where did these
lines come from?" question.  But in some cases (e.g. when "git show
Y" has only one '-' line, immediately followed by '+' line, both
with quite a similar contents and no other change in the patch---it
is highly plausible that in such a change, the former became the
latter, like the example in Barret's message I was responding to),
we can say "this was how the line that was modified in Y looked like
before Y" and blaming while ignoring Y can have a reasonable answer.


[Footnote]

*1* Here, I use the word "solution" in the same sense as "solution
to a math equation", not in the sense to describe the steps to
derive that "solution".

Patch
diff mbox series

diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index 16323eb80e31..e41375374892 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -10,6 +10,7 @@  SYNOPSIS
 [verse]
 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]
 	    [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]
+	    [-i <rev>] [--no-default-ignores] [--ignore-file=<file>]
 	    [--progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>..<rev>]
 	    [--] <file>
 
@@ -84,6 +85,20 @@  include::blame-options.txt[]
 	Ignore whitespace when comparing the parent's version and
 	the child's to find where the lines came from.
 
+-i <rev>::
+	Ignore revision when assigning blame.  Lines that were changed by an
+	ignored commit will be marked with a `*` in the blame output.  Lines
+	that were added by an ignored commit may be attributed commits making
+	nearby changes or to the first commit touching the file.
+
+--no-default-ignores::
+	Do not automatically ignore revisions in the file
+	`.git-blame-ignore-revs`.
+
+--ignore-file=<file>::
+	Ignore revisions listed in `file`, one revision per line.  Whitespace
+	and comments beginning with `#` are ignored.
+
 --abbrev=<n>::
 	Instead of using the default 7+1 hexadecimal digits as the
 	abbreviated object name, use <n>+1 digits. Note that 1 column
diff --git a/blame.c b/blame.c
index d84c93778080..9e338cfa83e3 100644
--- a/blame.c
+++ b/blame.c
@@ -474,7 +474,8 @@  void blame_coalesce(struct blame_scoreboard *sb)
 
 	for (ent = sb->ent; ent && (next = ent->next); ent = next) {
 		if (ent->suspect == next->suspect &&
-		    ent->s_lno + ent->num_lines == next->s_lno) {
+		    ent->s_lno + ent->num_lines == next->s_lno &&
+		    ent->ignored == next->ignored) {
 			ent->num_lines += next->num_lines;
 			ent->next = next->next;
 			blame_origin_decref(next->suspect);
@@ -726,6 +727,8 @@  static void split_overlap(struct blame_entry *split,
 	int chunk_end_lno;
 	memset(split, 0, sizeof(struct blame_entry [3]));
 
+	split[0].ignored = split[1].ignored = split[2].ignored = e->ignored;
+
 	if (e->s_lno < tlno) {
 		/* there is a pre-chunk part not blamed on parent */
 		split[0].suspect = blame_origin_incref(e->suspect);
@@ -845,10 +848,10 @@  static struct blame_entry *reverse_blame(struct blame_entry *head,
  */
 static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,
 			int tlno, int offset, int same,
-			struct blame_origin *parent)
+			struct blame_origin *parent, int ignore_diffs)
 {
 	struct blame_entry *e = **srcq;
-	struct blame_entry *samep = NULL, *diffp = NULL;
+	struct blame_entry *samep = NULL, *diffp = NULL, *ignoredp = NULL;
 
 	while (e && e->s_lno < tlno) {
 		struct blame_entry *next = e->next;
@@ -862,6 +865,7 @@  static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,
 			int len = tlno - e->s_lno;
 			struct blame_entry *n = xcalloc(1, sizeof (struct blame_entry));
 			n->suspect = e->suspect;
+			n->ignored = e->ignored;
 			n->lno = e->lno + len;
 			n->s_lno = e->s_lno + len;
 			n->num_lines = e->num_lines - len;
@@ -916,6 +920,7 @@  static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,
 			int len = same - e->s_lno;
 			struct blame_entry *n = xcalloc(1, sizeof (struct blame_entry));
 			n->suspect = blame_origin_incref(e->suspect);
+			n->ignored = e->ignored;
 			n->lno = e->lno + len;
 			n->s_lno = e->s_lno + len;
 			n->num_lines = e->num_lines - len;
@@ -925,10 +930,24 @@  static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,
 			n->next = samep;
 			samep = n;
 		}
-		e->next = diffp;
-		diffp = e;
+		if (ignore_diffs) {
+			/* These go to the parent, like the ones before tlno. */
+			blame_origin_decref(e->suspect);
+			e->suspect = blame_origin_incref(parent);
+			e->s_lno += offset;
+			e->ignored = 1;
+			e->next = ignoredp;
+			ignoredp = e;
+		} else {
+			e->next = diffp;
+			diffp = e;
+		}
 		e = next;
 	}
+	if (ignoredp) {
+		**dstq = reverse_blame(ignoredp, **dstq);
+		*dstq = &ignoredp->next;
+	}
 	**srcq = reverse_blame(diffp, reverse_blame(samep, e));
 	/* Move across elements that are in the unblamable portion */
 	if (diffp)
@@ -938,6 +957,7 @@  static void blame_chunk(struct blame_entry ***dstq, struct blame_entry ***srcq,
 struct blame_chunk_cb_data {
 	struct blame_origin *parent;
 	long offset;
+	int ignore_diffs;
 	struct blame_entry **dstq;
 	struct blame_entry **srcq;
 };
@@ -950,7 +970,7 @@  static int blame_chunk_cb(long start_a, long count_a,
 	if (start_a - start_b != d->offset)
 		die("internal error in blame::blame_chunk_cb");
 	blame_chunk(&d->dstq, &d->srcq, start_b, start_a - start_b,
-		    start_b + count_b, d->parent);
+		    start_b + count_b, d->parent, d->ignore_diffs);
 	d->offset = start_a + count_a - (start_b + count_b);
 	return 0;
 }
@@ -973,18 +993,22 @@  static void pass_blame_to_parent(struct blame_scoreboard *sb,
 
 	d.parent = parent;
 	d.offset = 0;
+	d.ignore_diffs = 0;
 	d.dstq = &newdest; d.srcq = &target->suspects;
 
 	fill_origin_blob(&sb->revs->diffopt, parent, &file_p, &sb->num_read_blob);
 	fill_origin_blob(&sb->revs->diffopt, target, &file_o, &sb->num_read_blob);
 	sb->num_get_patch++;
 
+	if (oidset_contains(&sb->ignores, &target->commit->object.oid))
+		d.ignore_diffs = 1;
+
 	if (diff_hunks(&file_p, &file_o, blame_chunk_cb, &d, sb->xdl_opts))
 		die("unable to generate diff (%s -> %s)",
 		    oid_to_hex(&parent->commit->object.oid),
 		    oid_to_hex(&target->commit->object.oid));
 	/* The rest are the same as the parent */
-	blame_chunk(&d.dstq, &d.srcq, INT_MAX, d.offset, INT_MAX, parent);
+	blame_chunk(&d.dstq, &d.srcq, INT_MAX, d.offset, INT_MAX, parent, 0);
 	*d.dstq = NULL;
 	queue_blames(sb, parent, newdest);
 
diff --git a/blame.h b/blame.h
index be3a895043e0..3fe71a59d372 100644
--- a/blame.h
+++ b/blame.h
@@ -92,6 +92,7 @@  struct blame_entry {
 	 * scanning the lines over and over.
 	 */
 	unsigned score;
+	int ignored;
 };
 
 /*
@@ -117,6 +118,8 @@  struct blame_scoreboard {
 	/* linked list of blames */
 	struct blame_entry *ent;
 
+	struct oidset ignores;
+
 	/* look-up a line in the final buffer */
 	int num_lines;
 	int *lineno;
diff --git a/builtin/blame.c b/builtin/blame.c
index 6d798f99392e..698834426771 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -516,8 +516,13 @@  static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 						   ci.author_tz.buf,
 						   show_raw_time));
 			}
-			printf(" %*d) ",
-			       max_digits, ent->lno + 1 + cnt);
+			if (ent->ignored) {
+				printf(" %*d%c) ", max_digits - 1,
+				       ent->lno + 1 + cnt, '*');
+			} else {
+				printf(" %*d) ", max_digits,
+				       ent->lno + 1 + cnt);
+			}
 		}
 		if (reset)
 			fputs(reset, stdout);
@@ -603,6 +608,7 @@  static void find_alignment(struct blame_scoreboard *sb, int *option)
 {
 	int longest_src_lines = 0;
 	int longest_dst_lines = 0;
+	int has_ignore = 0;
 	unsigned largest_score = 0;
 	struct blame_entry *e;
 	int compute_auto_abbrev = (abbrev < 0);
@@ -639,9 +645,11 @@  static void find_alignment(struct blame_scoreboard *sb, int *option)
 			longest_dst_lines = num;
 		if (largest_score < blame_entry_score(sb, e))
 			largest_score = blame_entry_score(sb, e);
+		if (e->ignored)
+			has_ignore = 1;
 	}
 	max_orig_digits = decimal_width(longest_src_lines);
-	max_digits = decimal_width(longest_dst_lines);
+	max_digits = decimal_width(longest_dst_lines) + has_ignore;
 	max_score_digits = decimal_width(largest_score);
 
 	if (compute_auto_abbrev)
@@ -774,6 +782,43 @@  static int is_a_rev(const char *name)
 	return OBJ_NONE < oid_object_info(the_repository, &oid, NULL);
 }
 
+static void handle_ignore_list(struct blame_scoreboard *sb,
+			       struct string_list *ignores)
+{
+	struct string_list_item *i;
+	struct object_id oid;
+
+	oidset_init(&sb->ignores, 0);
+	for_each_string_list_item(i, ignores) {
+		if (get_oid_committish(i->string, &oid))
+			die(_("Can't find revision '%s' to ignore"), i->string);
+		oidset_insert(&sb->ignores, &oid);
+	}
+}
+
+static int handle_ignore_file(const char *path, struct string_list *ignores)
+{
+	FILE *fp = fopen(path, "r");
+	struct strbuf sb = STRBUF_INIT;
+
+	if (!fp)
+		return -1;
+	while (!strbuf_getline(&sb, fp)) {
+		const char *hash;
+
+		hash = strchr(sb.buf, '#');
+		if (hash)
+			strbuf_setlen(&sb, hash - sb.buf);
+		strbuf_trim(&sb);
+		if (!sb.len)
+			continue;
+		string_list_append(ignores, sb.buf);
+	}
+	fclose(fp);
+	strbuf_release(&sb);
+	return 0;
+}
+
 int cmd_blame(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
@@ -785,8 +830,11 @@  int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct progress_info pi = { NULL, 0 };
 
 	struct string_list range_list = STRING_LIST_INIT_NODUP;
+	struct string_list ignore_list = STRING_LIST_INIT_DUP;
 	int output_option = 0, opt = 0;
 	int show_stats = 0;
+	int no_default_ignores = 0;
+	const char *ignore_file = NULL;
 	const char *revs_file = NULL;
 	const char *contents_from = NULL;
 	const struct option options[] = {
@@ -806,6 +854,9 @@  int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('s', NULL, &output_option, N_("Suppress author name and timestamp (Default: off)"), OUTPUT_NO_AUTHOR),
 		OPT_BIT('e', "show-email", &output_option, N_("Show author email instead of name (Default: off)"), OUTPUT_SHOW_EMAIL),
 		OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
+		OPT_STRING_LIST('i', NULL, &ignore_list, N_("rev"), N_("Ignore <rev> when blaming")),
+		OPT_BOOL(0, "no-default-ignores", &no_default_ignores, N_("Do not ignore revisions from the .git-blame-ignore-revs file")),
+		OPT_STRING(0, "ignore-file", &ignore_file, N_("file"), N_("Ignore revisions from <file>")),
 		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),
 		OPT_BIT(0, "color-by-age", &output_option, N_("color lines by age"), OUTPUT_SHOW_AGE_WITH_COLOR),
 
@@ -987,6 +1038,13 @@  int cmd_blame(int argc, const char **argv, const char *prefix)
 		argv[argc - 1] = "--";
 	}
 
+	if (!no_default_ignores)
+		handle_ignore_file(".git-blame-ignore-revs", &ignore_list);
+	if (ignore_file) {
+		if (handle_ignore_file(ignore_file, &ignore_list))
+			die(_("Unable to open ignore-file '%s'"), ignore_file);
+	}
+
 	revs.disable_stdin = 1;
 	setup_revisions(argc, argv, &revs, NULL);
 
@@ -995,6 +1053,8 @@  int cmd_blame(int argc, const char **argv, const char *prefix)
 	sb.contents_from = contents_from;
 	sb.reverse = reverse;
 	sb.repo = the_repository;
+	handle_ignore_list(&sb, &ignore_list);
+	string_list_clear(&ignore_list, 0);
 	setup_scoreboard(&sb, path, &o);
 	lno = sb.num_lines;