From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Wed, 10 Apr 2019 19:00:48 GMT
Subject: Re: [PATCH v6 3/6] blame: add the ability to ignore commits and their changes
Message-ID: <878swhfzxb.fsf@evledraar.gmail.com>
In-Reply-To: <20190410162409.117264-4-brho@google.com>


On Wed, Apr 10 2019, Barret Rhoden wrote:

(Just skimming)

> revisions for commits that perform mass reformatting, and their users
> have the optional to ignore all of the commits in that file.

s/have the optional/have the option/

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

Maybe just say "Ignore revisions listed in `file`, which is expected to
be in the same format as an `fsck.skipList`.".

> +	the `blame.ignoreRevsFile` config option.  An empty file name, `""`, will
> +	clear the list of revs from previously processed files.

Maybe I haven't read this carefully enough but the use-case for this
doesn't seem to be explained, you need this for the option, but the
config file too? If I want to override fsck.skipList I do
`fsck.skipList=/dev/zero`. Isn't that enough for this use-case without
introducing config state-machine magic?

> +	split[0].unblamable = e->unblamable;
> +	split[1].unblamable = e->unblamable;
> +	split[2].unblamable = e->unblamable;

I wonder what the comfort level for people in general is before turning
this sort of thing into a for-loop, 4? :)

> +	nr_lines = e->num_lines;	// e changes in the loop

A C++-like trailing comment.

> +	grep "^[0-9a-f]\+ [0-9]\+ 1" blame_raw | sed -e "s/ .*//" >actual &&
> +	git rev-parse X >expect &&
> +	test_cmp expect actual &&
> +
> +	grep "^[0-9a-f]\+ [0-9]\+ 2" blame_raw | sed -e "s/ .*//" >actual &&
> +	git rev-parse X >expect &&
> +	test_cmp expect actual

The grep here is a bug. See my 4abf20f004 ("tests: fix unportable "\?"
and "\+" regex syntax", 2019-02-21).