| Message ID | 20240110141559.387815-2-toon@iotcl.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0aabeaa562fa269834f10cbda6e966b553dcec71 |
| Headers | show |
| Series | Fix error message in git-show-ref(1) --exists | expand |
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.
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
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
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 '
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
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.
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 '
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(-)