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

[Junio C Hamano <gitster@pobox.com>]

The command reads list of object names to place on the ignore list
either from the command line or from a file, but they are not
checked with their object type (those read from the file are not
even checked for object existence).

Extend the oidset_parse_file() API and allow it to take a callback
that can be used to die (e.g. when an inappropriate input is read)
or modify the object name read (e.g. when a tag pointing at a commit
is read, and the caller wants a commit object name), and use it in
the code that handles ignore list.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/blame.c              | 27 ++++++++++++++++++++++++--
 oidset.c                     |  9 ++++++++-
 oidset.h                     |  9 +++++++++
 t/t8013-blame-ignore-revs.sh | 37 ++++++++++++++++++++++++++----------
 4 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 94ef57c1cc..baa5d979cc 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -27,6 +27,7 @@
 #include "object-store.h"
 #include "blame.h"
 #include "refs.h"
+#include "tag.h"
 
 static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
 
@@ -803,6 +804,26 @@ static int is_a_rev(const char *name)
 	return OBJ_NONE < oid_object_info(the_repository, &oid, NULL);
 }
 
+static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)
+{
+	struct repository *r = ((struct blame_scoreboard *)cbdata)->repo;
+	struct object_id oid;
+
+	oidcpy(&oid, oid_ret);
+	while (1) {
+		struct object *obj;
+		int kind = oid_object_info(r, &oid, NULL);
+		if (kind == OBJ_COMMIT) {
+			oidcpy(oid_ret, &oid);
+			return 0;
+		}
+		if (kind != OBJ_TAG)
+			return -1;
+		obj = deref_tag(r, parse_object(r, &oid), NULL, 0);
+		oidcpy(&oid, &obj->oid);
+	}
+}
+
 static void build_ignorelist(struct blame_scoreboard *sb,
 			     struct string_list *ignore_revs_file_list,
 			     struct string_list *ignore_rev_list)
@@ -815,10 +836,12 @@ static void build_ignorelist(struct blame_scoreboard *sb,
 		if (!strcmp(i->string, ""))
 			oidset_clear(&sb->ignore_list);
 		else
-			oidset_parse_file(&sb->ignore_list, i->string);
+			oidset_parse_file_carefully(&sb->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))
+		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);
 	}
diff --git a/oidset.c b/oidset.c
index 15d4e18c37..2d0ab76fb5 100644
--- a/oidset.c
+++ b/oidset.c
@@ -42,6 +42,12 @@ int oidset_size(struct oidset *set)
 }
 
 void oidset_parse_file(struct oidset *set, const char *path)
+{
+	oidset_parse_file_carefully(set, path, NULL, NULL);
+}
+
+void oidset_parse_file_carefully(struct oidset *set, const char *path,
+				 oidset_parse_tweak_fn fn, void *cbdata)
 {
 	FILE *fp;
 	struct strbuf sb = STRBUF_INIT;
@@ -66,7 +72,8 @@ void oidset_parse_file(struct oidset *set, const char *path)
 		if (!sb.len)
 			continue;
 
-		if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0')
+		if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0' ||
+		    (fn && fn(&oid, cbdata)))
 			die("invalid object name: %s", sb.buf);
 		oidset_insert(set, &oid);
 	}
diff --git a/oidset.h b/oidset.h
index 209ae7a173..01f6560283 100644
--- a/oidset.h
+++ b/oidset.h
@@ -73,6 +73,15 @@ void oidset_clear(struct oidset *set);
  */
 void oidset_parse_file(struct oidset *set, const char *path);
 
+/*
+ * Similar to the above, but with a callback which can (1) return non-zero to
+ * signal displeasure with the object and (2) replace object ID with something
+ * else (meant to be used to "peel").
+ */
+typedef int (*oidset_parse_tweak_fn)(struct object_id *, void *);
+void oidset_parse_file_carefully(struct oidset *set, const char *path,
+				 oidset_parse_tweak_fn fn, void *cbdata);
+
 struct oidset_iter {
 	kh_oid_set_t *set;
 	khiter_t iter;
diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
index 67de83ae2b..24ae5018e8 100755
--- a/t/t8013-blame-ignore-revs.sh
+++ b/t/t8013-blame-ignore-revs.sh
@@ -21,6 +21,7 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m X &&
 	git tag X &&
+	git tag -a -m "X (annotated)" XT &&
 
 	git blame --line-porcelain file >blame_raw &&
 
@@ -33,19 +34,35 @@ test_expect_success setup '
 	test_cmp expect actual
 '
 
-# Ignore X, make sure A is blamed for line 1 and B for line 2.
-test_expect_success ignore_rev_changing_lines '
-	git blame --line-porcelain --ignore-rev X file >blame_raw &&
-
-	grep -E "^[0-9a-f]+ [0-9]+ 1" blame_raw | sed -e "s/ .*//" >actual &&
-	git rev-parse A >expect &&
-	test_cmp expect actual &&
+# Ensure bogus --ignore-rev requests are caught
+test_expect_success 'validate --ignore-rev' '
+	test_must_fail git blame --ignore-rev X^{tree} file
+'
 
-	grep -E "^[0-9a-f]+ [0-9]+ 2" blame_raw | sed -e "s/ .*//" >actual &&
-	git rev-parse B >expect &&
-	test_cmp expect actual
+# Ensure bogus --ignore-revs-file requests are caught
+test_expect_success 'validate --ignore-revs-file' '
+	git rev-parse X^{tree} >ignore_x &&
+	test_must_fail git blame --ignore-revs-file ignore_x file
 '
 
+for I in X XT
+do
+	# Ignore X (or XT), make sure A is blamed for line 1 and B for line 2.
+	# Giving X (i.e. commit) and XT (i.e. annotated tag to commit) should
+	# produce the same result.
+	test_expect_success "ignore_rev_changing_lines ($I)" '
+		git blame --line-porcelain --ignore-rev $I file >blame_raw &&
+
+		grep -E "^[0-9a-f]+ [0-9]+ 1" blame_raw | sed -e "s/ .*//" >actual &&
+		git rev-parse A >expect &&
+		test_cmp expect actual &&
+
+		grep -E "^[0-9a-f]+ [0-9]+ 2" blame_raw | sed -e "s/ .*//" >actual &&
+		git rev-parse B >expect &&
+		test_cmp expect actual
+	'
+done
+
 # For ignored revs that have added 'unblamable' lines, attribute those to the
 # ignored commit.
 # 	A--B--X--Y
-- 
2.28.0-718-gd8d5e3da39