Re: [GSoC][PATCH 1/2] builtin/refs: add 'exists' subcommand

[Patrick Steinhardt <ps@pks.im>]

On Thu, Aug 21, 2025 at 02:22:45PM +0530, Meet Soni wrote:
> 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.

> 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.

> +	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.

> +	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