Re: [PATCH v5 0/5] for-each-ref: add '--include-root-refs' option

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 5944 bytes --]

On Fri, Feb 23, 2024 at 11:01:07AM +0100, Karthik Nayak wrote:
> This is the fifth version of my patch series to print root refs
> in git-for-each-ref(1).
> 
> With the introduction of the reftable backend, it becomes ever
> so important to provide the necessary tooling for printing all refs
> associated with a worktree.
> 
> While regular refs stored within the "refs/" namespace are currently
> supported by multiple commands like git-for-each-ref(1),
> git-show-ref(1). Neither support printing root refs within the worktree.
> 
> This patch series is a follow up to the RFC/discussion we had earlier on
> the list [1].
> 
> The first 4 commits add the required functionality to ensure we can print
> all refs (regular, pseudo, HEAD). The 5th commit modifies the
> git-for-each-ref(1) command to add the "--include-root-refs" command which
> will include HEAD and pseudorefs with regular "refs/" refs.
> 
> [1]: https://lore.kernel.org/git/20231221170715.110565-1-karthik.188@gmail.com/#t
> 
> Changes from v4:
> 1. Fixed erratic whitespace
> 2. Remove braces from single line block
> 3. Starting the comments with a capital and also adding more context.
> 4. Removed a duplicate check.
> 
> Thanks for the reviews.
> 
> Range diff against v4:

The range-diff looks as expected, so this patch series looks good to me.
Thanks!

Patrick

> 1:  98130a7ad7 ! 1:  6016042965 refs: introduce `is_pseudoref()` and `is_headref()`
>     @@ refs.c: static int is_pseudoref_syntax(const char *refname)
>      +
>      +	if (ends_with(refname, "_HEAD")) {
>      +		refs_resolve_ref_unsafe(refs, refname,
>     -+   					RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>     -+   					&oid, NULL);
>     -+   		return !is_null_oid(&oid);
>     ++					RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>     ++					&oid, NULL);
>     ++		return !is_null_oid(&oid);
>      +	}
>      +
>      +	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
>      +		if (!strcmp(refname, irregular_pseudorefs[i])) {
>      +			refs_resolve_ref_unsafe(refs, refname,
>     -+   						RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>     -+   						&oid, NULL);
>     ++						RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>     ++						&oid, NULL);
>      +			return !is_null_oid(&oid);
>      +		}
>      +
> 2:  060ab08af5 = 2:  acaa014841 refs: extract out `loose_fill_ref_dir_regular_file()`
> 3:  40d2375ad9 = 3:  f51c5bc307 refs: introduce `refs_for_each_include_root_refs()`
> 4:  b4b9435505 = 4:  7c004db6e7 ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
> 5:  ee99ac41ae ! 5:  53f6c0a6db for-each-ref: add new option to include root refs
>     @@ builtin/for-each-ref.c: int cmd_for_each_ref(int argc, const char **argv, const
>       		filter.name_patterns = argv;
>       	}
>       
>     -+	if (include_root_refs) {
>     ++	if (include_root_refs)
>      +		flags |= FILTER_REFS_ROOT_REFS;
>     -+	}
>      +
>       	filter.match_as_path = 1;
>      -	filter_and_format_refs(&filter, FILTER_REFS_REGULAR, sorting, &format);
>     @@ ref-filter.c: static int for_each_fullref_in_pattern(struct ref_filter *filter,
>       				       void *cb_data)
>       {
>      +	if (filter->kind == FILTER_REFS_KIND_MASK) {
>     -+		/* in this case, we want to print all refs including root refs. */
>     ++		/* In this case, we want to print all refs including root refs. */
>      +		return refs_for_each_include_root_refs(get_main_ref_store(the_repository),
>      +						       cb, cb_data);
>      +	}
>     @@ ref-filter.c: static struct ref_array_item *apply_ref_filter(const char *refname
>       
>       	/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
>       	kind = filter_ref_kind(filter, refname);
>     +-	if (!(kind & filter->kind))
>      +
>      +	/*
>     -+	 * When printing HEAD with all other refs, we want to apply the same formatting
>     -+	 * rules as the other refs, so we simply ask it to be treated as a pseudoref.
>     ++	 * Generally HEAD refs are printed with special description denoting a rebase,
>     ++	 * detached state and so forth. This is useful when only printing the HEAD ref
>     ++	 * But when it is being printed along with other pseudorefs, it makes sense to
>     ++	 * keep the formatting consistent. So we mask the type to act like a pseudoref.
>      +	 */
>      +	if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD)
>      +		kind = FILTER_REFS_PSEUDOREFS;
>      +	else if (!(kind & filter->kind))
>     -+		return NULL;
>     -+
>     - 	if (!(kind & filter->kind))
>       		return NULL;
>       
>     + 	if (!filter_pattern_match(filter, refname))
>      @@ ref-filter.c: static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
>       			ret = for_each_fullref_in("refs/tags/", fn, cb_data);
>       		else if (filter->kind & FILTER_REFS_REGULAR)
> 
> 
> Karthik Nayak (5):
>   refs: introduce `is_pseudoref()` and `is_headref()`
>   refs: extract out `loose_fill_ref_dir_regular_file()`
>   refs: introduce `refs_for_each_include_root_refs()`
>   ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
>   for-each-ref: add new option to include root refs
> 
>  Documentation/git-for-each-ref.txt |   5 +-
>  builtin/for-each-ref.c             |  10 ++-
>  ref-filter.c                       |  30 ++++++-
>  ref-filter.h                       |   7 +-
>  refs.c                             |  48 +++++++++++
>  refs.h                             |   9 ++
>  refs/files-backend.c               | 127 +++++++++++++++++++++--------
>  refs/refs-internal.h               |   6 ++
>  refs/reftable-backend.c            |  11 ++-
>  t/t6302-for-each-ref-filter.sh     |  31 +++++++
>  10 files changed, 237 insertions(+), 47 deletions(-)
> 
> -- 
> 2.43.GIT
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]