Re: [PATCH v3 5/7] refs: add pseudorefs array and iteration functions

From: Kousik Sanagavarapu <five231003@gmail.com>
Date: Feb 5, 2024, 18:55 UTC
Message-ID: <ZcEvLwp0t8-rcyGn@five231003>
In-Reply-To: <20231023221143.72489-6-andy.koppe@gmail.com>

Andy Koppe <andy.koppe@gmail.com> wrote:

Show 12 quoted lines

> Define const array 'pseudorefs' with the names of the pseudorefs that
> are documented in gitrevisions.1, and add functions for_each_pseudoref()
> and refs_for_each_pseudoref() for iterating over them.
> 
> The functions process the pseudorefs in the same way as head_ref() and
> refs_head_ref() process HEAD, invoking an each_ref_fn callback on each
> pseudoref that exists.
> 
> This is in preparation for adding pseudorefs to log decorations.
> 
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---

[...]

Show 21 quoted lines

> +/*
> + * List of documented pseudorefs. This needs to be kept in sync with the list
> + * in Documentation/revisions.txt.
> + */
> +static const char *const pseudorefs[] = {
> +	"FETCH_HEAD",
> +	"ORIG_HEAD",
> +	"MERGE_HEAD",
> +	"REBASE_HEAD",
> +	"CHERRY_PICK_HEAD",
> +	"REVERT_HEAD",
> +	"BISECT_HEAD",
> +	"AUTO_MERGE",
> +};
> +
>  struct ref_namespace_info ref_namespace[] = {
>  	[NAMESPACE_HEAD] = {
>  		.ref = "HEAD",
> @@ -1549,6 +1564,33 @@ int head_ref(each_ref_fn fn, void *cb_data)
>  	return refs_head_ref(get_main_ref_store(the_repository), fn, cb_data);
>  }

The first thing that popped up in my head was "Should we somehow use is_pseudoref_syntax() instead of manually listing these?" (although I read in this thread later that Junio was okay with the listing) but then ...

I thought I saw something similar in some other thread (which entered the mailing list much after this patch series was submitted) ...

	https://lore.kernel.org/git/20231221170715.110565-2-karthik.188@gmail.com/T/

The whole thread is really interesting but some points that are worth to be mentioned in this context are

	" ... Patrick's reftable work based on Han-Wen's work revealed
	the need to treat FETCH_HEAD and MERGE_HEAD as "even more
	pecurilar than pseudorefs" that need different term (tentatively
	called "special refs") ... "

So since we are introducing this array in refs.c, which acts as a "refs API" currently

	"A lot more reasonable thing to do may be to scan the
	$GIT_DIR for files whose name satisfy refs.c:is_pseudoref_syntax()
	and list them, instead of having a hardcoded list of these special
	refs.  In addition, when reftable and other backends that can
	natively store things outside refs/ hierarchy is in use, they ought
	to know what they have so enumerating these would not be an issue
	for them without having such a hardcoded table of names."

All that said, the above mentioned thread led to a series of patches for a different purpose than this [1] (which are currently on their way to "master" according to the latest "What's Cooking" email on Feb 2). The ones that have significance w.r.t. to THIS patch series though, are

	https://lore.kernel.org/git/20240129113527.607022-2-karthik.188@gmail.com/
	https://lore.kernel.org/git/20240129113527.607022-4-karthik.188@gmail.com/

(ignoring the reftable part).

I find these to make sense HERE because using the functions introduced THERE are much more robust when dealing with pseudorefs and can be used HERE.

I haven't given it much thought but I think we would still end up writing "for_each_pseudoref()", although much differently from below (and can't use "refs_for_each_all_refs()" directly) because of how we call this function in PATCH 7/7 when actually doing the decoration - that is the decoration for pseudorefs is different (?)

Another approach would be I think to refactor the whole of how decorations with refs work and somehow use "refs_for_each_all_refs()" with its callback handling how we decorate the various refs - I need to dig deeper :) - since the end goal is to support showing all kinds of refs when showing the log

	$ git log -1 --clear-decorations --oneline master
	2a540e432f (ORIG_HEAD, FETCH_HEAD, upstream/master, upstream/HEAD, master) The thirteenth batch

(with color enabled)

Show 55 quoted lines

> +int refs_for_each_pseudoref(struct ref_store *refs,
> +			    each_ref_fn fn, void *cb_data)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(pseudorefs); i++) {
> +		struct object_id oid;
> +		int flag;
> +
> +		if (refs_resolve_ref_unsafe(refs, pseudorefs[i],
> +					    RESOLVE_REF_READING, &oid, &flag)) {
> +			int ret = fn(pseudorefs[i], &oid, flag, cb_data);
> +
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int for_each_pseudoref(each_ref_fn fn, void *cb_data)
> +{
> +	return refs_for_each_pseudoref(get_main_ref_store(the_repository),
> +				       fn, cb_data);
> +}
> +
>  struct ref_iterator *refs_ref_iterator_begin(
>  		struct ref_store *refs,
>  		const char *prefix,
> diff --git a/refs.h b/refs.h
> index 23211a5ea1..7b55cced31 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -320,6 +320,8 @@ typedef int each_repo_ref_fn(struct repository *r,
>   */
>  int refs_head_ref(struct ref_store *refs,
>  		  each_ref_fn fn, void *cb_data);
> +int refs_for_each_pseudoref(struct ref_store *refs,
> +			    each_ref_fn fn, void *cb_data);
>  int refs_for_each_ref(struct ref_store *refs,
>  		      each_ref_fn fn, void *cb_data);
>  int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
> @@ -334,6 +336,9 @@ int refs_for_each_remote_ref(struct ref_store *refs,
>  /* just iterates the head ref. */
>  int head_ref(each_ref_fn fn, void *cb_data);
>  
> +/* iterates pseudorefs. */
> +int for_each_pseudoref(each_ref_fn fn, void *cb_data);
> +
>  /* iterates all refs. */
>  int for_each_ref(each_ref_fn fn, void *cb_data);
>  
> -- 
> 2.42.GIT

So yeah, I just wanted to point out the above things as we would need to refactor this commit and the commits following this - patches 6/7 and 7/7.

Thanks

[1]: https://lore.kernel.org/git/20240129113527.607022-1-karthik.188@gmail.com/