Merge branch 'tc/last-modified-not-a-tree'

Giving "git last-modified" a tree (not a commit-ish) died an
uncontrolled death, which has been corrected.

* tc/last-modified-not-a-tree:
  last-modified: verify revision argument is a commit-ish
  last-modified: remove double error message
  last-modified: fix memory leak when more than one commit is given
  last-modified: rewrite error message when more than one commit given

2 files changed, 34 insertions, 11 deletions

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index f7f4c5109c..8900ceece1 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -124,7 +124,7 @@ static void add_path_from_diff(struct diff_queue_struct *q,
 
 static int populate_paths_from_revs(struct last_modified *lm)
 {
-	int num_interesting = 0;
+	int num_interesting = 0, ret = 0;
 	struct diff_options diffopt;
 
 	/*
@@ -146,16 +146,25 @@ static int populate_paths_from_revs(struct last_modified *lm)
 		if (obj->item->flags & UNINTERESTING)
 			continue;
 
-		if (num_interesting++)
-			return error(_("last-modified can only operate on one tree at a time"));
+		if (num_interesting++) {
+			ret = error(_("last-modified can only operate on one commit at a time"));
+			goto out;
+		}
+
+		if (!repo_peel_to_type(lm->rev.repo, obj->path, 0, obj->item, OBJ_COMMIT)) {
+			ret = error(_("revision argument '%s' is a %s, not a commit-ish"), obj->name, type_name(obj->item->type));
+			goto out;
+		}
 
 		diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
 			      &obj->item->oid, "", &diffopt);
 		diff_flush(&diffopt);
 	}
+
+out:
 	clear_pathspec(&diffopt.pathspec);
 
-	return 0;
+	return ret;
 }
 
 static void last_modified_emit(struct last_modified *lm,
@@ -494,7 +503,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
 	lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
 
 	if (populate_paths_from_revs(lm) < 0)
-		return error(_("unable to setup last-modified"));
+		return -1;
 
 	CALLOC_ARRAY(lm->all_paths, hashmap_get_size(&lm->paths));
 	lm->all_paths_nr = 0;
diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
index 3944d2e153..9dba4b9d90 100755
--- a/t/t8020-last-modified.sh
+++ b/t/t8020-last-modified.sh
@@ -8,14 +8,11 @@ test_expect_success 'setup' '
 	test_commit 1 file &&
 	mkdir a &&
 	test_commit 2 a/file &&
+	git tag -mA t2 2 &&
 	mkdir a/b &&
 	test_commit 3 a/b/file
 '
 
-test_expect_success 'cannot run last-modified on two trees' '
-	test_must_fail git last-modified HEAD HEAD~1
-'
-
 check_last_modified() {
 	local indir= &&
 	while test $# != 0
@@ -34,7 +31,7 @@ check_last_modified() {
 
 	cat >expect &&
 	git ${indir:+-C "$indir"} last-modified "$@" >tmp.1 &&
-	git name-rev --annotate-stdin --name-only --tags \
+	git name-rev --annotate-stdin --name-only --tags --exclude=t2 \
 		<tmp.1 >tmp.2 &&
 	tr '\t' ' ' <tmp.2 >actual &&
 	test_cmp expect actual
@@ -55,6 +52,13 @@ test_expect_success 'last-modified recursive' '
 	EOF
 '
 
+test_expect_success 'last-modified on annotated tag' '
+	check_last_modified t2 <<-\EOF
+	2 a
+	1 file
+	EOF
+'
+
 test_expect_success 'last-modified recursive with show-trees' '
 	check_last_modified -r -t <<-\EOF
 	3 a/b
@@ -265,9 +269,19 @@ test_expect_success 'last-modified merge undoes changes' '
 	EOF
 '
 
+test_expect_success 'cannot run last-modified on two commits' '
+	test_must_fail git last-modified HEAD HEAD~1 2>err &&
+	test_grep "last-modified can only operate on one commit at a time" err
+'
+
 test_expect_success 'last-modified complains about unknown arguments' '
 	test_must_fail git last-modified --foo 2>err &&
-	grep "unknown last-modified argument: --foo" err
+	test_grep "unknown last-modified argument: --foo" err
+'
+
+test_expect_success 'last-modified expects commit-ish' '
+	test_must_fail git last-modified HEAD^{tree} 2>err &&
+	test_grep "revision argument ${SQ}HEAD^{tree}${SQ} is a tree, not a commit-ish" err
 '
 
 test_done