Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list

["René Scharfe" <l.s.r@web.de>]

Am 28.09.20 um 15:26 schrieb Barret Rhoden:
> On 9/26/20 1:06 PM, Junio C Hamano wrote:
>> A user who says "ignore v2.3", sees that the commit pointed at by
>> that release tag is not ignored, comes here to complain, and is
>> told to write v2.3^0 instead, would not be happy.  It is a mistake
>> easy to catch to help users, so I am more for than against that
>> part of the change.
>
> That sounds like a nice change.

I agree -- peeling down to the commit is good.

>> I am completely neutral about "you told me to ignore this, but as
>> far as I can tell it does not even exist---did you screw up when
>> you prepared the list of stuff to ignore?" part.  I do not mind
>> seeing it removed.
>
> Part of my reasoning for "fail if you can't find it" was that it was
> highly likely to be a user error.  Especially because it will fail
> for a short hash from a file.  If you do have a list of commits to
> ignore (.git-blame-ignore-revs), that list is probably under version
> control in the same git repo, so it should change as you change
> branches.
>
> But all in all, I'm fine with skipping unknown objects.  Or for
> warning or having a git-config option, like we do for a couple other
> aspects of blame-ignore, since one size doesn't fit all.

I would expect user errors to be more likely with --ignore-rev and
interactive use (command line typos).  I see how aborting if the
referenced commit doesn't exists can help, and it's consistent with
other options that require a revision name.

I don't know how most people use --ignore-revs-file, but can imagine
a big bin of boring commits that is just appended to.  Having to
keep that file clean by removing commits from abandoned and garbage-
collected branches sounds like unnecessary busy-work to me.  (What
can I say -- I'm lazy.)

> Any performance improvement would be welcome.  I haven't looked at
> the code in a while, but I don't recall any reasons why this wouldn't
> work.

Using a commit flag instead of an oidset would only improve
performance noticeably if the product of the number of suspects and
ignored commits was huge, I guess.

I get weird timings for an ignore file containing basically all commits
(created with "git log --format=%H").  With Git's own repo and rc1:

Benchmark #1: ./git-blame --ignore-revs-file hashes Makefile
  Time (mean ± σ):      8.470 s ±  0.049 s    [User: 7.923 s, System: 0.547 s]
  Range (min … max):    8.434 s …  8.605 s    10 runs

And with the patch at the bottom:

Benchmark #1: ./git-blame --ignore-revs-file hashes Makefile
  Time (mean ± σ):      8.048 s ±  0.061 s    [User: 7.899 s, System: 0.146 s]
  Range (min … max):    7.987 s …  8.175 s    10 runs

That looks like a nice speedup, but why for system time alone?  Malloc
overhead perhaps?  And when I get rid of the intermediate oidset by
partially duplicating oidset_parse_file_carefully() it takes longer than
nine seconds.  Weird.  Perhaps a silly bug.

>>> This preexisting feature is curious.  It's even documented ('An
>>> empty file name, "", will clear the list of revs from previously
>>> processed files.') and covered by t8013.6.  Why would we need
>>> such magic in addition to the standard negation
>>> (--no-ignore-revs-file) for clearing the list?  The latter
>>> counters blame.ignoreRevsFile as well. *puzzled*
>>
>> I shared the puzzlement when I saw it, but ditto.
>
> I don't recall exactly.  Someone on the list might have wanted to
> both counter the blame.ignoreRevsFile and specify another file.  Or
> maybe they just wanted to counter the ignoreRevsFile, and I didn't
> know that --no- would already do that.  I'm certainly not wed to it.

The first step would be to show a deprecation warning, wait a few
releases and then remove that feature.  Not sure the effort and
potential user irritation is worth the saved conditional, doc lines
and test.  (We already established that I'm lazy.)

Anyway, here's the patch:
---
 blame.c         |  2 +-
 blame.h         |  5 +++--
 builtin/blame.c | 16 ++++++++++++----
 object.h        |  3 ++-
 4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/blame.c b/blame.c
index 686845b2b4..6e8c8fec9b 100644
--- a/blame.c
+++ b/blame.c
@@ -2487,7 +2487,7 @@ static void pass_blame(struct blame_scoreboard *sb, struct blame_origin *origin,
 	/*
 	 * Pass remaining suspects for ignored commits to their parents.
 	 */
-	if (oidset_contains(&sb->ignore_list, &commit->object.oid)) {
+	if (commit->object.flags & BLAME_IGNORE) {
 		for (i = 0, sg = first_scapegoat(revs, commit, sb->reverse);
 		     i < num_sg && sg;
 		     sg = sg->next, i++) {
diff --git a/blame.h b/blame.h
index b6bbee4147..d35167e8bd 100644
--- a/blame.h
+++ b/blame.h
@@ -16,6 +16,9 @@
 #define BLAME_DEFAULT_MOVE_SCORE	20
 #define BLAME_DEFAULT_COPY_SCORE	40

+/* Remember to update object flag allocation in object.h */
+#define BLAME_IGNORE	(1u<<14)
+
 struct fingerprint;

 /*
@@ -125,8 +128,6 @@ struct blame_scoreboard {
 	/* linked list of blames */
 	struct blame_entry *ent;

-	struct oidset ignore_list;
-
 	/* look-up a line in the final buffer */
 	int num_lines;
 	int *lineno;
diff --git a/builtin/blame.c b/builtin/blame.c
index bb0f29300e..1c6721b5d5 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -830,21 +830,29 @@ static void build_ignorelist(struct blame_scoreboard *sb,
 {
 	struct string_list_item *i;
 	struct object_id oid;
+	const struct object_id *o;
+	struct oidset_iter iter;
+	struct oidset ignore_list = OIDSET_INIT;

-	oidset_init(&sb->ignore_list, 0);
 	for_each_string_list_item(i, ignore_revs_file_list) {
 		if (!strcmp(i->string, ""))
-			oidset_clear(&sb->ignore_list);
+			oidset_clear(&ignore_list);
 		else
-			oidset_parse_file_carefully(&sb->ignore_list, i->string,
+			oidset_parse_file_carefully(&ignore_list, i->string,
 						    peel_to_commit_oid, sb);
 	}
 	for_each_string_list_item(i, ignore_rev_list) {
 		if (get_oid_committish(i->string, &oid) ||
 		    peel_to_commit_oid(&oid, sb))
 			die(_("cannot find revision %s to ignore"), i->string);
-		oidset_insert(&sb->ignore_list, &oid);
+		oidset_insert(&ignore_list, &oid);
 	}
+	oidset_iter_init(&ignore_list, &iter);
+	while ((o = oidset_iter_next(&iter))) {
+		struct commit *commit = lookup_commit(sb->repo, o);
+		commit->object.flags |= BLAME_IGNORE;
+	}
+	oidset_clear(&ignore_list);
 }

 int cmd_blame(int argc, const char **argv, const char *prefix)
diff --git a/object.h b/object.h
index 20b18805f0..6818c9296b 100644
--- a/object.h
+++ b/object.h
@@ -64,7 +64,8 @@ struct object_array {
  * negotiator/default.c:       2--5
  * walker.c:                 0-2
  * upload-pack.c:                4       11-----14  16-----19
- * builtin/blame.c:                        12-13
+ * blame.c:                                     14
+ * builtin/blame.c:                        12---14
  * bisect.c:                                        16
  * bundle.c:                                        16
  * http-push.c:                          11-----14
--
2.28.0