From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 24 Sep 2025 06:32:43 GMT
Subject: Re: [GSoC][PATCH] builtin/refs: add 'get' subcommand
Message-ID: <aNOQi04mS0uXD4iv@pks.im>
In-Reply-To: <20250923104533.21165-1-meetsoni3017@gmail.com>

On Tue, Sep 23, 2025 at 04:15:33PM +0530, Meet Soni wrote:
> diff --git a/Documentation/git-refs.adoc b/Documentation/git-refs.adoc
> index bfa9b3ea2d..f07fe8c864 100644
> --- a/Documentation/git-refs.adoc
> +++ b/Documentation/git-refs.adoc
> @@ -19,6 +19,7 @@ git refs list [--count=<count>] [--shell|--perl|--python|--tcl]
>  		   [(--exclude=<pattern>)...] [--start-after=<marker>]
>  		   [ --stdin | (<pattern>...)]
>  git refs exists <ref>
> +git refs get <ref>
>  
>  DESCRIPTION
>  -----------
> @@ -45,6 +46,12 @@ exists::
>  	failed with an error other than the reference being missing. This does
>  	not verify whether the reference resolves to an actual object.
>  
> +get::
> +	Reads the raw value of a single, exact reference. Instead of

Let's say "fully qualified" instead of "exact".

> +	recursively dereferencing symbolic references, this command prints the
> +	direct target of the symref (e.g., ref: refs/heads/main). For regular
> +	references, it prints the object ID (SHA-1) they point to.

I'd drop the reference to SHA1 here, as it may be any object hash.

> diff --git a/builtin/refs.c b/builtin/refs.c
> index 91548783b7..b473a78e18 100644
> --- a/builtin/refs.c
> +++ b/builtin/refs.c
> @@ -159,6 +163,43 @@ static int cmd_refs_exists(int argc, const char **argv, const char *prefix,
>  	return ret;
>  }
>  
> +static int cmd_refs_get(int argc, const char **argv, const char *prefix,
> +			struct repository *repo UNUSED)
> +{
> +	const char *refname;
> +	struct object_id oid;
> +	unsigned int type;
> +	int failure_errno = 0;
> +	struct strbuf referent = STRBUF_INIT;

Tiny nit: we typically order variables that aren't accessed by options
after the options array.

> +	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 != 1)
> +		die("refs get requires exactly one reference");

This should be translatable. Furthermore, we can probably use `usagef()`
instead to have a "usage:" prefix instead of "fatal:".

> +	refname = *argv++;
> +	if (refs_read_raw_ref(get_main_ref_store(the_repository), refname,
> +			      &oid, &referent, &type, &failure_errno)) {
> +		die("'%s' - not a valid ref", refname);

We should discern by `failure_errno` here. Most importantly, I think we
should handle `ENOENT` and `EISDIR` specially to both mean that the
reference does not exist. So, e.g.:

	if (refs_read_raw_ref(get_main_ref_store(the_repository), refname,
			      &oid, &referent, &type, &failure_errno)) {
		if (failure_errno == ENOENT || failure_errno == EISDIR)
			die(_("reference does not exist"));
		else
			die_errno(_("failed to look up reference"));
	}

> +	}
> +
> +	if (type & REF_ISSYMREF) {
> +		printf("ref: %s\n", referent.buf);
> +	} else {
> +		printf("%s\n", oid_to_hex(&oid));
> +	}

We can drop the curly braces around single-line bodies.

Patrick