Re: [PATCH 11/12] builtin/show-ref: add new mode to check for reference existence
From: Eric Sunshine <hidden>
Date: 2023-10-24 21:02:09
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt [off-list ref] 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/
quoted hunk ↗ jump to hunk
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 <redacted> ---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.
quoted hunk ↗ jump to hunk
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;
quoted hunk ↗ jump to hunk
@@ -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).