From: Eric Sunshine <sunshine@sunshineco.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Han-Wen Nienhuys <hanwen@google.com>
Subject: Re: [PATCH 11/12] builtin/show-ref: add new mode to check for reference existence
Date: Tue, 24 Oct 2023 17:01:55 -0400 [thread overview]
Message-ID: <CAPig+cRTOMie0rUf=Mhbo9e2EXf-_2kQyMeqpB9OCRB1MZZ1rw@mail.gmail.com> (raw)
In-Reply-To: <2f876e61dd36a8887a1286bb8db9fb6577c55c9b.1698152926.git.ps@pks.im>
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> While we have multiple ways to show the value of a given reference, we
> do not have any way to check whether a reference exists at all. While
> commands like git-rev-parse(1) or git-show-ref(1) can be used to check
> for reference existence in case the reference resolves to something
> sane, neither of them can be used to check for existence in some other
> scenarios where the reference does not resolve cleanly:
>
> - References which have an invalid name cannot be resolved.
>
> - References to nonexistent objects cannot be resolved.
>
> - Dangling symrefs can be resolved via git-symbolic-ref(1), but this
> requires the caller to special case existence checks depending on
> whteher or not a reference is symbolic or direct.
s/whteher/whether/
> Furthermore, git-rev-list(1) and other commands do not let the caller
> distinguish easily between an actually missing reference and a generic
> error.
>
> Taken together, this gseems like sufficient motivation to introduce a
s/gseems/seems/
> separate plumbing command to explicitly check for the existence of a
> reference without trying to resolve its contents.
>
> This new command comes in the form of `git show-ref --exists`. This
> new mode will exit successfully when the reference exists, with a
> specific error code of 2 when it does not exist, or with 1 when there
> has been a generic error.
>
> Note that the only way to properly implement this command is by using
> the internal `refs_read_raw_ref()` function. While the public function
> `refs_resolve_ref_unsafe()` can be made to behave in the same way by
> passing various flags, it does not provide any way to obtain the errno
> with which the reference backend failed when reading the reference. As
> such, it becomes impossible for us to distinguish generic errors from
> the explicit case where the reference wasn't found.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
> @@ -65,6 +70,12 @@ OPTIONS
> +--exists::
> +
> + Check whether the given reference exists. Returns an error code of 0 if
We probably want to call this "exit code" rather than "error code"
since the latter is unnecessarily scary sounding for the success case
(when the ref does exit).
> + it does, 2 if it is missing, and 128 in case looking up the reference
> + failed with an error other than the reference being missing.
The commit message says it returns 1 for a generic error, but this
inconsistently says it returns 128 for that case. The actual
implementation returns 1.
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> @@ -214,6 +215,41 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,
> +static int cmd_show_ref__exists(const char **refs)
> +{
> + struct strbuf unused_referent = STRBUF_INIT;
> + struct object_id unused_oid;
> + unsigned int unused_type;
> + int failure_errno = 0;
> + const char *ref;
> + int ret = 1;
> +
> + if (!refs || !*refs)
> + die("--exists requires a reference");
> + ref = *refs++;
> + if (*refs)
> + die("--exists requires exactly one reference");
> +
> + if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
> + &unused_oid, &unused_referent, &unused_type,
> + &failure_errno)) {
> + if (failure_errno == ENOENT) {
> + error(_("reference does not exist"));
The documentation doesn't mention this printing any output, and indeed
one would intuitively expect a boolean-like operation to not produce
any printed output since its exit code indicates the result (except,
of course, in the case of a real error).
> + ret = 2;
> + } else {
> + error(_("failed to look up reference: %s"), strerror(failure_errno));
Or use error_errno():
errno = failure_errno;
error_errno(_("failed to look up reference: %s"));
> + }
> +
> + goto out;
> + }
> +
> + ret = 0;
> +
> +out:
> + strbuf_release(&unused_referent);
> + return ret;
> +}
It's a bit odd having `ret` be 1 at the outset rather than 0, thus
making the logic a bit more difficult to reason about. I would have
expected it to be organized like this:
int ret = 0;
if (refs_read_raw_ref(...)) {
if (failure_errno == ENOENT) {
ret = 2;
} else {
ret = 1;
errno = failure_errno;
error_errno(_("failed to look up reference: %s"));
}
}
strbuf_release(...);
return ret;
> @@ -272,13 +309,15 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> + if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
> + die(_("only one of --exclude-existing, --exists or --verify can be given"));
When reviewing an earlier patch in this series, I forgot to mention
that we can simplify the life of translators by using placeholders:
die(_("options '%s', '%s' or '%s' cannot be used together"),
"--exclude-existing", "--exists", "--verify");
which ensures that they don't translate the literal option names, and
makes it possible to reuse the translated message in multiple
locations (since it doesn't mention hard-coded option names).
next prev parent reply other threads:[~2023-10-24 21:02 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-24 13:10 [PATCH 00/12] show-ref: introduce mode to check for ref existence Patrick Steinhardt
2023-10-24 13:10 ` [PATCH 01/12] builtin/show-ref: convert pattern to a local variable Patrick Steinhardt
2023-10-24 13:10 ` [PATCH 02/12] builtin/show-ref: split up different subcommands Patrick Steinhardt
2023-10-24 17:55 ` Eric Sunshine
2023-10-24 13:10 ` [PATCH 03/12] builtin/show-ref: fix leaking string buffer Patrick Steinhardt
2023-10-24 13:10 ` [PATCH 04/12] builtin/show-ref: fix dead code when passing patterns Patrick Steinhardt
2023-10-24 18:02 ` Eric Sunshine
2023-10-24 13:10 ` [PATCH 05/12] builtin/show-ref: refactor `--exclude-existing` options Patrick Steinhardt
2023-10-24 18:48 ` Eric Sunshine
2023-10-25 11:50 ` Patrick Steinhardt
2023-10-24 13:11 ` [PATCH 06/12] builtin/show-ref: stop using global variable to count matches Patrick Steinhardt
2023-10-24 13:11 ` [PATCH 07/12] builtin/show-ref: stop using global vars for `show_one()` Patrick Steinhardt
2023-10-24 13:11 ` [PATCH 08/12] builtin/show-ref: refactor options for patterns subcommand Patrick Steinhardt
2023-10-24 13:11 ` [PATCH 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands Patrick Steinhardt
2023-10-24 19:25 ` Eric Sunshine
2023-10-24 13:11 ` [PATCH 10/12] builtin/show-ref: explicitly spell out different modes in synopsis Patrick Steinhardt
2023-10-24 19:39 ` Eric Sunshine
2023-10-25 11:50 ` Patrick Steinhardt
2023-10-24 13:11 ` [PATCH 11/12] builtin/show-ref: add new mode to check for reference existence Patrick Steinhardt
2023-10-24 21:01 ` Eric Sunshine [this message]
2023-10-25 11:50 ` Patrick Steinhardt
2023-10-24 13:11 ` [PATCH 12/12] t: use git-show-ref(1) to check for ref existence Patrick Steinhardt
2023-10-24 19:17 ` [PATCH 00/12] show-ref: introduce mode " Junio C Hamano
2023-10-25 14:26 ` Han-Wen Nienhuys
2023-10-25 14:44 ` Phillip Wood
2023-10-26 9:48 ` Patrick Steinhardt
2023-10-27 13:06 ` Phillip Wood
2023-10-26 9:44 ` Patrick Steinhardt
2023-10-26 9:56 ` [PATCH v2 " Patrick Steinhardt
2023-10-26 9:56 ` [PATCH v2 01/12] builtin/show-ref: convert pattern to a local variable Patrick Steinhardt
2023-10-26 9:56 ` [PATCH v2 02/12] builtin/show-ref: split up different subcommands Patrick Steinhardt
2023-10-26 9:56 ` [PATCH v2 03/12] builtin/show-ref: fix leaking string buffer Patrick Steinhardt
2023-10-30 18:10 ` Taylor Blau
2023-10-26 9:56 ` [PATCH v2 04/12] builtin/show-ref: fix dead code when passing patterns Patrick Steinhardt
2023-10-30 18:24 ` Taylor Blau
2023-10-26 9:56 ` [PATCH v2 05/12] builtin/show-ref: refactor `--exclude-existing` options Patrick Steinhardt
2023-10-30 18:37 ` Taylor Blau
2023-10-31 8:10 ` Patrick Steinhardt
2023-10-30 18:55 ` Taylor Blau
2023-10-31 8:10 ` Patrick Steinhardt
2023-10-26 9:56 ` [PATCH v2 06/12] builtin/show-ref: stop using global variable to count matches Patrick Steinhardt
2023-10-30 19:14 ` Taylor Blau
2023-10-26 9:56 ` [PATCH v2 07/12] builtin/show-ref: stop using global vars for `show_one()` Patrick Steinhardt
2023-10-26 9:56 ` [PATCH v2 08/12] builtin/show-ref: refactor options for patterns subcommand Patrick Steinhardt
2023-10-26 9:56 ` [PATCH v2 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands Patrick Steinhardt
2023-10-30 19:31 ` Taylor Blau
2023-10-31 8:10 ` Patrick Steinhardt
2023-10-26 9:57 ` [PATCH v2 10/12] builtin/show-ref: explicitly spell out different modes in synopsis Patrick Steinhardt
2023-10-26 9:57 ` [PATCH v2 11/12] builtin/show-ref: add new mode to check for reference existence Patrick Steinhardt
2023-10-26 9:57 ` [PATCH v2 12/12] t: use git-show-ref(1) to check for ref existence Patrick Steinhardt
2023-10-30 19:32 ` [PATCH v2 00/12] show-ref: introduce mode " Taylor Blau
2023-10-31 2:26 ` Junio C Hamano
2023-10-31 8:16 ` [PATCH v3 00/12] builtin/show-ref: " Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 01/12] builtin/show-ref: convert pattern to a local variable Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 02/12] builtin/show-ref: split up different subcommands Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 03/12] builtin/show-ref: fix leaking string buffer Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 04/12] builtin/show-ref: fix dead code when passing patterns Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 05/12] builtin/show-ref: refactor `--exclude-existing` options Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 06/12] builtin/show-ref: stop using global variable to count matches Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 07/12] builtin/show-ref: stop using global vars for `show_one()` Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 08/12] builtin/show-ref: refactor options for patterns subcommand Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 10/12] builtin/show-ref: explicitly spell out different modes in synopsis Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 11/12] builtin/show-ref: add new mode to check for reference existence Patrick Steinhardt
2023-10-31 8:16 ` [PATCH v3 12/12] t: use git-show-ref(1) to check for ref existence Patrick Steinhardt
2023-10-31 19:06 ` [PATCH v3 00/12] builtin/show-ref: introduce mode " Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPig+cRTOMie0rUf=Mhbo9e2EXf-_2kQyMeqpB9OCRB1MZZ1rw@mail.gmail.com' \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=hanwen@google.com \
--cc=ps@pks.im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).