blame: validate and peel the object names on the ignore list

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>

diff --git a/builtin/blame.c b/builtin/blame.c
index 94ef57c1cc304963452772e24125e5939dff0481..baa5d979cc2dbbdc229753de9b47955e9c0377b3 100644 (file)
--- 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 15d4e18c37353bf0f186155de78f1af322fd855c..2d0ab76fb569be21dacc68e66098a4814191325b 100644 (file)
--- 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 209ae7a1736e49818acc1e05b7f3cb4aceada0fe..01f6560283c38660a010ab84d90c24c9a0219189 100644 (file)
--- 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 67de83ae2bd6d7013afc11d4d7d7f5c605748275..24ae5018e8850d065da1ce0c6c33d81533ce0240 100755 (executable)
--- 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