Re: [GSoC][PATCH 1/2] builtin/refs: add 'exists' subcommand
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Aug 21, 2025, 10:21 UTC
- Message-ID
- <aKbzLhqryk5d-zgh@pks.im>
- In-Reply-To
- <20250821085246.929307-2-meetsoni3017@gmail.com>
On Thu, Aug 21, 2025 at 02:22:45PM +0530, Meet Soni wrote:
Show 10 quoted lines
> As part of the ongoing effort to consolidate reference handling, > introduce a new `exists` subcommand. This command provides the same > functionality and exit-code behavior as `git show-ref --exists`, serving > as its modern replacement. > > The logic for `show-ref --exists` is minimal. Rather than creating a > shared helper function which would be overkill for ~20 lines of code, > its implementation is intentionally duplicated here. This contrasts with > `git refs list`, where sharing the larger implementation of > `for-each-ref` was necessary.
I agree with this decision. It doesn't really feel worth it to share code for such trivial functionality.
Show 17 quoted lines
> diff --git a/builtin/refs.c b/builtin/refs.c
> index 76224feba4..617d8ab138 100644
> --- a/builtin/refs.c
> +++ b/builtin/refs.c
> @@ -113,6 +117,48 @@ static int cmd_refs_list(int argc, const char **argv, const char *prefix,
> return for_each_ref_core(argc, argv, prefix, repo, refs_list_usage);
> }
>
> +static int cmd_refs_exists(int argc, const char **argv, const char *prefix,
> + struct repository *repo UNUSED)
> +{
> + struct strbuf unused_referent = STRBUF_INIT;
> + struct object_id unused_oid;
> + unsigned int unused_type;
> + int failure_errno = 0;
> + const char *ref;
> +Let's drop this empty newline.
Show 15 quoted lines
> + const char * const exists_usage[] = {
> + REFS_EXISTS_USAGE,
> + NULL,
> + };
> + struct option options[] = {
> + OPT_END(),
> + };
> +
> + argc = parse_options(argc, argv, prefix, options, exists_usage, 0);
> + if (!argc)
> + die("'git refs exists' requires a reference");
> +
> + ref = *argv++;
> + if (*argv)
> + die("'git refs exists' requires exactly one reference");We can combine these two error messages to just say `if (argc != 1)`. Also, the strings should be marked for translation.
Show 10 quoted lines
> + if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
> + &unused_oid, &unused_referent, &unused_type,
> + &failure_errno)) {
> + if (failure_errno == ENOENT || failure_errno == EISDIR) {
> + error(_("reference does not exist"));
> + return 2;
> + } else {
> + errno = failure_errno;
> + error_errno(_("failed to look up reference"));
> + return 1;I'd personally prefer to se a common exit path and use `goto` so that one doesn't have to worry about whether or not the `struct strbuf` needs to be free'd in error cases. But I'll leave it up to you to decide whether you want to do this change.
Patrick