threads / patch / 60718

patchbuiltin/show-ref: treat directory directory as non-existing in --exists

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

## tl;dr

8 messages between Jan 10, 2024 and Jan 18, 2024. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Toon Claes· Jan 10, 2024, 14:15 UTC · lore

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

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

Toon Claes· Jan 10, 2024, 14:15 UTC · re: Toon Claes · lore

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(-)
Show changes to 2 files +3 −3

builtin/show-ref.c, t/t1403-show-ref.sh

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
Eric Sunshine· Jan 10, 2024, 14:36 UTC · re: Toon Claes · lore

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

On Wed, Jan 10, 2024 at 9:19 AM Toon Claes <toon@iotcl.com> wrote:
Show 13 quoted lines
> 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.

Toon Claes· Jan 10, 2024, 15:20 UTC · re: Eric Sunshine · lore

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

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

Patrick Steinhardt· Jan 17, 2024, 06:29 UTC · re: Toon Claes · lore

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

On Wed, Jan 10, 2024 at 04:20:41PM +0100, Toon Claes wrote:
Show 11 quoted lines
> 
> 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
Junio C Hamano· Jan 17, 2024, 23:07 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 8 quoted lines
> 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(-)
Show changes to 2 files +3 −3

builtin/show-ref.c, t/t1403-show-ref.sh

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
Toon Claes· Jan 18, 2024, 08:24 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> 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.
Show 51 quoted lines
> 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

Junio C Hamano· Jan 18, 2024, 19:19 UTC · re: Toon Claes · lore

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

Toon Claes <toon@iotcl.com> writes:
Show 11 quoted lines
> 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.

← back to recent threads