[PATCH 0/1] Fix error message in git-show-ref(1) --exists

[PATCH 0/1] Fix error message in git-show-ref(1) --exists

[Toon Claes]

References can validly contain forward slashes in their name. With the ref files
backend, these are stored as a directory tree. This means when you look up a
reference, you might find a directory where you expected a file.

This causes the option --exists, recently added to git-show-ref(1), to return
the following error:

    error: failed to look up reference: Is a directory

Other backends, like reftables, might store refs with forward slashes
differently. So they will not encounter the same error.

To make the error consistent across refs backend implementations, and to be more
clear to user, hide the error about having found a directory as a generic error:

    error: reference does not exist

Toon Claes (1):
  builtin/show-ref: treat directory directory as non-existing in
    --exists

 builtin/show-ref.c  | 2 +-
 t/t1403-show-ref.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)


base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
--
2.42.1

[PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists

[Toon Claes]

Recently [1] the option --exists was added to git-show-ref(1). When you
use this option against a ref that doesn't exist, but it is a parent
directory of an existing ref, you're getting the following error:

    $ git show-ref --exists refs/heads
    error: failed to look up reference: Is a directory

This error is confusing to the user. Instead, print the same error as
when the ref was not found:

    error: reference does not exist

[1]: 9080a7f178 (builtin/show-ref: add new mode to check for reference
existence, 2023-10-31)

Signed-off-by: Toon Claes <toon@iotcl.com>
---
 builtin/show-ref.c  | 2 +-
 t/t1403-show-ref.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index aaa2c39b2f..79955c2856 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -238,7 +238,7 @@ static int cmd_show_ref__exists(const char **refs)
 	if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
 			      &unused_oid, &unused_referent, &unused_type,
 			      &failure_errno)) {
-		if (failure_errno == ENOENT) {
+		if (failure_errno == ENOENT || failure_errno == EISDIR) {
 			error(_("reference does not exist"));
 			ret = 2;
 		} else {
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index ec1957b709..d0a8f7b121 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -262,9 +262,9 @@ test_expect_success '--exists with non-commit object' '
 
 test_expect_success '--exists with directory fails with generic error' '
 	cat >expect <<-EOF &&
-	error: failed to look up reference: Is a directory
+	error: reference does not exist
 	EOF
-	test_expect_code 1 git show-ref --exists refs/heads 2>err &&
+	test_expect_code 2 git show-ref --exists refs/heads 2>err &&
 	test_cmp expect err
 '
 
-- 
2.42.1

Re: [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists

[Eric Sunshine]

On Wed, Jan 10, 2024 at 9:19 AM Toon Claes <toon@iotcl.com> wrote:
> Recently [1] the option --exists was added to git-show-ref(1). When you
> use this option against a ref that doesn't exist, but it is a parent
> directory of an existing ref, you're getting the following error:
>
>     $ git show-ref --exists refs/heads
>     error: failed to look up reference: Is a directory
>
> This error is confusing to the user. Instead, print the same error as
> when the ref was not found:
>
>     error: reference does not exist
>
> Signed-off-by: Toon Claes <toon@iotcl.com>

It may not be worth a reroll, but I found the explanation you gave in
the cover letter more illuminating than what is written above for
explaining why this change is desirable. In particular, the discussion
of the reftable backend was very helpful.

Re: [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists

[Toon Claes]

Eric Sunshine <sunshine@sunshineco.com> writes:

> It may not be worth a reroll, but I found the explanation you gave in
> the cover letter more illuminating than what is written above for
> explaining why this change is desirable. In particular, the discussion
> of the reftable backend was very helpful.

Well, I wasn't sure the explanation would be relevant in the present,
because the reftable backend might happen relatively far into the
future.

--
Toon

Re: [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1356 bytes --]

On Wed, Jan 10, 2024 at 04:20:41PM +0100, Toon Claes wrote:
> 
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > It may not be worth a reroll, but I found the explanation you gave in
> > the cover letter more illuminating than what is written above for
> > explaining why this change is desirable. In particular, the discussion
> > of the reftable backend was very helpful.
> 
> Well, I wasn't sure the explanation would be relevant in the present,
> because the reftable backend might happen relatively far into the
> future.

I think it's not _that_ far in the future anymore. All prerequisite
topics are in flight already, so I expect that I can send the reftable
backend's implementation upstream around the end of January or start of
February. It will surely require several iterations, but we might be
able to land it in v2.44 (very optimistic) or v2.45 (more reasonable).

With that in mind I think it is okay to already mention the new backend
in commit messages -- at least I have been doing that, as well. Also,
the tree already knows about the reftable backend because we have both
the library and technical documentation in it for quite some time
already.

The patch itself looks good to me, thanks! Whether we want to reroll
just to amend the commit message I'll leave to you and others to decide.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> With that in mind I think it is okay to already mention the new backend
> in commit messages -- at least I have been doing that, as well. Also,
> the tree already knows about the reftable backend because we have both
> the library and technical documentation in it for quite some time
> already.
>
> The patch itself looks good to me, thanks! Whether we want to reroll
> just to amend the commit message I'll leave to you and others to decide.

Yup.  The patch looks good.  Here is what I tentatively queued.

----- >8 -----
From: Toon Claes <toon@iotcl.com>
Date: Wed, 10 Jan 2024 15:15:59 +0100
Subject: [PATCH] builtin/show-ref: treat directory directory as non-existing
 in --exists

9080a7f178 (builtin/show-ref: add new mode to check for reference
existence, 2023-10-31) added the option --exists to git-show-ref(1).

When you use this option against a ref that doesn't exist, but it is
a parent directory of an existing ref, you get the following error:

    $ git show-ref --exists refs/heads
    error: failed to look up reference: Is a directory

when the ref-files backend is in use.  To be more clear to user,
hide the error about having found a directory.  What matters to the
user is that the named ref does not exist.  Instead, print the same
error as when the ref was not found:

    error: reference does not exist

Signed-off-by: Toon Claes <toon@iotcl.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/show-ref.c  | 2 +-
 t/t1403-show-ref.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 7aac525a87..6025ce223d 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -239,7 +239,7 @@ static int cmd_show_ref__exists(const char **refs)
 	if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
 			      &unused_oid, &unused_referent, &unused_type,
 			      &failure_errno)) {
-		if (failure_errno == ENOENT) {
+		if (failure_errno == ENOENT || failure_errno == EISDIR) {
 			error(_("reference does not exist"));
 			ret = 2;
 		} else {
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index b50ae6fcf1..a8055f7fe1 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -260,9 +260,9 @@ test_expect_success '--exists with non-commit object' '
 
 test_expect_success '--exists with directory fails with generic error' '
 	cat >expect <<-EOF &&
-	error: failed to look up reference: Is a directory
+	error: reference does not exist
 	EOF
-	test_expect_code 1 git show-ref --exists refs/heads 2>err &&
+	test_expect_code 2 git show-ref --exists refs/heads 2>err &&
 	test_cmp expect err
 '
 
-- 
2.43.0-367-g186b115d30

Re: [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists

[Toon Claes]

Junio C Hamano <gitster@pobox.com> writes:

> Yup.  The patch looks good.  Here is what I tentatively queued.
>
> ----- >8 -----
> From: Toon Claes <toon@iotcl.com>
> Date: Wed, 10 Jan 2024 15:15:59 +0100
> Subject: [PATCH] builtin/show-ref: treat directory directory as non-existing
>  in --exists

You have a duplicate "directory" here.

> 9080a7f178 (builtin/show-ref: add new mode to check for reference
> existence, 2023-10-31) added the option --exists to git-show-ref(1).
>
> When you use this option against a ref that doesn't exist, but it is
> a parent directory of an existing ref, you get the following error:
>
>     $ git show-ref --exists refs/heads
>     error: failed to look up reference: Is a directory
>
> when the ref-files backend is in use.  To be more clear to user,
> hide the error about having found a directory.  What matters to the
> user is that the named ref does not exist.  Instead, print the same
> error as when the ref was not found:
>
>     error: reference does not exist
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/show-ref.c  | 2 +-
>  t/t1403-show-ref.sh | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index 7aac525a87..6025ce223d 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -239,7 +239,7 @@ static int cmd_show_ref__exists(const char **refs)
>  	if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
>  			      &unused_oid, &unused_referent, &unused_type,
>  			      &failure_errno)) {
> -		if (failure_errno == ENOENT) {
> +		if (failure_errno == ENOENT || failure_errno == EISDIR) {
>  			error(_("reference does not exist"));
>  			ret = 2;
>  		} else {
> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
> index b50ae6fcf1..a8055f7fe1 100755
> --- a/t/t1403-show-ref.sh
> +++ b/t/t1403-show-ref.sh
> @@ -260,9 +260,9 @@ test_expect_success '--exists with non-commit object' '
>
>  test_expect_success '--exists with directory fails with generic error' '
>  	cat >expect <<-EOF &&
> -	error: failed to look up reference: Is a directory
> +	error: reference does not exist
>  	EOF
> -	test_expect_code 1 git show-ref --exists refs/heads 2>err &&
> +	test_expect_code 2 git show-ref --exists refs/heads 2>err &&
>  	test_cmp expect err
>  '

The rest looks all good to me.

--
Toon

Re: [PATCH 1/1] builtin/show-ref: treat directory directory as non-existing in --exists

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Yup.  The patch looks good.  Here is what I tentatively queued.
>>
>> ----- >8 -----
>> From: Toon Claes <toon@iotcl.com>
>> Date: Wed, 10 Jan 2024 15:15:59 +0100
>> Subject: [PATCH] builtin/show-ref: treat directory directory as non-existing
>>  in --exists
>
> You have a duplicate "directory" here.

Yikes.  I did notice a breakage in somebody's mailer that inserted a
SP after the first dash in "--exists" and fixed it, but did not
notice you had "directory" duplicated there.  Thanks for noticing.

Amended.  Let's merge it down to 'next'.

Thanks.