[PATCH v3 0/5] show-ref: Allow -d, --head to work with --verify

[PATCH v3 0/5] show-ref: Allow -d, --head to work with --verify

[Vladimir Panteleev]

Third iteration, according to Junio's comments. This time we keep
show_ref and show_one separate, accept HEAD with --verify even without
--head, and add tests for dangling ref validation with --verify.

[PATCH v3 1/5] show-ref: Accept HEAD with --verify

[Vladimir Panteleev]

Previously, when --verify was specified, show-ref would use a separate
code path which did not handle HEAD and treated it as an invalid
ref. Thus, "git show-ref --verify HEAD" (where "--verify" is used
because the user is not interested in seeing refs/remotes/origin/HEAD)
did not work as expected.

Instead of insisting that the input begins with "refs/", allow "HEAD"
as well in the codepath that handles "--verify", so that all valid
full refnames including HEAD are passed to the same output machinery.

Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
---
 builtin/show-ref.c  |  2 +-
 t/t1403-show-ref.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 6d4e66900..0e53e3da4 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -202,7 +202,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
 		while (*pattern) {
 			struct object_id oid;
 
-			if (starts_with(*pattern, "refs/") &&
+			if ((starts_with(*pattern, "refs/") || !strcmp(*pattern, "HEAD")) &&
 			    !read_ref(*pattern, oid.hash)) {
 				if (!quiet)
 					show_one(*pattern, &oid);
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 7e10bcfe3..5932beada 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -164,4 +164,15 @@ test_expect_success 'show-ref --heads, --tags, --head, pattern' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show-ref --verify HEAD' '
+	echo $(git rev-parse HEAD) HEAD >expect &&
+	git show-ref --verify HEAD >actual &&
+	test_cmp expect actual &&
+
+	>expect &&
+
+	git show-ref --verify -q HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.11.0

[PATCH v3 2/5] show-ref: Allow -d to work with --verify

[Vladimir Panteleev]

Move handling of -d into show_one, so that it takes effect when
--verify is present as well as when it is absent. This is useful when
the user wishes to avoid the costly iteration of refs.

Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
---
 builtin/show-ref.c  | 23 ++++++++++++-----------
 t/t1403-show-ref.sh |  9 +++++++++
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 0e53e3da4..a72a626b1 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -19,19 +19,27 @@ static const char *exclude_existing_arg;
 
 static void show_one(const char *refname, const struct object_id *oid)
 {
-	const char *hex = find_unique_abbrev(oid->hash, abbrev);
+	const char *hex;
+	struct object_id peeled;
+
+	hex = find_unique_abbrev(oid->hash, abbrev);
 	if (hash_only)
 		printf("%s\n", hex);
 	else
 		printf("%s %s\n", hex, refname);
+
+	if (!deref_tags)
+		return;
+
+	if (!peel_ref(refname, peeled.hash)) {
+		hex = find_unique_abbrev(peeled.hash, abbrev);
+		printf("%s %s^{}\n", hex, refname);
+	}
 }
 
 static int show_ref(const char *refname, const struct object_id *oid,
 		    int flag, void *cbdata)
 {
-	const char *hex;
-	struct object_id peeled;
-
 	if (show_head && !strcmp(refname, "HEAD"))
 		goto match;
 
@@ -79,13 +87,6 @@ static int show_ref(const char *refname, const struct object_id *oid,
 
 	show_one(refname, oid);
 
-	if (!deref_tags)
-		return 0;
-
-	if (!peel_ref(refname, peeled.hash)) {
-		hex = find_unique_abbrev(peeled.hash, abbrev);
-		printf("%s %s^{}\n", hex, refname);
-	}
 	return 0;
 }
 
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 5932beada..c6872bd96 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -97,6 +97,9 @@ test_expect_success 'show-ref -d' '
 	git show-ref -d refs/tags/A refs/tags/C >actual &&
 	test_cmp expect actual &&
 
+	git show-ref --verify -d refs/tags/A refs/tags/C >actual &&
+	test_cmp expect actual &&
+
 	echo $(git rev-parse refs/heads/master) refs/heads/master >expect &&
 	git show-ref -d master >actual &&
 	test_cmp expect actual &&
@@ -116,6 +119,12 @@ test_expect_success 'show-ref -d' '
 	test_cmp expect actual &&
 
 	test_must_fail git show-ref -d --verify heads/master >actual &&
+	test_cmp expect actual &&
+
+	test_must_fail git show-ref --verify -d A C >actual &&
+	test_cmp expect actual &&
+
+	test_must_fail git show-ref --verify -d tags/A tags/C >actual &&
 	test_cmp expect actual
 
 '
-- 
2.11.0

[PATCH v3 3/5] show-ref: Move --quiet handling into show_one

[Vladimir Panteleev]

Do the same with --quiet as was done with -d, to remove the need to
perform this check at show_one's call site from the --verify branch.

Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
---
 builtin/show-ref.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index a72a626b1..ab8e0dc41 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -22,6 +22,9 @@ static void show_one(const char *refname, const struct object_id *oid)
 	const char *hex;
 	struct object_id peeled;
 
+	if (quiet)
+		return;
+
 	hex = find_unique_abbrev(oid->hash, abbrev);
 	if (hash_only)
 		printf("%s\n", hex);
@@ -82,9 +85,6 @@ static int show_ref(const char *refname, const struct object_id *oid,
 		die("git show-ref: bad ref %s (%s)", refname,
 		    oid_to_hex(oid));
 
-	if (quiet)
-		return 0;
-
 	show_one(refname, oid);
 
 	return 0;
@@ -205,8 +205,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
 
 			if ((starts_with(*pattern, "refs/") || !strcmp(*pattern, "HEAD")) &&
 			    !read_ref(*pattern, oid.hash)) {
-				if (!quiet)
-					show_one(*pattern, &oid);
+				show_one(*pattern, &oid);
 			}
 			else if (!quiet)
 				die("'%s' - not a valid ref", *pattern);
-- 
2.11.0

[PATCH v3 4/5] show-ref: Detect dangling refs under --verify as well

[Vladimir Panteleev]

Move detection of dangling refs into show_one, so that they are
detected when --verify is present as well as when it is absent.

Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
---
 builtin/show-ref.c  | 16 ++++++++--------
 t/t1403-show-ref.sh | 22 ++++++++++++++++++++++
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index ab8e0dc41..107d05fe0 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -22,6 +22,14 @@ static void show_one(const char *refname, const struct object_id *oid)
 	const char *hex;
 	struct object_id peeled;
 
+	/* This changes the semantics slightly that even under quiet we
+	 * detect and return error if the repository is corrupt and
+	 * ref points at a nonexistent object.
+	 */
+	if (!has_sha1_file(oid->hash))
+		die("git show-ref: bad ref %s (%s)", refname,
+		    oid_to_hex(oid));
+
 	if (quiet)
 		return;
 
@@ -77,14 +85,6 @@ static int show_ref(const char *refname, const struct object_id *oid,
 match:
 	found_match++;
 
-	/* This changes the semantics slightly that even under quiet we
-	 * detect and return error if the repository is corrupt and
-	 * ref points at a nonexistent object.
-	 */
-	if (!has_sha1_file(oid->hash))
-		die("git show-ref: bad ref %s (%s)", refname,
-		    oid_to_hex(oid));
-
 	show_one(refname, oid);
 
 	return 0;
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index c6872bd96..30354fd26 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -184,4 +184,26 @@ test_expect_success 'show-ref --verify HEAD' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show-ref --verify with dangling ref' '
+	sha1_file() {
+		echo "$*" | sed "s#..#.git/objects/&/#"
+	} &&
+
+	remove_object() {
+		file=$(sha1_file "$*") &&
+		test -e "$file" &&
+		rm -f "$file"
+	} &&
+
+	test_when_finished "rm -rf dangling" &&
+	(
+		git init dangling &&
+		cd dangling &&
+		test_commit dangling &&
+		sha=$(git rev-parse refs/tags/dangling) &&
+		remove_object $sha &&
+		test_must_fail git show-ref --verify refs/tags/dangling
+	)
+'
+
 test_done
-- 
2.11.0

[PATCH v3 5/5] show-ref: Remove dead `if (verify)' check

[Vladimir Panteleev]

As show_ref is only ever called on the path where --verify is not
specified, `verify' can never possibly be true here.

Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
---
 builtin/show-ref.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 107d05fe0..2dfcb5634 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -73,9 +73,6 @@ static int show_ref(const char *refname, const struct object_id *oid,
 				continue;
 			if (len == reflen)
 				goto match;
-			/* "--verify" requires an exact match */
-			if (verify)
-				continue;
 			if (refname[reflen - len - 1] == '/')
 				goto match;
 		}
-- 
2.11.0

Re: [PATCH v3 0/5] show-ref: Allow -d, --head to work with --verify

[Junio C Hamano]

Vladimir Panteleev <git@thecybershadow.net> writes:

> Third iteration, according to Junio's comments. This time we keep
> show_ref and show_one separate, accept HEAD with --verify even without
> --head, and add tests for dangling ref validation with --verify.

I am no longer a neutral judge but to me the resulting code looks a
lot more naturally structured than the original.

Thanks.  Will queue.

Re: [PATCH v3 4/5] show-ref: Detect dangling refs under --verify as well

[Junio C Hamano]

Vladimir Panteleev <git@thecybershadow.net> writes:

> Move detection of dangling refs into show_one, so that they are
> detected when --verify is present as well as when it is absent.
>
> Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
> ---
>  builtin/show-ref.c  | 16 ++++++++--------
>  t/t1403-show-ref.sh | 22 ++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index ab8e0dc41..107d05fe0 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -22,6 +22,14 @@ static void show_one(const char *refname, const struct object_id *oid)
>  	const char *hex;
>  	struct object_id peeled;
>  
> +	/* This changes the semantics slightly that even under quiet we
> +	 * detect and return error if the repository is corrupt and
> +	 * ref points at a nonexistent object.
> +	 */

This is my fault from more than 10 years ago, but I think the
comment shouldn't have been here (or at its original location).  It
talks about the behaviour change relative to the previous version
when the comment was added, i.e. cf0adba788 ("Store peeled refs in
packed-refs file.", 2006-11-19).

I'll remove it after the series settles.