[PATCH 0/5] for-each-ref: print all refs on empty string pattern

[PATCH 0/5] for-each-ref: print all refs on empty string pattern

[Karthik Nayak]

With the upcoming introduction of the reftable backend, it becomes ever
so important to provide the necessary tooling for printing all refs
associated with a repository.

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 all the operational refs
within the repository.

This is crucial because with the reftable backend, all these refs will
also move to reftable. It would be necessary to identify all the refs
that are stored within the reftable since there is no easy way to do so
otherwise. This is because the reftable itself is a binary format and
all access will be via git. Unlike the filesystem backend, which allows
access directly via the filesystem.

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 print all refs when an empty string pattern
is used. This is a deviation from the current situation wherein the empty
string pattern currently matches and prints no refs.

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

Karthik Nayak (5):
  refs: expose `is_pseudoref_syntax()`
  refs: make `is_pseudoref_syntax()` stricter
  refs: extract out `loose_fill_ref_dir_regular_file()`
  refs: introduce `refs_for_each_all_refs()`
  for-each-ref: avoid filtering on empty pattern

 Documentation/git-for-each-ref.txt |   3 +-
 builtin/for-each-ref.c             |  21 ++++-
 ref-filter.c                       |  13 ++-
 ref-filter.h                       |   4 +-
 refs.c                             |  32 ++++++--
 refs.h                             |  12 +++
 refs/files-backend.c               | 126 +++++++++++++++++++++--------
 refs/refs-internal.h               |   7 ++
 t/t1407-worktree-ref-store.sh      |  12 +--
 t/t6302-for-each-ref-filter.sh     |  34 ++++++++
 10 files changed, 214 insertions(+), 50 deletions(-)

-- 
2.43.GIT

[PATCH 1/5] refs: expose `is_pseudoref_syntax()`

[Karthik Nayak]

The `is_pseudoref_syntax()` function is static, since it is only used
within `refs.c`. In the following commit, we will use this function to
provide an utility to add pseudorefs to the loose refs cache. So let's
expose this function via `refs.h`.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c | 2 +-
 refs.h | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 2f58a3460a..5999605230 100644
--- a/refs.c
+++ b/refs.c
@@ -827,7 +827,7 @@ int is_per_worktree_ref(const char *refname)
 	       starts_with(refname, "refs/rewritten/");
 }
 
-static int is_pseudoref_syntax(const char *refname)
+int is_pseudoref_syntax(const char *refname)
 {
 	const char *c;
 
diff --git a/refs.h b/refs.h
index ff113bb12a..f1bbad83fb 100644
--- a/refs.h
+++ b/refs.h
@@ -846,6 +846,12 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs);
 /* Is this a per-worktree ref living in the refs/ namespace? */
 int is_per_worktree_ref(const char *refname);
 
+/*
+ * Check whether a refname matches the pseudoref syntax. This is a surface
+ * level check and can present false positives.
+ */
+int is_pseudoref_syntax(const char *refname);
+
 /* Describes how a refname relates to worktrees */
 enum ref_worktree_type {
 	REF_WORKTREE_CURRENT, /* implicitly per worktree, eg. HEAD or
-- 
2.43.GIT

Re: [PATCH 1/5] refs: expose `is_pseudoref_syntax()`

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> The `is_pseudoref_syntax()` function is static, since it is only used
> within `refs.c`. In the following commit, we will use this function to
> provide an utility to add pseudorefs to the loose refs cache. So let's
> expose this function via `refs.h`.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c | 2 +-
>  refs.h | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 2f58a3460a..5999605230 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -827,7 +827,7 @@ int is_per_worktree_ref(const char *refname)
>  	       starts_with(refname, "refs/rewritten/");
>  }
>  
> -static int is_pseudoref_syntax(const char *refname)
> +int is_pseudoref_syntax(const char *refname)
>  {
>  	const char *c;
>  
> diff --git a/refs.h b/refs.h
> index ff113bb12a..f1bbad83fb 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -846,6 +846,12 @@ const char **hidden_refs_to_excludes(const struct strvec *hide_refs);
>  /* Is this a per-worktree ref living in the refs/ namespace? */
>  int is_per_worktree_ref(const char *refname);
>  
> +/*
> + * Check whether a refname matches the pseudoref syntax. This is a surface
> + * level check and can present false positives.
> + */

What does "false positive" mean in this context?

is_pseudoref_syntax("FOO_HEAD") says "true", and then if it is
"false positive", that would mean "FOO_HEAD" is not a pseudo ref,
right?  What can a caller of this function do to deal with a false
positive?

Or do you mean "FOO_HEAD" is still a pseudo ref, but it may not
currently exist?  That is different from "false positive".

As the check is about "does it match the pseudoref syntax?", I would
understand if what you wanted to say was something like: This only
checks the syntax, and such a pseudoref may not currently exist in
the repository---for that you'd need to call read_ref_full() or
other ref API functions.

Puzzled...

Thanks.

Re: [PATCH 1/5] refs: expose `is_pseudoref_syntax()`

[Karthik Nayak]

On Fri, Jan 19, 2024 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > +/*
> > + * Check whether a refname matches the pseudoref syntax. This is a surface
> > + * level check and can present false positives.
> > + */
>
> What does "false positive" mean in this context?
>
> is_pseudoref_syntax("FOO_HEAD") says "true", and then if it is
> "false positive", that would mean "FOO_HEAD" is not a pseudo ref,
> right?  What can a caller of this function do to deal with a false
> positive?
>
> Or do you mean "FOO_HEAD" is still a pseudo ref, but it may not
> currently exist?  That is different from "false positive".
>

Yes, I think this is what I wanted to say and what you say below is what I'll
change it to.

> As the check is about "does it match the pseudoref syntax?", I would
> understand if what you wanted to say was something like: This only
> checks the syntax, and such a pseudoref may not currently exist in
> the repository---for that you'd need to call read_ref_full() or
> other ref API functions.
>
> Puzzled...
>
> Thanks.

Thanks!

[PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Karthik Nayak]

The `is_pseudoref_syntax()` function is used to determine if a
particular refname follows the pseudoref syntax. The pseudoref syntax is
loosely defined at this instance as all refs which are in caps and use
underscores. Most of the pseudorefs also have the "HEAD" suffix.

Using this information we make the `is_pseudoref_syntax()` function
stricter, by adding the check for "HEAD" suffix and for refs which don't
end with the HEAD suffix, matching them with a predetermined list.

This requires fixing up t1407 to use the "HEAD" suffix for creation of
pseudorefs.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c                        | 21 ++++++++++++++++++---
 t/t1407-worktree-ref-store.sh | 12 ++++++------
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 5999605230..b84e173762 100644
--- a/refs.c
+++ b/refs.c
@@ -829,6 +829,14 @@ int is_per_worktree_ref(const char *refname)
 
 int is_pseudoref_syntax(const char *refname)
 {
+	/* TODO: move these pseudorefs to have _HEAD suffix */
+	static const char *const irregular_pseudorefs[] = {
+		"BISECT_EXPECTED_REV",
+		"NOTES_MERGE_PARTIAL",
+		"NOTES_MERGE_REF",
+		"AUTO_MERGE"
+	};
+	size_t i;
 	const char *c;
 
 	for (c = refname; *c; c++) {
@@ -837,10 +845,17 @@ int is_pseudoref_syntax(const char *refname)
 	}
 
 	/*
-	 * HEAD is not a pseudoref, but it certainly uses the
-	 * pseudoref syntax.
+	 * Most pseudorefs end with _HEAD. HEAD itself is not a
+	 * pseudoref, but it certainly uses the pseudoref syntax.
 	 */
-	return 1;
+	if (ends_with(refname, "HEAD"))
+		return 1;
+
+	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
+		if (!strcmp(refname, irregular_pseudorefs[i]))
+			return 1;
+
+	return 0;
 }
 
 static int is_current_worktree_ref(const char *ref) {
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index 05b1881c59..53592c95f3 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -61,18 +61,18 @@ test_expect_success 'create_symref(FOO, refs/heads/main)' '
 # PSEUDO-WT and refs/bisect/random do not create reflogs by default, so it is
 # not testing a realistic scenario.
 test_expect_success REFFILES 'for_each_reflog()' '
-	echo $ZERO_OID > .git/logs/PSEUDO-MAIN &&
+	echo $ZERO_OID >.git/logs/PSEUDO_MAIN_HEAD &&
 	mkdir -p     .git/logs/refs/bisect &&
-	echo $ZERO_OID > .git/logs/refs/bisect/random &&
+	echo $ZERO_OID >.git/logs/refs/bisect/random &&
 
-	echo $ZERO_OID > .git/worktrees/wt/logs/PSEUDO-WT &&
+	echo $ZERO_OID >.git/worktrees/wt/logs/PSEUDO_WT_HEAD &&
 	mkdir -p     .git/worktrees/wt/logs/refs/bisect &&
-	echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&
+	echo $ZERO_OID >.git/worktrees/wt/logs/refs/bisect/wt-random &&
 
 	$RWT for-each-reflog | cut -d" " -f 2- | sort >actual &&
 	cat >expected <<-\EOF &&
 	HEAD 0x1
-	PSEUDO-WT 0x0
+	PSEUDO_WT_HEAD 0x0
 	refs/bisect/wt-random 0x0
 	refs/heads/main 0x0
 	refs/heads/wt-main 0x0
@@ -82,7 +82,7 @@ test_expect_success REFFILES 'for_each_reflog()' '
 	$RMAIN for-each-reflog | cut -d" " -f 2- | sort >actual &&
 	cat >expected <<-\EOF &&
 	HEAD 0x1
-	PSEUDO-MAIN 0x0
+	PSEUDO_MAIN_HEAD 0x0
 	refs/bisect/random 0x0
 	refs/heads/main 0x0
 	refs/heads/wt-main 0x0
-- 
2.43.GIT

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Using this information we make the `is_pseudoref_syntax()` function
> stricter, by adding the check for "HEAD" suffix and for refs which don't
> end with the HEAD suffix, matching them with a predetermined list.

OK, so this partly answers my question on the previous step.  Before
making it more strict, the function worked only on the "syntax", so
a random string that can be a pseudo ref passed the check.

But stepping back a bit, if we call this function is_pseudoref_syntax(),
wouldn't it be what we want to have anyway?  You seem to want a
separate function called is_pseudoref() that rejects bogus uppercase
string "FOO_BAR" while accepting the known-good pseudoref you add
tests for, plus the ${FOO}_HEAD for any value of ${FOO} we currently
have and we may want to add in the future.

>  int is_pseudoref_syntax(const char *refname)
>  {
> +	/* TODO: move these pseudorefs to have _HEAD suffix */
> +	static const char *const irregular_pseudorefs[] = {
> +		"BISECT_EXPECTED_REV",
> +		"NOTES_MERGE_PARTIAL",
> +		"NOTES_MERGE_REF",
> +		"AUTO_MERGE"
> +	};
> +	size_t i;
>  	const char *c;
>  
>  	for (c = refname; *c; c++) {
> @@ -837,10 +845,17 @@ int is_pseudoref_syntax(const char *refname)
>  	}
>  
>  	/*
> -	 * HEAD is not a pseudoref, but it certainly uses the
> -	 * pseudoref syntax.
> +	 * Most pseudorefs end with _HEAD. HEAD itself is not a
> +	 * pseudoref, but it certainly uses the pseudoref syntax.
>  	 */
> -	return 1;
> +	if (ends_with(refname, "HEAD"))
> +		return 1;

I would imagine that at the final stage in which something like this
will be named is_pseudoref(), asking is_pseudoref("HEAD") would
return "No" (even though "is_pseudoref_syntax()", if the helper
function remains, may say "Yes" to "HEAD").  And this ends_with()
will use "_HEAD", instead of "HEAD".  But I am reading ahead of
myself, so let's keep going.

> +	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
> +		if (!strcmp(refname, irregular_pseudorefs[i]))
> +			return 1;
> +
> +	return 0;
>  }

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Phillip Wood]

Hi Karthik

On 19/01/2024 14:27, Karthik Nayak wrote:
> The `is_pseudoref_syntax()` function is used to determine if a
> particular refname follows the pseudoref syntax. The pseudoref syntax is
> loosely defined at this instance as all refs which are in caps and use
> underscores. Most of the pseudorefs also have the "HEAD" suffix.
> 
> Using this information we make the `is_pseudoref_syntax()` function
> stricter, by adding the check for "HEAD" suffix and for refs which don't
> end with the HEAD suffix, matching them with a predetermined list.
> 
> This requires fixing up t1407 to use the "HEAD" suffix for creation of
> pseudorefs.

I'm concerned that this change is a regression. is_pseudoref_syntax() is 
used by is_current_worktree_ref() and so scripts that create pseudorefs 
that do not conform to your new rules will break as git will no-longer 
consider the pseudorefs they create to be worktree specific. The list of 
hard coded exceptions also looks quite short, I can see MERGE_AUTOSTASH 
and BISECT_START are missing and there are probably others I've not 
thought of.

The commit message would be a good place to discuss why you're making 
this change, the implications of the change and any alternative 
approaches that you considered. As I understand it you're tying to get 
round the problem that the files backend stores pseudorefs mixed up with 
other non-ref files in $GIT_DIR. Another approach would be to read all 
the files whose name matches the pseudoref syntax and see if its 
contents looks like a valid ref skipping names like COMMIT_EDITMSG that 
we know are not pseudorefs.

Best Wishes

Phillip

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>   refs.c                        | 21 ++++++++++++++++++---
>   t/t1407-worktree-ref-store.sh | 12 ++++++------
>   2 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 5999605230..b84e173762 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -829,6 +829,14 @@ int is_per_worktree_ref(const char *refname)
>   
>   int is_pseudoref_syntax(const char *refname)
>   {
> +	/* TODO: move these pseudorefs to have _HEAD suffix */
> +	static const char *const irregular_pseudorefs[] = {
> +		"BISECT_EXPECTED_REV",
> +		"NOTES_MERGE_PARTIAL",
> +		"NOTES_MERGE_REF",
> +		"AUTO_MERGE"
> +	};
> +	size_t i;
>   	const char *c;
>   
>   	for (c = refname; *c; c++) {
> @@ -837,10 +845,17 @@ int is_pseudoref_syntax(const char *refname)
>   	}
>   
>   	/*
> -	 * HEAD is not a pseudoref, but it certainly uses the
> -	 * pseudoref syntax.
> +	 * Most pseudorefs end with _HEAD. HEAD itself is not a
> +	 * pseudoref, but it certainly uses the pseudoref syntax.
>   	 */
> -	return 1;
> +	if (ends_with(refname, "HEAD"))
> +		return 1;
> +
> +	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
> +		if (!strcmp(refname, irregular_pseudorefs[i]))
> +			return 1;
> +
> +	return 0;
>   }
>   
>   static int is_current_worktree_ref(const char *ref) {
> diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
> index 05b1881c59..53592c95f3 100755
> --- a/t/t1407-worktree-ref-store.sh
> +++ b/t/t1407-worktree-ref-store.sh
> @@ -61,18 +61,18 @@ test_expect_success 'create_symref(FOO, refs/heads/main)' '
>   # PSEUDO-WT and refs/bisect/random do not create reflogs by default, so it is
>   # not testing a realistic scenario.
>   test_expect_success REFFILES 'for_each_reflog()' '
> -	echo $ZERO_OID > .git/logs/PSEUDO-MAIN &&
> +	echo $ZERO_OID >.git/logs/PSEUDO_MAIN_HEAD &&
>   	mkdir -p     .git/logs/refs/bisect &&
> -	echo $ZERO_OID > .git/logs/refs/bisect/random &&
> +	echo $ZERO_OID >.git/logs/refs/bisect/random &&
>   
> -	echo $ZERO_OID > .git/worktrees/wt/logs/PSEUDO-WT &&
> +	echo $ZERO_OID >.git/worktrees/wt/logs/PSEUDO_WT_HEAD &&
>   	mkdir -p     .git/worktrees/wt/logs/refs/bisect &&
> -	echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&
> +	echo $ZERO_OID >.git/worktrees/wt/logs/refs/bisect/wt-random &&
>   
>   	$RWT for-each-reflog | cut -d" " -f 2- | sort >actual &&
>   	cat >expected <<-\EOF &&
>   	HEAD 0x1
> -	PSEUDO-WT 0x0
> +	PSEUDO_WT_HEAD 0x0
>   	refs/bisect/wt-random 0x0
>   	refs/heads/main 0x0
>   	refs/heads/wt-main 0x0
> @@ -82,7 +82,7 @@ test_expect_success REFFILES 'for_each_reflog()' '
>   	$RMAIN for-each-reflog | cut -d" " -f 2- | sort >actual &&
>   	cat >expected <<-\EOF &&
>   	HEAD 0x1
> -	PSEUDO-MAIN 0x0
> +	PSEUDO_MAIN_HEAD 0x0
>   	refs/bisect/random 0x0
>   	refs/heads/main 0x0
>   	refs/heads/wt-main 0x0

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I'm concerned that this change is a regression. is_pseudoref_syntax()
> is used by is_current_worktree_ref() and so scripts that create
> pseudorefs that do not conform to your new rules will break as git
> will no-longer consider the pseudorefs they create to be worktree
> specific.

Ideally, when the exception list in the function becomes more
complete, those "pseudorefs" created by those scripts shouldn't
probably be created either as common or worktree specific thing
if they are not "pseudoref".

> The list of hard coded exceptions also looks quite short, I
> can see MERGE_AUTOSTASH and BISECT_START are missing and there are
> probably others I've not thought of.

I agree that it is something we need to fix.

> The commit message would be a good place to discuss why you're making
> this change, the implications of the change and any alternative
> approaches that you considered. As I understand it you're tying to get
> round the problem that the files backend stores pseudorefs mixed up
> with other non-ref files in $GIT_DIR.

Yup.  The rationale may want to be explained better.

> Another approach would be to
> read all the files whose name matches the pseudoref syntax and see if
> its contents looks like a valid ref skipping names like COMMIT_EDITMSG
> that we know are not pseudorefs.

In the longer term, I'd prefer to see a simpler rule, like "all-caps
or underscore string, ending with _HEAD and nothing else are the
pseudorefs but we have these small number of exceptions that are
grandfathered".

Thanks.

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Phillip Wood]

Hi Junio

On 22/01/2024 20:22, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> I'm concerned that this change is a regression. is_pseudoref_syntax()
>> is used by is_current_worktree_ref() and so scripts that create
>> pseudorefs that do not conform to your new rules will break as git
>> will no-longer consider the pseudorefs they create to be worktree
>> specific.
> 
> Ideally, when the exception list in the function becomes more
> complete, those "pseudorefs" created by those scripts shouldn't
> probably be created either as common or worktree specific thing
> if they are not "pseudoref".

I not sure I quite understand what you mean here. Are you saying that 
scripts should stop using "git update-ref" and "git rev-parse" for 
anything that does not match the new pseudoref syntax?

>> Another approach would be to
>> read all the files whose name matches the pseudoref syntax and see if
>> its contents looks like a valid ref skipping names like COMMIT_EDITMSG
>> that we know are not pseudorefs.
> 
> In the longer term, I'd prefer to see a simpler rule, like "all-caps
> or underscore string, ending with _HEAD and nothing else are the
> pseudorefs but we have these small number of exceptions that are
> grandfathered".

Hopefully such a rule would stop us adding pseudorefs that are really 
private state like MERGE_AUTOSTASH. I think that is good in the long 
term but isn't it is happening now with this patch without any warning 
to users? This patch changes the behavior of parse_worktree_ref() which 
the files backend uses to figure out the path it should use when reading 
and writing a ref.

Best Wishes

Phillip

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Karthik Nayak]

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

Hello Phillip,

Phillip Wood <phillip.wood123@gmail.com> writes:
>
> Hopefully such a rule would stop us adding pseudorefs that are really
> private state like MERGE_AUTOSTASH. I think that is good in the long
> term but isn't it is happening now with this patch without any warning
> to users? This patch changes the behavior of parse_worktree_ref() which
> the files backend uses to figure out the path it should use when reading
> and writing a ref.
>

I do agree with the problem you're outlining here. Changing
`is_pseudoref_syntax()` does indeed break things since its also used by
`parse_worktree_ref()`.

I first thought I could get around this by adding the required missing
refs, but even that wouldn't work. Because BISECT_START has dual nature,
it act as a ref and also as file storing a branch name as Patrick
mentions in detail in his email [1]. Meaning if `is_pseudoref_syntax()`
identifies it as a pseudoref, it could be wrong and printing it as such
might not work. But we can't not match it because that is the current
expectation.

So there is no way to make `is_pseudoref_syntax()` stricter without
breaking backward compatibility. While we do want to reach that goal, we
have to go about in the other way around, that i.e.
1. Fix all pseudorefs to have the '_HEAD' suffix.
2. Move bisect files to '$GIT_DIR/bisect-state' (see [1] for more
details).
After this, we can safely make `is_pseudoref_syntax()` stricter.

Given this, I think for the next version, I'll do the following changes:
1. keep `is_pseudoref_syntax()` as is.
2. introduce `is_pseudoref()` which calls `is_pseudoref_syntax()` and
also checks the content of the file.
3. replace use of `is_pseudoref_syntax()` with `is_pseudoref()` in this
patch series.

[1]: https://public-inbox.org/git/20240119142705.139374-1-karthik.188@gmail.com/T/#m6e3790e30613fd68349708faaf5f4d9c704ba677

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

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[phillip.wood123]

Hi Karthik

On 23/01/2024 12:49, Karthik Nayak wrote:
> Hello Phillip,
> 
> Phillip Wood <phillip.wood123@gmail.com> writes:
> [...]
> So there is no way to make `is_pseudoref_syntax()` stricter without
> breaking backward compatibility. While we do want to reach that goal, we
> have to go about in the other way around, that i.e.
> 1. Fix all pseudorefs to have the '_HEAD' suffix.

I'm wary of renaming user facing pseudorefs like AUTO_MERGE. In the 
future we should try and avoid adding any without a "_HEAD" suffix

> 2. Move bisect files to '$GIT_DIR/bisect-state' (see [1] for more
> details).
> After this, we can safely make `is_pseudoref_syntax()` stricter.
> 
> Given this, I think for the next version, I'll do the following changes:
> 1. keep `is_pseudoref_syntax()` as is.
> 2. introduce `is_pseudoref()` which calls `is_pseudoref_syntax()` and
> also checks the content of the file.

We could perhaps make is_pseudoref() stricter from the start as we're 
adding a new function, it could have a list of exceptions to the "a 
pseudoref must end with '_HEAD'" rule like this patch. Being strict 
about the pseudorefs we list with "git for-each-ref" might encourage 
users to update any scripts they have that create "pseudorefs" that do 
not match the new rules without us making any changes that actually 
break those scripts.

> 3. replace use of `is_pseudoref_syntax()` with `is_pseudoref()` in this
> patch series.

That sounds like a good way forward to me, lets see if Junio agrees.

Best Wishes

Phillip


> [1]: https://public-inbox.org/git/20240119142705.139374-1-karthik.188@gmail.com/T/#m6e3790e30613fd68349708faaf5f4d9c704ba677

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Given this, I think for the next version, I'll do the following changes:
> 1. keep `is_pseudoref_syntax()` as is.
> 2. introduce `is_pseudoref()` which calls `is_pseudoref_syntax()` and
> also checks the content of the file.
> 3. replace use of `is_pseudoref_syntax()` with `is_pseudoref()` in this
> patch series.

The content check in 2. was something that was mentioned in an
earlier discussion, lack of which I completely missed during the
review of this current round.  Sounds very good to add that.

Thanks.

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I not sure I quite understand what you mean here. Are you saying that
> scripts should stop using "git update-ref" and "git rev-parse" for
> anything that does not match the new pseudoref syntax?

Yes, I am saying that, and also that we should have been stricter on
what we accept and consider as pseudorefs---not just any file that
sits directly under $GIT_DIR/., but ideally we should have limited
to "^[A-Z]*_HEAD$" or something.  The idea I am floating is to see if
such a tightening can be done now without hearing too many screams.

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Patrick Steinhardt]

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

On Mon, Jan 22, 2024 at 12:22:49PM -0800, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> > The list of hard coded exceptions also looks quite short, I
> > can see MERGE_AUTOSTASH and BISECT_START are missing and there are
> > probably others I've not thought of.
> 
> I agree that it is something we need to fix.

I've taken a deeper look at BISECT_START because I previously missed it
in my conversion to make special refs become normal pseudo refs. But as
it turns out, BISECT_START is not a ref at all.

Depending on how you execute git-bisect(1), it will either contain the
object ID of the detached HEAD or the branch you're starting the bisect
from. This information is used to switch back to that state when you
abort the bisect. So far this sounds like a proper ref indeed. But in
case you're starting from a branch it will not be a symref that points
to this branch, but it will just contain the branch name. This is not a
valid format that could be read as a loose ref, and thus this file is
not a proper ref at all (except that sometimes it behaves like one when
starting from a detached HEAD).

My first hunch was to convert it so that it indeed always is a proper
ref. But thinking about it a bit more I'm less convinced that this is
sensible as it is deeply tied to the behaviour of git-bisect(1) and only
represents its internal state. I thus came to the conclusion that it is
more similar to the sequencer state that we have in ".git/rebase-merge"
and ".git/rebase-apply" than anything else.

So if we wanted to rectify this, I think the most sensible way to
address this would be to introduce a new ".git/bisect-state" directory
that contains all of git-bisect(1)'s state:

    - BISECT_TERMS -> bisect-state/terms
    - BISECT_LOG -> bisect-state/log
    - BISECT_START -> bisect-state/start
    - BISECT_RUN -> bisect-state/run
    - BISECT_FIRST_PARENT -> bisect-state/first-parent
    - BISECT_ANCESTORS_OK -> bisect-state/ancestors-ok

I think this would make for a much cleaner solution overall as things
are neatly contained. Cleaning up after a bisect would thus only require
a delete of ".git/bisect-state/" and we're done.

Of course, this would be a backwards-incompatible change. We could
transition to that newer schema by having newer Git versions recognize
both ways to store the state, but only ever write the new schema. But
I'm not sure whether it would ultimately be worth it.

Patrick

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

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Phillip Wood]

Hi Patrick

On 23/01/2024 11:16, Patrick Steinhardt wrote:
> On Mon, Jan 22, 2024 at 12:22:49PM -0800, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>> The list of hard coded exceptions also looks quite short, I
>>> can see MERGE_AUTOSTASH and BISECT_START are missing and there are
>>> probably others I've not thought of.
>>
>> I agree that it is something we need to fix.
> 
> I've taken a deeper look at BISECT_START because I previously missed it
> in my conversion to make special refs become normal pseudo refs. But as
> it turns out, BISECT_START is not a ref at all.
> > Depending on how you execute git-bisect(1), it will either contain the
> object ID of the detached HEAD or the branch you're starting the bisect
> from. This information is used to switch back to that state when you
> abort the bisect. So far this sounds like a proper ref indeed. But in
> case you're starting from a branch it will not be a symref that points
> to this branch, but it will just contain the branch name. This is not a
> valid format that could be read as a loose ref, and thus this file is
> not a proper ref at all (except that sometimes it behaves like one when
> starting from a detached HEAD).

Thank you, I'd missed that

> My first hunch was to convert it so that it indeed always is a proper
> ref. But thinking about it a bit more I'm less convinced that this is
> sensible as it is deeply tied to the behaviour of git-bisect(1) and only
> represents its internal state. I thus came to the conclusion that it is
> more similar to the sequencer state that we have in ".git/rebase-merge"
> and ".git/rebase-apply" than anything else.
> 
> So if we wanted to rectify this, I think the most sensible way to
> address this would be to introduce a new ".git/bisect-state" directory
> that contains all of git-bisect(1)'s state:
> 
>      - BISECT_TERMS -> bisect-state/terms
>      - BISECT_LOG -> bisect-state/log
>      - BISECT_START -> bisect-state/start
>      - BISECT_RUN -> bisect-state/run
>      - BISECT_FIRST_PARENT -> bisect-state/first-parent
>      - BISECT_ANCESTORS_OK -> bisect-state/ancestors-ok
> 
> I think this would make for a much cleaner solution overall as things
> are neatly contained. Cleaning up after a bisect would thus only require
> a delete of ".git/bisect-state/" and we're done.
> 
> Of course, this would be a backwards-incompatible change. We could
> transition to that newer schema by having newer Git versions recognize
> both ways to store the state, but only ever write the new schema. But
> I'm not sure whether it would ultimately be worth it.

I think that is a really good suggestion, it would bring bisect into 
line with am, rebase, cherry-pick etc. which keep their state in a 
subdirectory rather than cluttering up .git.

Best Wishes

Phillip

> Patrick

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> My first hunch was to convert it so that it indeed always is a proper
> ref. But thinking about it a bit more I'm less convinced that this is
> sensible as it is deeply tied to the behaviour of git-bisect(1) and only
> represents its internal state. I thus came to the conclusion that it is
> more similar to the sequencer state that we have in ".git/rebase-merge"
> and ".git/rebase-apply" than anything else.

Fair enough.

> So if we wanted to rectify this, I think the most sensible way to
> address this would be to introduce a new ".git/bisect-state" directory
> that contains all of git-bisect(1)'s state:
>
>     - BISECT_TERMS -> bisect-state/terms
>     - BISECT_LOG -> bisect-state/log
>     - BISECT_START -> bisect-state/start
>     - BISECT_RUN -> bisect-state/run
>     - BISECT_FIRST_PARENT -> bisect-state/first-parent
>     - BISECT_ANCESTORS_OK -> bisect-state/ancestors-ok
>
> I think this would make for a much cleaner solution overall as things
> are neatly contained. Cleaning up after a bisect would thus only require
> a delete of ".git/bisect-state/" and we're done.

And bisect-state/ needs to be marked as per-worktree hierarchy, I suppose.

> Of course, this would be a backwards-incompatible change.

As long as we ignore folks who switches versions of Git in the
middle of their "git bisect" session, we should be OK.

If we really cared the backward compatibility, the new version of
Git that knows and uses this new layout could notice these old-style
filenames and move them over to the new place under new names.  From
there, everything should work (including things like "git bisect log").

Thanks.

Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter

[Patrick Steinhardt]

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

On Tue, Jan 23, 2024 at 09:44:21AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > My first hunch was to convert it so that it indeed always is a proper
> > ref. But thinking about it a bit more I'm less convinced that this is
> > sensible as it is deeply tied to the behaviour of git-bisect(1) and only
> > represents its internal state. I thus came to the conclusion that it is
> > more similar to the sequencer state that we have in ".git/rebase-merge"
> > and ".git/rebase-apply" than anything else.
> 
> Fair enough.
> 
> > So if we wanted to rectify this, I think the most sensible way to
> > address this would be to introduce a new ".git/bisect-state" directory
> > that contains all of git-bisect(1)'s state:
> >
> >     - BISECT_TERMS -> bisect-state/terms
> >     - BISECT_LOG -> bisect-state/log
> >     - BISECT_START -> bisect-state/start
> >     - BISECT_RUN -> bisect-state/run
> >     - BISECT_FIRST_PARENT -> bisect-state/first-parent
> >     - BISECT_ANCESTORS_OK -> bisect-state/ancestors-ok
> >
> > I think this would make for a much cleaner solution overall as things
> > are neatly contained. Cleaning up after a bisect would thus only require
> > a delete of ".git/bisect-state/" and we're done.
> 
> And bisect-state/ needs to be marked as per-worktree hierarchy, I suppose.

Yes, "bisect-state/" would need to be stored in GIT_DIR, not COMMON_DIR.

> > Of course, this would be a backwards-incompatible change.
> 
> As long as we ignore folks who switches versions of Git in the
> middle of their "git bisect" session, we should be OK.
> 
> If we really cared the backward compatibility, the new version of
> Git that knows and uses this new layout could notice these old-style
> filenames and move them over to the new place under new names.  From
> there, everything should work (including things like "git bisect log").

We also have consider that there may be alternate implementations of Git
that would only know to handle the old layout. Those tools would be
broken in case we did such a migration, but they would be broken anyway
if the bisect was started via Git and not via the tool.

Anyway, I'll add this to our growing backlog of issues that we might
want to investigate once the reftable backend has been upstreamed. Which
of course shouldn't preclude anybody else from picking up this topic in
case they are interested.

Patrick

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

[PATCH 3/5] refs: extract out `loose_fill_ref_dir_regular_file()`

[Karthik Nayak]

Extract out the code for adding a single file to the loose ref dir as
`loose_fill_ref_dir_regular_file()` from `loose_fill_ref_dir()` in
`refs/files-backend.c`.

This allows us to use this function independently in the following
commits where we add code to also add pseudorefs to the ref dir.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c | 62 +++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6734f2a309..a4884f557d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -229,6 +229,38 @@ static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dir
 	}
 }
 
+static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
+					    const char *refname,
+					    struct ref_dir *dir)
+{
+	struct object_id oid;
+	int flag;
+
+	if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_READING,
+				     &oid, &flag)) {
+		oidclr(&oid);
+		flag |= REF_ISBROKEN;
+	} else if (is_null_oid(&oid)) {
+		/*
+		 * It is so astronomically unlikely
+		 * that null_oid is the OID of an
+		 * actual object that we consider its
+		 * appearance in a loose reference
+		 * file to be repo corruption
+		 * (probably due to a software bug).
+		 */
+		flag |= REF_ISBROKEN;
+	}
+
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		if (!refname_is_safe(refname))
+			die("loose refname is dangerous: %s", refname);
+		oidclr(&oid);
+		flag |= REF_BAD_NAME | REF_ISBROKEN;
+	}
+	add_entry_to_dir(dir, create_ref_entry(refname, &oid, flag));
+}
+
 /*
  * Read the loose references from the namespace dirname into dir
  * (without recursing).  dirname must end with '/'.  dir must be the
@@ -257,8 +289,6 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 	strbuf_add(&refname, dirname, dirnamelen);
 
 	while ((de = readdir(d)) != NULL) {
-		struct object_id oid;
-		int flag;
 		unsigned char dtype;
 
 		if (de->d_name[0] == '.')
@@ -274,33 +304,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 					 create_dir_entry(dir->cache, refname.buf,
 							  refname.len));
 		} else if (dtype == DT_REG) {
-			if (!refs_resolve_ref_unsafe(&refs->base,
-						     refname.buf,
-						     RESOLVE_REF_READING,
-						     &oid, &flag)) {
-				oidclr(&oid);
-				flag |= REF_ISBROKEN;
-			} else if (is_null_oid(&oid)) {
-				/*
-				 * It is so astronomically unlikely
-				 * that null_oid is the OID of an
-				 * actual object that we consider its
-				 * appearance in a loose reference
-				 * file to be repo corruption
-				 * (probably due to a software bug).
-				 */
-				flag |= REF_ISBROKEN;
-			}
-
-			if (check_refname_format(refname.buf,
-						 REFNAME_ALLOW_ONELEVEL)) {
-				if (!refname_is_safe(refname.buf))
-					die("loose refname is dangerous: %s", refname.buf);
-				oidclr(&oid);
-				flag |= REF_BAD_NAME | REF_ISBROKEN;
-			}
-			add_entry_to_dir(dir,
-					 create_ref_entry(refname.buf, &oid, flag));
+			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
 		}
 		strbuf_setlen(&refname, dirnamelen);
 	}
-- 
2.43.GIT

[PATCH 4/5] refs: introduce `refs_for_each_all_refs()`

[Karthik Nayak]

Introduce a new ref iteration flag `DO_FOR_EACH_INCLUDE_ALL_REFS`, which
will be used to iterate over all refs. In the files backend this is
limited to regular refs, pseudorefs and HEAD. For other backends like
the reftable this is the universal set of all refs stored in the
backend.

Refs which fall outside the `refs/` and aren't either pseudorefs or HEAD
are more of a grey area. This is because we don't block the users from
creating such refs but they are not officially supported. In the files
backend, we cannot isolate such files from other files.

Introduce `refs_for_each_all_refs()` which calls `do_for_each_ref()`
with this newly introduced flag.

In `refs/files-backend.c`, introduce a new function
`add_pseudoref_like_entries()` to add pseudorefs and HEAD to the
`ref_dir`. We then finally call `add_pseudoref_like_entries()` whenever
the `DO_FOR_EACH_INCLUDE_ALL_REFS` flag is set. Any new ref backend will
also have to implement similar changes on its end.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c               |  7 +++++
 refs.h               |  6 +++++
 refs/files-backend.c | 64 ++++++++++++++++++++++++++++++++++++++++----
 refs/refs-internal.h |  7 +++++
 4 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index b84e173762..e527c6199d 100644
--- a/refs.c
+++ b/refs.c
@@ -1722,6 +1722,13 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data);
 }
 
+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
+			   void *cb_data)
+{
+	return do_for_each_ref(refs, "", NULL, fn, 0,
+			       DO_FOR_EACH_INCLUDE_ALL_REFS, cb_data);
+}
+
 static int qsort_strcmp(const void *va, const void *vb)
 {
 	const char *a = *(const char **)va;
diff --git a/refs.h b/refs.h
index f1bbad83fb..fef70b6599 100644
--- a/refs.h
+++ b/refs.h
@@ -393,6 +393,12 @@ int for_each_namespaced_ref(const char **exclude_patterns,
 int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
 int for_each_rawref(each_ref_fn fn, void *cb_data);
 
+/*
+ * Iterates over all ref types, regular, pseudorefs and HEAD.
+ */
+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
+			   void *cb_data);
+
 /*
  * Normalizes partial refs to their fully qualified form.
  * Will prepend <prefix> to the <pattern> if it doesn't start with 'refs/'.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a4884f557d..95a73b11bb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -315,9 +315,58 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 	add_per_worktree_entries_to_dir(dir, dirname);
 }
 
-static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
+/*
+ * Add pseudorefs and HEAD to the ref dir by parsing the directory
+ * for any files which follow the pseudoref syntax.
+ */
+static void add_pseudoref_like_entries(struct ref_store *ref_store,
+					 struct ref_dir *dir,
+					 const char *dirname)
+{
+	struct files_ref_store *refs =
+		files_downcast(ref_store, REF_STORE_READ, "fill_ref_dir");
+	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
+	struct dirent *de;
+	size_t dirnamelen;
+	DIR *d;
+
+	files_ref_path(refs, &path, dirname);
+
+	d = opendir(path.buf);
+	if (!d) {
+		strbuf_release(&path);
+		return;
+	}
+
+	strbuf_addstr(&refname, dirname);
+	dirnamelen = refname.len;
+
+	while ((de = readdir(d)) != NULL) {
+		unsigned char dtype;
+
+		if (de->d_name[0] == '.')
+			continue;
+		if (ends_with(de->d_name, ".lock"))
+			continue;
+		strbuf_addstr(&refname, de->d_name);
+
+		dtype = get_dtype(de, &path, 1);
+		if (dtype == DT_REG && is_pseudoref_syntax(de->d_name))
+			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
+
+		strbuf_setlen(&refname, dirnamelen);
+	}
+	strbuf_release(&refname);
+	strbuf_release(&path);
+	closedir(d);
+}
+
+static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
+					     unsigned int flags)
 {
 	if (!refs->loose) {
+		struct ref_dir *dir;
+
 		/*
 		 * Mark the top-level directory complete because we
 		 * are about to read the only subdirectory that can
@@ -328,12 +377,17 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
 		/* We're going to fill the top level ourselves: */
 		refs->loose->root->flag &= ~REF_INCOMPLETE;
 
+		dir = get_ref_dir(refs->loose->root);
+
+		if (flags & DO_FOR_EACH_INCLUDE_ALL_REFS)
+			add_pseudoref_like_entries(dir->cache->ref_store, dir,
+						     refs->loose->root->name);
+
 		/*
 		 * Add an incomplete entry for "refs/" (to be filled
 		 * lazily):
 		 */
-		add_entry_to_dir(get_ref_dir(refs->loose->root),
-				 create_dir_entry(refs->loose, "refs/", 5));
+		add_entry_to_dir(dir, create_dir_entry(refs->loose, "refs/", 5));
 	}
 	return refs->loose;
 }
@@ -861,7 +915,7 @@ static struct ref_iterator *files_ref_iterator_begin(
 	 * disk, and re-reads it if not.
 	 */
 
-	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
+	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
 					      prefix, ref_store->repo, 1);
 
 	/*
@@ -1222,7 +1276,7 @@ static int files_pack_refs(struct ref_store *ref_store,
 
 	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
 
-	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
+	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL,
 					the_repository, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
 		/*
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 4af83bf9a5..981a91c4c6 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -260,6 +260,13 @@ enum do_for_each_ref_flags {
 	 * INCLUDE_BROKEN, since they are otherwise not included at all.
 	 */
 	DO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2),
+
+	/*
+	 * Include all refs in the $GIT_DIR in contrast to generally only listing
+	 * references having the "refs/" prefix. In the files-backend this is
+	 * limited to regular refs, pseudorefs and HEAD.
+	 */
+	DO_FOR_EACH_INCLUDE_ALL_REFS = (1 << 3),
 };
 
 /*
-- 
2.43.GIT

Re: [PATCH 4/5] refs: introduce `refs_for_each_all_refs()`

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> +static void add_pseudoref_like_entries(struct ref_store *ref_store,
> +					 struct ref_dir *dir,
> +					 const char *dirname)
> +{
> +	struct files_ref_store *refs =
> +		files_downcast(ref_store, REF_STORE_READ, "fill_ref_dir");
> +	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
> +	struct dirent *de;
> +	size_t dirnamelen;
> +	DIR *d;
> +
> +	files_ref_path(refs, &path, dirname);
> +
> +	d = opendir(path.buf);
> +	if (!d) {
> +		strbuf_release(&path);
> +		return;
> +	}
> +
> +	strbuf_addstr(&refname, dirname);
> +	dirnamelen = refname.len;
> +
> +	while ((de = readdir(d)) != NULL) {
> +		unsigned char dtype;
> +
> +		if (de->d_name[0] == '.')
> +			continue;
> +		if (ends_with(de->d_name, ".lock"))
> +			continue;
> +		strbuf_addstr(&refname, de->d_name);
> +
> +		dtype = get_dtype(de, &path, 1);
> +		if (dtype == DT_REG && is_pseudoref_syntax(de->d_name))
> +			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);

This looks more like add_pseudoref_entries() given that the general
direction is to have an "allow" list of pseudo refs (at this point
after the previous step of the series, is_pseudoref_syntax() is the
is_pseudoref() function, and uses ends_with("_HEAD") as a mere
optimization to avoid listing all the possible pseudo refs that
exists or will be added in the future whose name ends with "_HEAD").

Other than the naming, I think these two steps make sense.

Thanks.

Re: [PATCH 4/5] refs: introduce `refs_for_each_all_refs()`

[Karthik Nayak]

On Fri, Jan 19, 2024 at 9:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> This looks more like add_pseudoref_entries() given that the general
> direction is to have an "allow" list of pseudo refs (at this point
> after the previous step of the series, is_pseudoref_syntax() is the
> is_pseudoref() function, and uses ends_with("_HEAD") as a mere
> optimization to avoid listing all the possible pseudo refs that
> exists or will be added in the future whose name ends with "_HEAD").
>
> Other than the naming, I think these two steps make sense.

I think overall the naming is correct, I would change the comments in
`is_pseudoref_syntax()`.

Because, apart from pseudorefs, we also want to print HEAD. This is also
why the pattern matches "HEAD" instead of "_HEAD". I'll add some more
comments to clarify this.

So to summarize:
1. `is_pseudoref_syntax()` checks for pseudoref like syntax, this also matches
HEAD. Will add comments here to clarify that we do not actually check
ref contents.
2. `add_pseudoref_like_entries()` adds pseudorefs and HEAD to the loose refs
cache.

Re: [PATCH 4/5] refs: introduce `refs_for_each_all_refs()`

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> On Fri, Jan 19, 2024 at 9:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> This looks more like add_pseudoref_entries() given that the general
>> direction is to have an "allow" list of pseudo refs (at this point
>> after the previous step of the series, is_pseudoref_syntax() is the
>> is_pseudoref() function, and uses ends_with("_HEAD") as a mere
>> optimization to avoid listing all the possible pseudo refs that
>> exists or will be added in the future whose name ends with "_HEAD").
>>
>> Other than the naming, I think these two steps make sense.
>
> I think overall the naming is correct, I would change the comments in
> `is_pseudoref_syntax()`.
>
> Because, apart from pseudorefs, we also want to print HEAD. This is also
> why the pattern matches "HEAD" instead of "_HEAD". I'll add some more
> comments to clarify this.

With the hardcoded "these are definitely pseudorefs" list in the
function, it no longer is is_pseudoref_SYNTAX() at all.  I would
rather prefer to see is_pseudoref() that says no to HEAD and have
the callers check

	-	if (is_pseudoref_syntax(foo))
	+	if (is_pseudoref(foo) || is_headref(foo))

than keeping the messy semantics we have.  My second preference is
to call it is_pseudoref_or_head() that says yes to "HEAD" and
pseudorefs, even though I like it much less.

Similarly, between giving the function under discussion a more
descriptive name add_pseudoref_and_head_entries(), or adding a new
function add_head_entry() to make the callers call add_head_entry()
and add_pseudoref_entries() separately, I have a slight preference
for the latter.

Thanks.

[PATCH 5/5] for-each-ref: avoid filtering on empty pattern

[Karthik Nayak]

When the user uses an empty string pattern (""), we don't match any refs
in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
path based matching and an empty string doesn't match any path.

In this commit we change this behavior by making empty string pattern
match all references. This is done by introducing a new flag
`FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
introduced `refs_for_each_all_refs()` function to iterate over all the
refs in the repository.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  3 ++-
 builtin/for-each-ref.c             | 21 +++++++++++++++++-
 ref-filter.c                       | 13 ++++++++++--
 ref-filter.h                       |  4 +++-
 t/t6302-for-each-ref-filter.sh     | 34 ++++++++++++++++++++++++++++++
 5 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index be9543f684..b1cb482bf5 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -32,7 +32,8 @@ OPTIONS
 	If one or more patterns are given, only refs are shown that
 	match against at least one pattern, either using fnmatch(3) or
 	literally, in the latter case matching completely or from the
-	beginning up to a slash.
+	beginning up to a slash. If an empty string is provided all refs
+	are printed, including HEAD and pseudorefs.
 
 --stdin::
 	If `--stdin` is supplied, then the list of patterns is read from
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3885a9c28e..5aa879e8be 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -25,6 +25,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	struct ref_format format = REF_FORMAT_INIT;
 	int from_stdin = 0;
 	struct strvec vec = STRVEC_INIT;
+	unsigned int flags = FILTER_REFS_ALL;
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &format.quote_style,
@@ -93,11 +94,29 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		/* vec.v is NULL-terminated, just like 'argv'. */
 		filter.name_patterns = vec.v;
 	} else {
+		size_t i;
+
 		filter.name_patterns = argv;
+
+		/*
+		 * Search for any empty string pattern, if it exists then we
+		 * print all refs without any filtering.
+		 */
+		i = 0;
+		while (argv[i]) {
+			if (!argv[i][0]) {
+				flags = FILTER_REFS_NO_FILTER;
+				/* doing this removes any pattern from being matched */
+				filter.name_patterns[0] = NULL;
+				break;
+			}
+
+			i++;
+		}
 	}
 
 	filter.match_as_path = 1;
-	filter_and_format_refs(&filter, FILTER_REFS_ALL, sorting, &format);
+	filter_and_format_refs(&filter, flags, sorting, &format);
 
 	ref_filter_clear(&filter);
 	ref_sorting_release(sorting);
diff --git a/ref-filter.c b/ref-filter.c
index 35b989e1df..6dac133b87 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2622,6 +2622,11 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
 				       each_ref_fn cb,
 				       void *cb_data)
 {
+	if (filter->kind & FILTER_REFS_NO_FILTER) {
+		return refs_for_each_all_refs(
+			get_main_ref_store(the_repository), cb, cb_data);
+	}
+
 	if (!filter->match_as_path) {
 		/*
 		 * in this case, the patterns are applied after
@@ -2775,8 +2780,12 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
 
 	/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
 	kind = filter_ref_kind(filter, refname);
-	if (!(kind & filter->kind))
+	if (filter->kind & FILTER_REFS_NO_FILTER) {
+		if (kind == FILTER_REFS_DETACHED_HEAD)
+			kind = FILTER_REFS_OTHERS;
+	} else if (!(kind & filter->kind)) {
 		return NULL;
+	}
 
 	if (!filter_pattern_match(filter, refname))
 		return NULL;
@@ -3041,7 +3050,7 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
 			ret = for_each_fullref_in("refs/remotes/", fn, cb_data);
 		else if (filter->kind == FILTER_REFS_TAGS)
 			ret = for_each_fullref_in("refs/tags/", fn, cb_data);
-		else if (filter->kind & FILTER_REFS_ALL)
+		else if (filter->kind & FILTER_REFS_ALL || filter->kind & FILTER_REFS_NO_FILTER)
 			ret = for_each_fullref_in_pattern(filter, fn, cb_data);
 		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
 			head_ref(fn, cb_data);
diff --git a/ref-filter.h b/ref-filter.h
index 07cd6f6da3..1eab325ce0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -22,7 +22,9 @@
 #define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
 				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
 #define FILTER_REFS_DETACHED_HEAD  0x0020
-#define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
+#define FILTER_REFS_NO_FILTER      0x0040
+#define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD | \
+				    FILTER_REFS_NO_FILTER)
 
 struct atom_value;
 struct ref_sorting;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 82f3d1ea0f..3922326cab 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -31,6 +31,40 @@ test_expect_success 'setup some history and refs' '
 	git update-ref refs/odd/spot main
 '
 
+cat >expect <<-\EOF
+	HEAD
+	ORIG_HEAD
+	refs/heads/main
+	refs/heads/side
+	refs/odd/spot
+	refs/tags/annotated-tag
+	refs/tags/doubly-annotated-tag
+	refs/tags/doubly-signed-tag
+	refs/tags/four
+	refs/tags/one
+	refs/tags/signed-tag
+	refs/tags/three
+	refs/tags/two
+EOF
+
+test_expect_success 'empty pattern prints pseudorefs' '
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" "" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'empty pattern with other patterns' '
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" "" "refs/" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'empty pattern towards the end' '
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" "refs/" "" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'filtering with --points-at' '
 	cat >expect <<-\EOF &&
 	refs/heads/main
-- 
2.43.GIT

[PATCH v2 0/4] for-each-ref: print all refs on empty string pattern

[Karthik Nayak]

This is the second version of my patch series to print refs
when and empty string pattern is used with git-for-each-ref(1).

With the upcoming introduction of the reftable backend, it becomes ever
so important to provide the necessary tooling for printing all refs
associated with a repository.

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 all the operational refs
within the repository.

This is crucial because with the reftable backend, all these refs will
also move to reftable. It would be necessary to identify all the refs
that are stored within the reftable since there is no easy way to do so
otherwise. This is because the reftable itself is a binary format and
all access will be via git. Unlike the filesystem backend, which allows
access directly via the filesystem.

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 print all refs when an empty string pattern
is used. This is a deviation from the current situation wherein the empty
string pattern currently matches and prints no refs.

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

Changes since v1:

- Introduce `is_pseudoref()` and `is_headref()` and use them instead of
directly using `is_pseudoref_syntax`.
- Rename `add_pseudoref_like_entries()` to `add_pseudoref_and_head_entries()`
since it also adds the HEAD ref.
- Also check for the pseudoref's contents to ensure it conforms to the ref
format. 

Karthik Nayak (4):
  refs: introduce `is_pseudoref()` and `is_headref()`
  refs: extract out `loose_fill_ref_dir_regular_file()`
  refs: introduce `refs_for_each_all_refs()`
  for-each-ref: avoid filtering on empty pattern

 Documentation/git-for-each-ref.txt |   3 +-
 builtin/for-each-ref.c             |  21 ++++-
 ref-filter.c                       |  13 ++-
 ref-filter.h                       |   4 +-
 refs.c                             |  39 +++++++++
 refs.h                             |   9 ++
 refs/files-backend.c               | 127 +++++++++++++++++++++--------
 refs/refs-internal.h               |   7 ++
 t/t6302-for-each-ref-filter.sh     |  34 ++++++++
 9 files changed, 218 insertions(+), 39 deletions(-)

-- 
2.43.GIT

[PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`

[Karthik Nayak]

Introduce two new functions `is_pseudoref()` and `is_headref()`. This
provides the necessary functionality for us to add pseudorefs and HEAD
to the loose ref cache in the files backend, allowing us to build
tooling to print these refs.

The `is_pseudoref()` function internally calls `is_pseudoref_syntax()`
but adds onto it by also checking to ensure that the pseudoref either
ends with a "_HEAD" suffix or matches a list of exceptions. After which
we also parse the contents of the pseudoref to ensure that it conforms
to the ref format.

We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
because the function is also used by `is_current_worktree_ref()` and
making it stricter to match only known pseudorefs might have unintended
consequences due to files like 'BISECT_START' which isn't a pseudoref
but sometimes contains object ID.

Keeping this in mind, we leave `is_pseudoref_syntax()` as is and create
`is_pseudoref()` which is stricter. Ideally we'd want to move the new
syntax checks to `is_pseudoref_syntax()` but a prerequisite for this
would be to actually remove the exception list by converting those
pseudorefs to also contain a '_HEAD' suffix and perhaps move bisect
related files like 'BISECT_START' to a new directory similar to the
'rebase-merge' directory.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c | 32 ++++++++++++++++++++++++++++++++
 refs.h |  3 +++
 2 files changed, 35 insertions(+)

diff --git a/refs.c b/refs.c
index 20e8f1ff1f..4b6bfc66fb 100644
--- a/refs.c
+++ b/refs.c
@@ -859,6 +859,38 @@ static int is_pseudoref_syntax(const char *refname)
 	return 1;
 }
 
+int is_pseudoref(struct ref_store *refs, const char *refname)
+{
+	static const char *const irregular_pseudorefs[] = {
+		"AUTO_MERGE",
+		"BISECT_EXPECTED_REV",
+		"NOTES_MERGE_PARTIAL",
+		"NOTES_MERGE_REF",
+		"MERGE_AUTOSTASH"
+	};
+	size_t i;
+
+	if (!is_pseudoref_syntax(refname))
+		return 0;
+
+	if (ends_with(refname, "_HEAD"))
+		return refs_ref_exists(refs, refname);
+
+	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
+		 if (!strcmp(refname, irregular_pseudorefs[i]))
+			 return refs_ref_exists(refs, refname);
+
+	return 0;
+}
+
+int is_headref(struct ref_store *refs, const char *refname)
+{
+	if (!strcmp(refname, "HEAD"))
+		return refs_ref_exists(refs, refname);
+
+	return 0;
+}
+
 static int is_current_worktree_ref(const char *ref) {
 	return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
 }
diff --git a/refs.h b/refs.h
index 11b3b6ccea..46b8085d63 100644
--- a/refs.h
+++ b/refs.h
@@ -1021,4 +1021,7 @@ extern struct ref_namespace_info ref_namespace[NAMESPACE__COUNT];
  */
 void update_ref_namespace(enum ref_namespace namespace, char *ref);
 
+int is_pseudoref(struct ref_store *refs, const char *refname);
+int is_headref(struct ref_store *refs, const char *refname);
+
 #endif /* REFS_H */
-- 
2.43.GIT

Re: [PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
> because the function is also used by `is_current_worktree_ref()` and
> making it stricter to match only known pseudorefs might have unintended
> consequences due to files like 'BISECT_START' which isn't a pseudoref
> but sometimes contains object ID.

Well described.

> diff --git a/refs.c b/refs.c
> index 20e8f1ff1f..4b6bfc66fb 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -859,6 +859,38 @@ static int is_pseudoref_syntax(const char *refname)
>  	return 1;
>  }
>  
> +int is_pseudoref(struct ref_store *refs, const char *refname)
> +{
> +	static const char *const irregular_pseudorefs[] = {
> +		"AUTO_MERGE",
> +		"BISECT_EXPECTED_REV",
> +		"NOTES_MERGE_PARTIAL",
> +		"NOTES_MERGE_REF",
> +		"MERGE_AUTOSTASH"

Let's end an array's initializer with a trailing comma, to help
future patches to add entries to this array without unnecessary
patch noise. 

> +	};
> +	size_t i;
> +
> +	if (!is_pseudoref_syntax(refname))
> +		return 0;
> +
> +	if (ends_with(refname, "_HEAD"))
> +		return refs_ref_exists(refs, refname);
> +
> +	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
> +		 if (!strcmp(refname, irregular_pseudorefs[i]))
> +			 return refs_ref_exists(refs, refname);
> +
> +	return 0;
> +}

The above uses refs_ref_exists() because we want these to
successfully resolve for reading.

> +int is_headref(struct ref_store *refs, const char *refname)
> +{
> +	if (!strcmp(refname, "HEAD"))
> +		return refs_ref_exists(refs, refname);

Given that "git for-each-ref refs/remotes" does not show
"refs/remotes/origin/HEAD" in the output when we do not have the
remote-tracking branch that symref points at, we probably do want
to omit "HEAD" from the output when the HEAD symref points at an
unborn branch.  If refs_ref_exists() says "no, it does not exist"
in such a case, we are perfectly fine with this code.

We do not have to worry about the unborn state for pseudorefs
because they would never be symbolic.  But that in turn makes me
suspect that the check done with refs_ref_exists() in the
is_pseudoref() helper is a bit too lenient by allowing it to be a
symbolic ref.  Shouldn't we be using a check based on
read_ref_full(), like we did in another topic recently [*]?


[Reference]

 * https://lore.kernel.org/git/xmqqzfxa9usx.fsf@gitster.g/

Re: [PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`

[Karthik Nayak]

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

Hello Junio,

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
>> because the function is also used by `is_current_worktree_ref()` and
>> making it stricter to match only known pseudorefs might have unintended
>> consequences due to files like 'BISECT_START' which isn't a pseudoref
>> but sometimes contains object ID.
>
> Well described.
>
>> diff --git a/refs.c b/refs.c
>> index 20e8f1ff1f..4b6bfc66fb 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -859,6 +859,38 @@ static int is_pseudoref_syntax(const char *refname)
>>  	return 1;
>>  }
>>
>> +int is_pseudoref(struct ref_store *refs, const char *refname)
>> +{
>> +	static const char *const irregular_pseudorefs[] = {
>> +		"AUTO_MERGE",
>> +		"BISECT_EXPECTED_REV",
>> +		"NOTES_MERGE_PARTIAL",
>> +		"NOTES_MERGE_REF",
>> +		"MERGE_AUTOSTASH"
>
> Let's end an array's initializer with a trailing comma, to help
> future patches to add entries to this array without unnecessary
> patch noise.

Sure, will add!

>> +	};
>> +	size_t i;
>> +
>> +	if (!is_pseudoref_syntax(refname))
>> +		return 0;
>> +
>> +	if (ends_with(refname, "_HEAD"))
>> +		return refs_ref_exists(refs, refname);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
>> +		 if (!strcmp(refname, irregular_pseudorefs[i]))
>> +			 return refs_ref_exists(refs, refname);
>> +
>> +	return 0;
>> +}
>
> The above uses refs_ref_exists() because we want these to
> successfully resolve for reading.
>
>> +int is_headref(struct ref_store *refs, const char *refname)
>> +{
>> +	if (!strcmp(refname, "HEAD"))
>> +		return refs_ref_exists(refs, refname);
>
> Given that "git for-each-ref refs/remotes" does not show
> "refs/remotes/origin/HEAD" in the output when we do not have the
> remote-tracking branch that symref points at, we probably do want
> to omit "HEAD" from the output when the HEAD symref points at an
> unborn branch.  If refs_ref_exists() says "no, it does not exist"
> in such a case, we are perfectly fine with this code.
>
> We do not have to worry about the unborn state for pseudorefs
> because they would never be symbolic.  But that in turn makes me
> suspect that the check done with refs_ref_exists() in the
> is_pseudoref() helper is a bit too lenient by allowing it to be a
> symbolic ref.  Shouldn't we be using a check based on
> read_ref_full(), like we did in another topic recently [*]?
>
>
> [Reference]
>
>  * https://lore.kernel.org/git/xmqqzfxa9usx.fsf@gitster.g/
>

Thanks, this makes sense and the link is helpful. I'll do something
similar, but since HEAD can be a symref, I'll drop the
`RESOLVE_REF_NO_RECURSE` flag and only use `RESOLVE_REF_READING`.

I'll wait a day or two, before sending in the new version with the
fixes. The current diff is

diff --git a/refs.c b/refs.c
index b5e63f133a..4a1fd30ef2 100644
--- a/refs.c
+++ b/refs.c
@@ -866,7 +866,7 @@ int is_pseudoref(struct ref_store *refs, const
char *refname)
 		"BISECT_EXPECTED_REV",
 		"NOTES_MERGE_PARTIAL",
 		"NOTES_MERGE_REF",
-		"MERGE_AUTOSTASH"
+		"MERGE_AUTOSTASH",
 	};
 	size_t i;

@@ -885,10 +885,23 @@ int is_pseudoref(struct ref_store *refs, const
char *refname)

 int is_headref(struct ref_store *refs, const char *refname)
 {
-	if (!strcmp(refname, "HEAD"))
-		return refs_ref_exists(refs, refname);
+	struct object_id oid;
+	int flag;

-	return 0;
+	if (strcmp(refname, "HEAD"))
+		return 0;
+
+	/*
+	 * If HEAD doesn't exist, we don't have to die, but rather,
+	 * we simply return 0.
+	 */
+	if (read_ref_full("HEAD", RESOLVE_REF_READING, &oid, &flag))
+		return 0;
+
+	if (is_null_oid(&oid))
+		return 0;
+
+	return 1;
 }

 static int is_current_worktree_ref(const char *ref) {

- Karthik

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

Re: [PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>>> +int is_headref(struct ref_store *refs, const char *refname)
>>> +{
>>> +	if (!strcmp(refname, "HEAD"))
>>> +		return refs_ref_exists(refs, refname);
>>
>> Given that "git for-each-ref refs/remotes" does not show
>> "refs/remotes/origin/HEAD" in the output when we do not have the
>> remote-tracking branch that symref points at, we probably do want
>> to omit "HEAD" from the output when the HEAD symref points at an
>> unborn branch.  If refs_ref_exists() says "no, it does not exist"
>> in such a case, we are perfectly fine with this code.
>>
>> We do not have to worry about the unborn state for pseudorefs
>> because they would never be symbolic.  But that in turn makes me
>> suspect that the check done with refs_ref_exists() in the
>> is_pseudoref() helper is a bit too lenient by allowing it to be a
>> symbolic ref.  Shouldn't we be using a check based on
>> read_ref_full(), like we did in another topic recently [*]?
>>
>>
>> [Reference]
>>
>>  * https://lore.kernel.org/git/xmqqzfxa9usx.fsf@gitster.g/
>>
>
> Thanks, this makes sense and the link is helpful. I'll do something
> similar, but since HEAD can be a symref, I'll drop the
> `RESOLVE_REF_NO_RECURSE` flag and only use `RESOLVE_REF_READING`.

Just to make sure there is no misunderstanding, I think how
is_headref() does what it does in the patch is perfectly fine,
including its use of refs_ref_exists().  The side I was referring to
with "in turn makes me suspect" is the other helper function that
will never have to deal with a symref.  Use of refs_ref_exists() in
that function is too loose.

Re: [PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`

[Karthik Nayak]

On Thu, Jan 25, 2024 at 5:28 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> >>> +int is_headref(struct ref_store *refs, const char *refname)
> >>> +{
> >>> +   if (!strcmp(refname, "HEAD"))
> >>> +           return refs_ref_exists(refs, refname);
> >>
> >> Given that "git for-each-ref refs/remotes" does not show
> >> "refs/remotes/origin/HEAD" in the output when we do not have the
> >> remote-tracking branch that symref points at, we probably do want
> >> to omit "HEAD" from the output when the HEAD symref points at an
> >> unborn branch.  If refs_ref_exists() says "no, it does not exist"
> >> in such a case, we are perfectly fine with this code.
> >>
> >> We do not have to worry about the unborn state for pseudorefs
> >> because they would never be symbolic.  But that in turn makes me
> >> suspect that the check done with refs_ref_exists() in the
> >> is_pseudoref() helper is a bit too lenient by allowing it to be a
> >> symbolic ref.  Shouldn't we be using a check based on
> >> read_ref_full(), like we did in another topic recently [*]?
> >>
> >>
> >> [Reference]
> >>
> >>  * https://lore.kernel.org/git/xmqqzfxa9usx.fsf@gitster.g/
> >>
> >
> > Thanks, this makes sense and the link is helpful. I'll do something
> > similar, but since HEAD can be a symref, I'll drop the
> > `RESOLVE_REF_NO_RECURSE` flag and only use `RESOLVE_REF_READING`.
>
> Just to make sure there is no misunderstanding, I think how
> is_headref() does what it does in the patch is perfectly fine,
> including its use of refs_ref_exists().  The side I was referring to
> with "in turn makes me suspect" is the other helper function that
> will never have to deal with a symref.  Use of refs_ref_exists() in
> that function is too loose.
>

AH! Totally misunderstood, thanks for reiterating.

[PATCH v2 2/4] refs: extract out `loose_fill_ref_dir_regular_file()`

[Karthik Nayak]

Extract out the code for adding a single file to the loose ref dir as
`loose_fill_ref_dir_regular_file()` from `loose_fill_ref_dir()` in
`refs/files-backend.c`.

This allows us to use this function independently in the following
commits where we add code to also add pseudorefs to the ref dir.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c | 62 +++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b288fc97db..22495a4807 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -229,6 +229,38 @@ static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dir
 	}
 }
 
+static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
+					    const char *refname,
+					    struct ref_dir *dir)
+{
+	struct object_id oid;
+	int flag;
+
+	if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_READING,
+				     &oid, &flag)) {
+		oidclr(&oid);
+		flag |= REF_ISBROKEN;
+	} else if (is_null_oid(&oid)) {
+		/*
+		 * It is so astronomically unlikely
+		 * that null_oid is the OID of an
+		 * actual object that we consider its
+		 * appearance in a loose reference
+		 * file to be repo corruption
+		 * (probably due to a software bug).
+		 */
+		flag |= REF_ISBROKEN;
+	}
+
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		if (!refname_is_safe(refname))
+			die("loose refname is dangerous: %s", refname);
+		oidclr(&oid);
+		flag |= REF_BAD_NAME | REF_ISBROKEN;
+	}
+	add_entry_to_dir(dir, create_ref_entry(refname, &oid, flag));
+}
+
 /*
  * Read the loose references from the namespace dirname into dir
  * (without recursing).  dirname must end with '/'.  dir must be the
@@ -257,8 +289,6 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 	strbuf_add(&refname, dirname, dirnamelen);
 
 	while ((de = readdir(d)) != NULL) {
-		struct object_id oid;
-		int flag;
 		unsigned char dtype;
 
 		if (de->d_name[0] == '.')
@@ -274,33 +304,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 					 create_dir_entry(dir->cache, refname.buf,
 							  refname.len));
 		} else if (dtype == DT_REG) {
-			if (!refs_resolve_ref_unsafe(&refs->base,
-						     refname.buf,
-						     RESOLVE_REF_READING,
-						     &oid, &flag)) {
-				oidclr(&oid);
-				flag |= REF_ISBROKEN;
-			} else if (is_null_oid(&oid)) {
-				/*
-				 * It is so astronomically unlikely
-				 * that null_oid is the OID of an
-				 * actual object that we consider its
-				 * appearance in a loose reference
-				 * file to be repo corruption
-				 * (probably due to a software bug).
-				 */
-				flag |= REF_ISBROKEN;
-			}
-
-			if (check_refname_format(refname.buf,
-						 REFNAME_ALLOW_ONELEVEL)) {
-				if (!refname_is_safe(refname.buf))
-					die("loose refname is dangerous: %s", refname.buf);
-				oidclr(&oid);
-				flag |= REF_BAD_NAME | REF_ISBROKEN;
-			}
-			add_entry_to_dir(dir,
-					 create_ref_entry(refname.buf, &oid, flag));
+			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
 		}
 		strbuf_setlen(&refname, dirnamelen);
 	}
-- 
2.43.GIT

[PATCH v2 3/4] refs: introduce `refs_for_each_all_refs()`

[Karthik Nayak]

Introduce a new ref iteration flag `DO_FOR_EACH_INCLUDE_ALL_REFS`, which
will be used to iterate over all refs. In the files backend this is
limited to regular refs, pseudorefs and HEAD. For other backends like
the reftable this is the universal set of all refs stored in the
backend.

Refs which fall outside the `refs/` and aren't either pseudorefs or HEAD
are more of a grey area. This is because we don't block the users from
creating such refs but they are not officially supported. In the files
backend, we can isolate such files from other files.

Introduce `refs_for_each_all_refs()` which calls `do_for_each_ref()`
with this newly introduced flag.

In `refs/files-backend.c`, introduce a new function
`add_pseudoref_and_head_entries()` to add pseudorefs and HEAD to the
`ref_dir`. We then finally call `add_pseudoref_and_head_entries()`
whenever the `DO_FOR_EACH_INCLUDE_ALL_REFS` flag is set. Any new ref
backend will also have to implement similar changes on its end.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c               |  7 +++++
 refs.h               |  6 ++++
 refs/files-backend.c | 65 ++++++++++++++++++++++++++++++++++++++++----
 refs/refs-internal.h |  7 +++++
 4 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 4b6bfc66fb..b5e63f133a 100644
--- a/refs.c
+++ b/refs.c
@@ -1755,6 +1755,13 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data);
 }
 
+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
+			   void *cb_data)
+{
+	return do_for_each_ref(refs, "", NULL, fn, 0,
+			       DO_FOR_EACH_INCLUDE_ALL_REFS, cb_data);
+}
+
 static int qsort_strcmp(const void *va, const void *vb)
 {
 	const char *a = *(const char **)va;
diff --git a/refs.h b/refs.h
index 46b8085d63..77ecb820f9 100644
--- a/refs.h
+++ b/refs.h
@@ -396,6 +396,12 @@ int for_each_namespaced_ref(const char **exclude_patterns,
 int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
 int for_each_rawref(each_ref_fn fn, void *cb_data);
 
+/*
+ * Iterates over all ref types, regular, pseudorefs and HEAD.
+ */
+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
+			   void *cb_data);
+
 /*
  * Normalizes partial refs to their fully qualified form.
  * Will prepend <prefix> to the <pattern> if it doesn't start with 'refs/'.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 22495a4807..104f2e1ac7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -315,9 +315,59 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 	add_per_worktree_entries_to_dir(dir, dirname);
 }
 
-static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
+/*
+ * Add pseudorefs to the ref dir by parsing the directory for any files
+ * which follow the pseudoref syntax.
+ */
+static void add_pseudoref_and_head_entries(struct ref_store *ref_store,
+					 struct ref_dir *dir,
+					 const char *dirname)
+{
+	struct files_ref_store *refs =
+		files_downcast(ref_store, REF_STORE_READ, "fill_ref_dir");
+	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
+	struct dirent *de;
+	size_t dirnamelen;
+	DIR *d;
+
+	files_ref_path(refs, &path, dirname);
+
+	d = opendir(path.buf);
+	if (!d) {
+		strbuf_release(&path);
+		return;
+	}
+
+	strbuf_addstr(&refname, dirname);
+	dirnamelen = refname.len;
+
+	while ((de = readdir(d)) != NULL) {
+		unsigned char dtype;
+
+		if (de->d_name[0] == '.')
+			continue;
+		if (ends_with(de->d_name, ".lock"))
+			continue;
+		strbuf_addstr(&refname, de->d_name);
+
+		dtype = get_dtype(de, &path, 1);
+		if (dtype == DT_REG && (is_pseudoref(ref_store, de->d_name) ||
+								is_headref(ref_store, de->d_name)))
+			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
+
+		strbuf_setlen(&refname, dirnamelen);
+	}
+	strbuf_release(&refname);
+	strbuf_release(&path);
+	closedir(d);
+}
+
+static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
+					     unsigned int flags)
 {
 	if (!refs->loose) {
+		struct ref_dir *dir;
+
 		/*
 		 * Mark the top-level directory complete because we
 		 * are about to read the only subdirectory that can
@@ -328,12 +378,17 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
 		/* We're going to fill the top level ourselves: */
 		refs->loose->root->flag &= ~REF_INCOMPLETE;
 
+		dir = get_ref_dir(refs->loose->root);
+
+		if (flags & DO_FOR_EACH_INCLUDE_ALL_REFS)
+			add_pseudoref_and_head_entries(dir->cache->ref_store, dir,
+										   refs->loose->root->name);
+
 		/*
 		 * Add an incomplete entry for "refs/" (to be filled
 		 * lazily):
 		 */
-		add_entry_to_dir(get_ref_dir(refs->loose->root),
-				 create_dir_entry(refs->loose, "refs/", 5));
+		add_entry_to_dir(dir, create_dir_entry(refs->loose, "refs/", 5));
 	}
 	return refs->loose;
 }
@@ -861,7 +916,7 @@ static struct ref_iterator *files_ref_iterator_begin(
 	 * disk, and re-reads it if not.
 	 */
 
-	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
+	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
 					      prefix, ref_store->repo, 1);
 
 	/*
@@ -1222,7 +1277,7 @@ static int files_pack_refs(struct ref_store *ref_store,
 
 	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
 
-	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
+	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL,
 					the_repository, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
 		/*
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 8e9f04cc67..1cf7506435 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -260,6 +260,13 @@ enum do_for_each_ref_flags {
 	 * INCLUDE_BROKEN, since they are otherwise not included at all.
 	 */
 	DO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2),
+
+	/*
+	 * Include all refs in the $GIT_DIR in contrast to generally only listing
+	 * references having the "refs/" prefix. In the files-backend this is
+	 * limited to regular refs, pseudorefs and HEAD.
+	 */
+	DO_FOR_EACH_INCLUDE_ALL_REFS = (1 << 3),
 };
 
 /*
-- 
2.43.GIT

[PATCH v2 4/4] for-each-ref: avoid filtering on empty pattern

[Karthik Nayak]

When the user uses an empty string pattern (""), we don't match any refs
in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
path based matching and an empty string doesn't match any path.

In this commit we change this behavior by making empty string pattern
match all references. This is done by introducing a new flag
`FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
introduced `refs_for_each_all_refs()` function to iterate over all the
refs in the repository.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  3 ++-
 builtin/for-each-ref.c             | 21 +++++++++++++++++-
 ref-filter.c                       | 13 ++++++++++--
 ref-filter.h                       |  4 +++-
 t/t6302-for-each-ref-filter.sh     | 34 ++++++++++++++++++++++++++++++
 5 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index be9543f684..b1cb482bf5 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -32,7 +32,8 @@ OPTIONS
 	If one or more patterns are given, only refs are shown that
 	match against at least one pattern, either using fnmatch(3) or
 	literally, in the latter case matching completely or from the
-	beginning up to a slash.
+	beginning up to a slash. If an empty string is provided all refs
+	are printed, including HEAD and pseudorefs.
 
 --stdin::
 	If `--stdin` is supplied, then the list of patterns is read from
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3885a9c28e..5aa879e8be 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -25,6 +25,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	struct ref_format format = REF_FORMAT_INIT;
 	int from_stdin = 0;
 	struct strvec vec = STRVEC_INIT;
+	unsigned int flags = FILTER_REFS_ALL;
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &format.quote_style,
@@ -93,11 +94,29 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		/* vec.v is NULL-terminated, just like 'argv'. */
 		filter.name_patterns = vec.v;
 	} else {
+		size_t i;
+
 		filter.name_patterns = argv;
+
+		/*
+		 * Search for any empty string pattern, if it exists then we
+		 * print all refs without any filtering.
+		 */
+		i = 0;
+		while (argv[i]) {
+			if (!argv[i][0]) {
+				flags = FILTER_REFS_NO_FILTER;
+				/* doing this removes any pattern from being matched */
+				filter.name_patterns[0] = NULL;
+				break;
+			}
+
+			i++;
+		}
 	}
 
 	filter.match_as_path = 1;
-	filter_and_format_refs(&filter, FILTER_REFS_ALL, sorting, &format);
+	filter_and_format_refs(&filter, flags, sorting, &format);
 
 	ref_filter_clear(&filter);
 	ref_sorting_release(sorting);
diff --git a/ref-filter.c b/ref-filter.c
index 35b989e1df..6dac133b87 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2622,6 +2622,11 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
 				       each_ref_fn cb,
 				       void *cb_data)
 {
+	if (filter->kind & FILTER_REFS_NO_FILTER) {
+		return refs_for_each_all_refs(
+			get_main_ref_store(the_repository), cb, cb_data);
+	}
+
 	if (!filter->match_as_path) {
 		/*
 		 * in this case, the patterns are applied after
@@ -2775,8 +2780,12 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
 
 	/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
 	kind = filter_ref_kind(filter, refname);
-	if (!(kind & filter->kind))
+	if (filter->kind & FILTER_REFS_NO_FILTER) {
+		if (kind == FILTER_REFS_DETACHED_HEAD)
+			kind = FILTER_REFS_OTHERS;
+	} else if (!(kind & filter->kind)) {
 		return NULL;
+	}
 
 	if (!filter_pattern_match(filter, refname))
 		return NULL;
@@ -3041,7 +3050,7 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
 			ret = for_each_fullref_in("refs/remotes/", fn, cb_data);
 		else if (filter->kind == FILTER_REFS_TAGS)
 			ret = for_each_fullref_in("refs/tags/", fn, cb_data);
-		else if (filter->kind & FILTER_REFS_ALL)
+		else if (filter->kind & FILTER_REFS_ALL || filter->kind & FILTER_REFS_NO_FILTER)
 			ret = for_each_fullref_in_pattern(filter, fn, cb_data);
 		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
 			head_ref(fn, cb_data);
diff --git a/ref-filter.h b/ref-filter.h
index 07cd6f6da3..1eab325ce0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -22,7 +22,9 @@
 #define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
 				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
 #define FILTER_REFS_DETACHED_HEAD  0x0020
-#define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
+#define FILTER_REFS_NO_FILTER      0x0040
+#define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD | \
+				    FILTER_REFS_NO_FILTER)
 
 struct atom_value;
 struct ref_sorting;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 82f3d1ea0f..3922326cab 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -31,6 +31,40 @@ test_expect_success 'setup some history and refs' '
 	git update-ref refs/odd/spot main
 '
 
+cat >expect <<-\EOF
+	HEAD
+	ORIG_HEAD
+	refs/heads/main
+	refs/heads/side
+	refs/odd/spot
+	refs/tags/annotated-tag
+	refs/tags/doubly-annotated-tag
+	refs/tags/doubly-signed-tag
+	refs/tags/four
+	refs/tags/one
+	refs/tags/signed-tag
+	refs/tags/three
+	refs/tags/two
+EOF
+
+test_expect_success 'empty pattern prints pseudorefs' '
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" "" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'empty pattern with other patterns' '
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" "" "refs/" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'empty pattern towards the end' '
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" "refs/" "" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'filtering with --points-at' '
 	cat >expect <<-\EOF &&
 	refs/heads/main
-- 
2.43.GIT

[PATCH v3 0/4] for-each-ref: print all refs on empty string pattern

[Karthik Nayak]

This is the second version of my patch series to print refs
when and empty string pattern is used with git-for-each-ref(1).

With the upcoming introduction of the reftable backend, it becomes ever
so important to provide the necessary tooling for printing all refs
associated with a repository.

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 all the operational refs
within the repository.

This is crucial because with the reftable backend, all these refs will
also move to reftable. It would be necessary to identify all the refs
that are stored within the reftable since there is no easy way to do so
otherwise. This is because the reftable itself is a binary format and
all access will be via git. Unlike the filesystem backend, which allows
access directly via the filesystem.

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 print all refs when an empty string pattern
is used. This is a deviation from the current situation wherein the empty
string pattern currently matches and prints no refs.

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

Changes since v1:
- Added missing comma to the end of the `irregular_pseudorefs` array.
- Modified `is_pseudoref` to only work with non symrefs.  

Range-diff against v2:

1:  116d4c0e6d ! 1:  2141a2a62b refs: introduce `is_pseudoref()` and `is_headref()`
    @@ refs.c: static int is_pseudoref_syntax(const char *refname)
     +		"BISECT_EXPECTED_REV",
     +		"NOTES_MERGE_PARTIAL",
     +		"NOTES_MERGE_REF",
    -+		"MERGE_AUTOSTASH"
    ++		"MERGE_AUTOSTASH",
     +	};
    ++	struct object_id oid;
     +	size_t i;
     +
     +	if (!is_pseudoref_syntax(refname))
     +		return 0;
     +
    -+	if (ends_with(refname, "_HEAD"))
    -+		return refs_ref_exists(refs, refname);
    ++	if (ends_with(refname, "_HEAD")) {
    ++		 read_ref_full(refname, 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]))
    -+			 return refs_ref_exists(refs, refname);
    ++		if (!strcmp(refname, irregular_pseudorefs[i])) {
    ++			read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
    ++						  &oid, NULL);
    ++			return !is_null_oid(&oid);
    ++		}
     +
     +	return 0;
     +}
2:  4d4ca1cb26 = 2:  c96f0a9c83 refs: extract out `loose_fill_ref_dir_regular_file()`
3:  a5c0c4bf31 = 3:  d165358b83 refs: introduce `refs_for_each_all_refs()`
4:  a1c6537815 = 4:  a17983d0ba for-each-ref: avoid filtering on empty pattern


Karthik Nayak (4):
  refs: introduce `is_pseudoref()` and `is_headref()`
  refs: extract out `loose_fill_ref_dir_regular_file()`
  refs: introduce `refs_for_each_all_refs()`
  for-each-ref: avoid filtering on empty pattern

 Documentation/git-for-each-ref.txt |   3 +-
 builtin/for-each-ref.c             |  21 ++++-
 ref-filter.c                       |  13 ++-
 ref-filter.h                       |   4 +-
 refs.c                             |  46 +++++++++++
 refs.h                             |   9 ++
 refs/files-backend.c               | 127 +++++++++++++++++++++--------
 refs/refs-internal.h               |   7 ++
 t/t6302-for-each-ref-filter.sh     |  34 ++++++++
 9 files changed, 225 insertions(+), 39 deletions(-)

-- 
2.43.GIT

[PATCH v3 1/4] refs: introduce `is_pseudoref()` and `is_headref()`

[Karthik Nayak]

Introduce two new functions `is_pseudoref()` and `is_headref()`. This
provides the necessary functionality for us to add pseudorefs and HEAD
to the loose ref cache in the files backend, allowing us to build
tooling to print these refs.

The `is_pseudoref()` function internally calls `is_pseudoref_syntax()`
but adds onto it by also checking to ensure that the pseudoref either
ends with a "_HEAD" suffix or matches a list of exceptions. After which
we also parse the contents of the pseudoref to ensure that it conforms
to the ref format.

We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
because the function is also used by `is_current_worktree_ref()` and
making it stricter to match only known pseudorefs might have unintended
consequences due to files like 'BISECT_START' which isn't a pseudoref
but sometimes contains object ID.

Keeping this in mind, we leave `is_pseudoref_syntax()` as is and create
`is_pseudoref()` which is stricter. Ideally we'd want to move the new
syntax checks to `is_pseudoref_syntax()` but a prerequisite for this
would be to actually remove the exception list by converting those
pseudorefs to also contain a '_HEAD' suffix and perhaps move bisect
related files like 'BISECT_START' to a new directory similar to the
'rebase-merge' directory.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c | 39 +++++++++++++++++++++++++++++++++++++++
 refs.h |  3 +++
 2 files changed, 42 insertions(+)

diff --git a/refs.c b/refs.c
index 20e8f1ff1f..559f5aeea8 100644
--- a/refs.c
+++ b/refs.c
@@ -859,6 +859,45 @@ static int is_pseudoref_syntax(const char *refname)
 	return 1;
 }
 
+int is_pseudoref(struct ref_store *refs, const char *refname)
+{
+	static const char *const irregular_pseudorefs[] = {
+		"AUTO_MERGE",
+		"BISECT_EXPECTED_REV",
+		"NOTES_MERGE_PARTIAL",
+		"NOTES_MERGE_REF",
+		"MERGE_AUTOSTASH",
+	};
+	struct object_id oid;
+	size_t i;
+
+	if (!is_pseudoref_syntax(refname))
+		return 0;
+
+	if (ends_with(refname, "_HEAD")) {
+		 read_ref_full(refname, 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])) {
+			read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+						  &oid, NULL);
+			return !is_null_oid(&oid);
+		}
+
+	return 0;
+}
+
+int is_headref(struct ref_store *refs, const char *refname)
+{
+	if (!strcmp(refname, "HEAD"))
+		return refs_ref_exists(refs, refname);
+
+	return 0;
+}
+
 static int is_current_worktree_ref(const char *ref) {
 	return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
 }
diff --git a/refs.h b/refs.h
index 11b3b6ccea..46b8085d63 100644
--- a/refs.h
+++ b/refs.h
@@ -1021,4 +1021,7 @@ extern struct ref_namespace_info ref_namespace[NAMESPACE__COUNT];
  */
 void update_ref_namespace(enum ref_namespace namespace, char *ref);
 
+int is_pseudoref(struct ref_store *refs, const char *refname);
+int is_headref(struct ref_store *refs, const char *refname);
+
 #endif /* REFS_H */
-- 
2.43.GIT

Re: [PATCH v3 1/4] refs: introduce `is_pseudoref()` and `is_headref()`

[Jeff King]

On Mon, Jan 29, 2024 at 12:35:24PM +0100, Karthik Nayak wrote:

> +int is_pseudoref(struct ref_store *refs, const char *refname)
> [...]
> +	if (ends_with(refname, "_HEAD")) {
> +		 read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> +		      &oid, NULL);
> +		 return !is_null_oid(&oid);
> +	}

I was going to prepare a patch on top, but since it looks like this was
reverted out of 'next' to be revamped, I thought I'd mention it now:
-Wunused-parameter notices that we never use the "refs" parameter to the
function. And indeed it looks like a (possible) bug, since
read_ref_full() is going to use the_repository to find the ref store.

I think you'd want something like this squashed in (note that it also
fixes a slight indent problem in the first block):

diff --git a/refs.c b/refs.c
index 3190df8d81..0d65c31117 100644
--- a/refs.c
+++ b/refs.c
@@ -875,15 +875,17 @@ int is_pseudoref(struct ref_store *refs, const char *refname)
 		return 0;
 
 	if (ends_with(refname, "_HEAD")) {
-		 read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
-		      &oid, NULL);
-		 return !is_null_oid(&oid);
+		refs_resolve_ref_unsafe(refs, refname,
+					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])) {
-			read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
-						  &oid, NULL);
+			refs_resolve_ref_unsafe(refs, refname,
+						RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+						&oid, NULL);
 			return !is_null_oid(&oid);
 		}
 

-Peff

Re: [PATCH v3 1/4] refs: introduce `is_pseudoref()` and `is_headref()`

[Karthik Nayak]

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

Jeff King <peff@peff.net> writes:

> On Mon, Jan 29, 2024 at 12:35:24PM +0100, Karthik Nayak wrote:
>
>> +int is_pseudoref(struct ref_store *refs, const char *refname)
>> [...]
>> +	if (ends_with(refname, "_HEAD")) {
>> +		 read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>> +		      &oid, NULL);
>> +		 return !is_null_oid(&oid);
>> +	}
>
> I was going to prepare a patch on top, but since it looks like this was
> reverted out of 'next' to be revamped, I thought I'd mention it now:
> -Wunused-parameter notices that we never use the "refs" parameter to the
> function. And indeed it looks like a (possible) bug, since
> read_ref_full() is going to use the_repository to find the ref store.
>
> I think you'd want something like this squashed in (note that it also
> fixes a slight indent problem in the first block):
>
> diff --git a/refs.c b/refs.c
> index 3190df8d81..0d65c31117 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -875,15 +875,17 @@ int is_pseudoref(struct ref_store *refs, const char *refname)
>  		return 0;
>
>  	if (ends_with(refname, "_HEAD")) {
> -		 read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> -		      &oid, NULL);
> -		 return !is_null_oid(&oid);
> +		refs_resolve_ref_unsafe(refs, refname,
> +					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])) {
> -			read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> -						  &oid, NULL);
> +			refs_resolve_ref_unsafe(refs, refname,
> +						RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> +						&oid, NULL);
>  			return !is_null_oid(&oid);
>  		}
>
>
> -Peff

Thanks Jeff, makes sense, will squash this in.

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

[PATCH v3 2/4] refs: extract out `loose_fill_ref_dir_regular_file()`

[Karthik Nayak]

Extract out the code for adding a single file to the loose ref dir as
`loose_fill_ref_dir_regular_file()` from `loose_fill_ref_dir()` in
`refs/files-backend.c`.

This allows us to use this function independently in the following
commits where we add code to also add pseudorefs to the ref dir.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c | 62 +++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b288fc97db..22495a4807 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -229,6 +229,38 @@ static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dir
 	}
 }
 
+static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
+					    const char *refname,
+					    struct ref_dir *dir)
+{
+	struct object_id oid;
+	int flag;
+
+	if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_READING,
+				     &oid, &flag)) {
+		oidclr(&oid);
+		flag |= REF_ISBROKEN;
+	} else if (is_null_oid(&oid)) {
+		/*
+		 * It is so astronomically unlikely
+		 * that null_oid is the OID of an
+		 * actual object that we consider its
+		 * appearance in a loose reference
+		 * file to be repo corruption
+		 * (probably due to a software bug).
+		 */
+		flag |= REF_ISBROKEN;
+	}
+
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		if (!refname_is_safe(refname))
+			die("loose refname is dangerous: %s", refname);
+		oidclr(&oid);
+		flag |= REF_BAD_NAME | REF_ISBROKEN;
+	}
+	add_entry_to_dir(dir, create_ref_entry(refname, &oid, flag));
+}
+
 /*
  * Read the loose references from the namespace dirname into dir
  * (without recursing).  dirname must end with '/'.  dir must be the
@@ -257,8 +289,6 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 	strbuf_add(&refname, dirname, dirnamelen);
 
 	while ((de = readdir(d)) != NULL) {
-		struct object_id oid;
-		int flag;
 		unsigned char dtype;
 
 		if (de->d_name[0] == '.')
@@ -274,33 +304,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 					 create_dir_entry(dir->cache, refname.buf,
 							  refname.len));
 		} else if (dtype == DT_REG) {
-			if (!refs_resolve_ref_unsafe(&refs->base,
-						     refname.buf,
-						     RESOLVE_REF_READING,
-						     &oid, &flag)) {
-				oidclr(&oid);
-				flag |= REF_ISBROKEN;
-			} else if (is_null_oid(&oid)) {
-				/*
-				 * It is so astronomically unlikely
-				 * that null_oid is the OID of an
-				 * actual object that we consider its
-				 * appearance in a loose reference
-				 * file to be repo corruption
-				 * (probably due to a software bug).
-				 */
-				flag |= REF_ISBROKEN;
-			}
-
-			if (check_refname_format(refname.buf,
-						 REFNAME_ALLOW_ONELEVEL)) {
-				if (!refname_is_safe(refname.buf))
-					die("loose refname is dangerous: %s", refname.buf);
-				oidclr(&oid);
-				flag |= REF_BAD_NAME | REF_ISBROKEN;
-			}
-			add_entry_to_dir(dir,
-					 create_ref_entry(refname.buf, &oid, flag));
+			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
 		}
 		strbuf_setlen(&refname, dirnamelen);
 	}
-- 
2.43.GIT

[PATCH v3 3/4] refs: introduce `refs_for_each_all_refs()`

[Karthik Nayak]

Introduce a new ref iteration flag `DO_FOR_EACH_INCLUDE_ALL_REFS`, which
will be used to iterate over all refs. In the files backend this is
limited to regular refs, pseudorefs and HEAD. For other backends like
the reftable this is the universal set of all refs stored in the
backend.

Refs which fall outside the `refs/` and aren't either pseudorefs or HEAD
are more of a grey area. This is because we don't block the users from
creating such refs but they are not officially supported. In the files
backend, we can isolate such files from other files.

Introduce `refs_for_each_all_refs()` which calls `do_for_each_ref()`
with this newly introduced flag.

In `refs/files-backend.c`, introduce a new function
`add_pseudoref_and_head_entries()` to add pseudorefs and HEAD to the
`ref_dir`. We then finally call `add_pseudoref_and_head_entries()`
whenever the `DO_FOR_EACH_INCLUDE_ALL_REFS` flag is set. Any new ref
backend will also have to implement similar changes on its end.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c               |  7 +++++
 refs.h               |  6 ++++
 refs/files-backend.c | 65 ++++++++++++++++++++++++++++++++++++++++----
 refs/refs-internal.h |  7 +++++
 4 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 559f5aeea8..89b925719f 100644
--- a/refs.c
+++ b/refs.c
@@ -1762,6 +1762,13 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data);
 }
 
+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
+			   void *cb_data)
+{
+	return do_for_each_ref(refs, "", NULL, fn, 0,
+			       DO_FOR_EACH_INCLUDE_ALL_REFS, cb_data);
+}
+
 static int qsort_strcmp(const void *va, const void *vb)
 {
 	const char *a = *(const char **)va;
diff --git a/refs.h b/refs.h
index 46b8085d63..77ecb820f9 100644
--- a/refs.h
+++ b/refs.h
@@ -396,6 +396,12 @@ int for_each_namespaced_ref(const char **exclude_patterns,
 int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
 int for_each_rawref(each_ref_fn fn, void *cb_data);
 
+/*
+ * Iterates over all ref types, regular, pseudorefs and HEAD.
+ */
+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
+			   void *cb_data);
+
 /*
  * Normalizes partial refs to their fully qualified form.
  * Will prepend <prefix> to the <pattern> if it doesn't start with 'refs/'.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 22495a4807..104f2e1ac7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -315,9 +315,59 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 	add_per_worktree_entries_to_dir(dir, dirname);
 }
 
-static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
+/*
+ * Add pseudorefs to the ref dir by parsing the directory for any files
+ * which follow the pseudoref syntax.
+ */
+static void add_pseudoref_and_head_entries(struct ref_store *ref_store,
+					 struct ref_dir *dir,
+					 const char *dirname)
+{
+	struct files_ref_store *refs =
+		files_downcast(ref_store, REF_STORE_READ, "fill_ref_dir");
+	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
+	struct dirent *de;
+	size_t dirnamelen;
+	DIR *d;
+
+	files_ref_path(refs, &path, dirname);
+
+	d = opendir(path.buf);
+	if (!d) {
+		strbuf_release(&path);
+		return;
+	}
+
+	strbuf_addstr(&refname, dirname);
+	dirnamelen = refname.len;
+
+	while ((de = readdir(d)) != NULL) {
+		unsigned char dtype;
+
+		if (de->d_name[0] == '.')
+			continue;
+		if (ends_with(de->d_name, ".lock"))
+			continue;
+		strbuf_addstr(&refname, de->d_name);
+
+		dtype = get_dtype(de, &path, 1);
+		if (dtype == DT_REG && (is_pseudoref(ref_store, de->d_name) ||
+								is_headref(ref_store, de->d_name)))
+			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
+
+		strbuf_setlen(&refname, dirnamelen);
+	}
+	strbuf_release(&refname);
+	strbuf_release(&path);
+	closedir(d);
+}
+
+static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
+					     unsigned int flags)
 {
 	if (!refs->loose) {
+		struct ref_dir *dir;
+
 		/*
 		 * Mark the top-level directory complete because we
 		 * are about to read the only subdirectory that can
@@ -328,12 +378,17 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
 		/* We're going to fill the top level ourselves: */
 		refs->loose->root->flag &= ~REF_INCOMPLETE;
 
+		dir = get_ref_dir(refs->loose->root);
+
+		if (flags & DO_FOR_EACH_INCLUDE_ALL_REFS)
+			add_pseudoref_and_head_entries(dir->cache->ref_store, dir,
+										   refs->loose->root->name);
+
 		/*
 		 * Add an incomplete entry for "refs/" (to be filled
 		 * lazily):
 		 */
-		add_entry_to_dir(get_ref_dir(refs->loose->root),
-				 create_dir_entry(refs->loose, "refs/", 5));
+		add_entry_to_dir(dir, create_dir_entry(refs->loose, "refs/", 5));
 	}
 	return refs->loose;
 }
@@ -861,7 +916,7 @@ static struct ref_iterator *files_ref_iterator_begin(
 	 * disk, and re-reads it if not.
 	 */
 
-	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
+	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
 					      prefix, ref_store->repo, 1);
 
 	/*
@@ -1222,7 +1277,7 @@ static int files_pack_refs(struct ref_store *ref_store,
 
 	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
 
-	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
+	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL,
 					the_repository, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
 		/*
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 8e9f04cc67..1cf7506435 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -260,6 +260,13 @@ enum do_for_each_ref_flags {
 	 * INCLUDE_BROKEN, since they are otherwise not included at all.
 	 */
 	DO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2),
+
+	/*
+	 * Include all refs in the $GIT_DIR in contrast to generally only listing
+	 * references having the "refs/" prefix. In the files-backend this is
+	 * limited to regular refs, pseudorefs and HEAD.
+	 */
+	DO_FOR_EACH_INCLUDE_ALL_REFS = (1 << 3),
 };
 
 /*
-- 
2.43.GIT

[PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Karthik Nayak]

When the user uses an empty string pattern (""), we don't match any refs
in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
path based matching and an empty string doesn't match any path.

In this commit we change this behavior by making empty string pattern
match all references. This is done by introducing a new flag
`FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
introduced `refs_for_each_all_refs()` function to iterate over all the
refs in the repository.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  3 ++-
 builtin/for-each-ref.c             | 21 +++++++++++++++++-
 ref-filter.c                       | 13 ++++++++++--
 ref-filter.h                       |  4 +++-
 t/t6302-for-each-ref-filter.sh     | 34 ++++++++++++++++++++++++++++++
 5 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index be9543f684..b1cb482bf5 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -32,7 +32,8 @@ OPTIONS
 	If one or more patterns are given, only refs are shown that
 	match against at least one pattern, either using fnmatch(3) or
 	literally, in the latter case matching completely or from the
-	beginning up to a slash.
+	beginning up to a slash. If an empty string is provided all refs
+	are printed, including HEAD and pseudorefs.
 
 --stdin::
 	If `--stdin` is supplied, then the list of patterns is read from
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3885a9c28e..5aa879e8be 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -25,6 +25,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	struct ref_format format = REF_FORMAT_INIT;
 	int from_stdin = 0;
 	struct strvec vec = STRVEC_INIT;
+	unsigned int flags = FILTER_REFS_ALL;
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &format.quote_style,
@@ -93,11 +94,29 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		/* vec.v is NULL-terminated, just like 'argv'. */
 		filter.name_patterns = vec.v;
 	} else {
+		size_t i;
+
 		filter.name_patterns = argv;
+
+		/*
+		 * Search for any empty string pattern, if it exists then we
+		 * print all refs without any filtering.
+		 */
+		i = 0;
+		while (argv[i]) {
+			if (!argv[i][0]) {
+				flags = FILTER_REFS_NO_FILTER;
+				/* doing this removes any pattern from being matched */
+				filter.name_patterns[0] = NULL;
+				break;
+			}
+
+			i++;
+		}
 	}
 
 	filter.match_as_path = 1;
-	filter_and_format_refs(&filter, FILTER_REFS_ALL, sorting, &format);
+	filter_and_format_refs(&filter, flags, sorting, &format);
 
 	ref_filter_clear(&filter);
 	ref_sorting_release(sorting);
diff --git a/ref-filter.c b/ref-filter.c
index 35b989e1df..6dac133b87 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2622,6 +2622,11 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
 				       each_ref_fn cb,
 				       void *cb_data)
 {
+	if (filter->kind & FILTER_REFS_NO_FILTER) {
+		return refs_for_each_all_refs(
+			get_main_ref_store(the_repository), cb, cb_data);
+	}
+
 	if (!filter->match_as_path) {
 		/*
 		 * in this case, the patterns are applied after
@@ -2775,8 +2780,12 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
 
 	/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
 	kind = filter_ref_kind(filter, refname);
-	if (!(kind & filter->kind))
+	if (filter->kind & FILTER_REFS_NO_FILTER) {
+		if (kind == FILTER_REFS_DETACHED_HEAD)
+			kind = FILTER_REFS_OTHERS;
+	} else if (!(kind & filter->kind)) {
 		return NULL;
+	}
 
 	if (!filter_pattern_match(filter, refname))
 		return NULL;
@@ -3041,7 +3050,7 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
 			ret = for_each_fullref_in("refs/remotes/", fn, cb_data);
 		else if (filter->kind == FILTER_REFS_TAGS)
 			ret = for_each_fullref_in("refs/tags/", fn, cb_data);
-		else if (filter->kind & FILTER_REFS_ALL)
+		else if (filter->kind & FILTER_REFS_ALL || filter->kind & FILTER_REFS_NO_FILTER)
 			ret = for_each_fullref_in_pattern(filter, fn, cb_data);
 		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
 			head_ref(fn, cb_data);
diff --git a/ref-filter.h b/ref-filter.h
index 07cd6f6da3..1eab325ce0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -22,7 +22,9 @@
 #define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
 				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
 #define FILTER_REFS_DETACHED_HEAD  0x0020
-#define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
+#define FILTER_REFS_NO_FILTER      0x0040
+#define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD | \
+				    FILTER_REFS_NO_FILTER)
 
 struct atom_value;
 struct ref_sorting;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 82f3d1ea0f..3922326cab 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -31,6 +31,40 @@ test_expect_success 'setup some history and refs' '
 	git update-ref refs/odd/spot main
 '
 
+cat >expect <<-\EOF
+	HEAD
+	ORIG_HEAD
+	refs/heads/main
+	refs/heads/side
+	refs/odd/spot
+	refs/tags/annotated-tag
+	refs/tags/doubly-annotated-tag
+	refs/tags/doubly-signed-tag
+	refs/tags/four
+	refs/tags/one
+	refs/tags/signed-tag
+	refs/tags/three
+	refs/tags/two
+EOF
+
+test_expect_success 'empty pattern prints pseudorefs' '
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" "" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'empty pattern with other patterns' '
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" "" "refs/" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'empty pattern towards the end' '
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" "refs/" "" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'filtering with --points-at' '
 	cat >expect <<-\EOF &&
 	refs/heads/main
-- 
2.43.GIT

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Phillip Wood]

Hi Karthik

On 29/01/2024 11:35, Karthik Nayak wrote:
> When the user uses an empty string pattern (""), we don't match any refs
> in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
> path based matching and an empty string doesn't match any path.
>
> In this commit we change this behavior by making empty string pattern
> match all references. This is done by introducing a new flag
> `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
> introduced `refs_for_each_all_refs()` function to iterate over all the
> refs in the repository.

It actually iterates over all the refs in the current worktree, not all 
the refs in the repository. I have to say that I find it extremely 
unintuitive that "" behaves differently to not giving a pattern. I 
wonder if we can find a better UI here - perhaps a command line option 
to include pseudorefs?

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>   Documentation/git-for-each-ref.txt |  3 ++-
>   builtin/for-each-ref.c             | 21 +++++++++++++++++-
>   ref-filter.c                       | 13 ++++++++++--
>   ref-filter.h                       |  4 +++-
>   t/t6302-for-each-ref-filter.sh     | 34 ++++++++++++++++++++++++++++++
>   5 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index be9543f684..b1cb482bf5 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -32,7 +32,8 @@ OPTIONS
>   	If one or more patterns are given, only refs are shown that
>   	match against at least one pattern, either using fnmatch(3) or
>   	literally, in the latter case matching completely or from the
> -	beginning up to a slash.
> +	beginning up to a slash. If an empty string is provided all refs
> +	are printed, including HEAD and pseudorefs.

I think it would be helpful to clarify that it is all the refs for the 
current worktree that are printed, not all the refs in the repository - 
we still don't have a way to iterate over the per-worktree refs from 
other worktrees

Best Wishes

Phillip

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Patrick Steinhardt]

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

On Mon, Feb 05, 2024 at 06:48:52PM +0000, Phillip Wood wrote:
> Hi Karthik
> 
> On 29/01/2024 11:35, Karthik Nayak wrote:
> > When the user uses an empty string pattern (""), we don't match any refs
> > in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
> > path based matching and an empty string doesn't match any path.
> > 
> > In this commit we change this behavior by making empty string pattern
> > match all references. This is done by introducing a new flag
> > `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
> > introduced `refs_for_each_all_refs()` function to iterate over all the
> > refs in the repository.
> 
> It actually iterates over all the refs in the current worktree, not all the
> refs in the repository. I have to say that I find it extremely unintuitive
> that "" behaves differently to not giving a pattern. I wonder if we can find
> a better UI here - perhaps a command line option to include pseudorefs?

I tend to agree, and have argued for a flag in another thread, too [1].
Something like `--show-all-refs` feels a lot more intuitive to me and
also doesn't have the potential to break preexisting scripts which pass
the empty prefix (which would have returned the empty set of refs, but
now returns all refs).

[1]: https://lore.kernel.org/git/ZZWCXFghtql4i4YE@tanuki/

Patrick

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

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Phillip Wood]

Hi Patrick

On 06/02/2024 05:33, Patrick Steinhardt wrote:
> On Mon, Feb 05, 2024 at 06:48:52PM +0000, Phillip Wood wrote:
>> Hi Karthik
>>
>> On 29/01/2024 11:35, Karthik Nayak wrote:
>>> When the user uses an empty string pattern (""), we don't match any refs
>>> in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
>>> path based matching and an empty string doesn't match any path.
>>>
>>> In this commit we change this behavior by making empty string pattern
>>> match all references. This is done by introducing a new flag
>>> `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
>>> introduced `refs_for_each_all_refs()` function to iterate over all the
>>> refs in the repository.
>>
>> It actually iterates over all the refs in the current worktree, not all the
>> refs in the repository. I have to say that I find it extremely unintuitive
>> that "" behaves differently to not giving a pattern. I wonder if we can find
>> a better UI here - perhaps a command line option to include pseudorefs?
> 
> I tend to agree, and have argued for a flag in another thread, too [1].

Thanks for that, I'd missed that discussion

> Something like `--show-all-refs` feels a lot more intuitive to me and
> also doesn't have the potential to break preexisting scripts which pass
> the empty prefix (which would have returned the empty set of refs, but
> now returns all refs).

Yes, and as you point out in that other thread flag would allow the 
pseuderefs to be filtered and allow us to show the refs for all 
worktrees as well in the future.

Best Wishes

Phillip

> [1]: https://lore.kernel.org/git/ZZWCXFghtql4i4YE@tanuki/
> 
> Patrick

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Karthik Nayak]

Hello,

On Mon, Feb 5, 2024 at 7:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Karthik
>
> On 29/01/2024 11:35, Karthik Nayak wrote:
> > When the user uses an empty string pattern (""), we don't match any refs
> > in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
> > path based matching and an empty string doesn't match any path.
> >
> > In this commit we change this behavior by making empty string pattern
> > match all references. This is done by introducing a new flag
> > `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
> > introduced `refs_for_each_all_refs()` function to iterate over all the
> > refs in the repository.
>
> It actually iterates over all the refs in the current worktree, not all
> the refs in the repository. I have to say that I find it extremely
> unintuitive that "" behaves differently to not giving a pattern. I
> wonder if we can find a better UI here - perhaps a command line option
> to include pseudorefs?
>

As Patrick mentioned, this was discussed a while ago and we decided to
move forward with the `git for-each-ref ""` syntax.

> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> > ---
> >   Documentation/git-for-each-ref.txt |  3 ++-
> >   builtin/for-each-ref.c             | 21 +++++++++++++++++-
> >   ref-filter.c                       | 13 ++++++++++--
> >   ref-filter.h                       |  4 +++-
> >   t/t6302-for-each-ref-filter.sh     | 34 ++++++++++++++++++++++++++++++
> >   5 files changed, 70 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> > index be9543f684..b1cb482bf5 100644
> > --- a/Documentation/git-for-each-ref.txt
> > +++ b/Documentation/git-for-each-ref.txt
> > @@ -32,7 +32,8 @@ OPTIONS
> >       If one or more patterns are given, only refs are shown that
> >       match against at least one pattern, either using fnmatch(3) or
> >       literally, in the latter case matching completely or from the
> > -     beginning up to a slash.
> > +     beginning up to a slash. If an empty string is provided all refs
> > +     are printed, including HEAD and pseudorefs.
>
> I think it would be helpful to clarify that it is all the refs for the
> current worktree that are printed, not all the refs in the repository -
> we still don't have a way to iterate over the per-worktree refs from
> other worktrees
>

I agree, based on if we keep the current commits or remove them, I'll
send in a new patch or amend the current series.

Thanks!

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Phillip Wood]

Hi Karthik

On 06/02/2024 08:52, Karthik Nayak wrote:
> On Mon, Feb 5, 2024 at 7:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 29/01/2024 11:35, Karthik Nayak wrote:
>>> When the user uses an empty string pattern (""), we don't match any refs
>>> in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
>>> path based matching and an empty string doesn't match any path.
>>>
>>> In this commit we change this behavior by making empty string pattern
>>> match all references. This is done by introducing a new flag
>>> `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
>>> introduced `refs_for_each_all_refs()` function to iterate over all the
>>> refs in the repository.
>>
>> It actually iterates over all the refs in the current worktree, not all
>> the refs in the repository. I have to say that I find it extremely
>> unintuitive that "" behaves differently to not giving a pattern. I
>> wonder if we can find a better UI here - perhaps a command line option
>> to include pseudorefs?
>>
> 
> As Patrick mentioned, this was discussed a while ago and we decided to
> move forward with the `git for-each-ref ""` syntax.

Thanks I'd missed that discussion. I see that at the end of that 
discussion Junio was concerned that the proposed "" did not account for 
"refs/worktrees/$worktree/*" [1] - how has that been resolved?

Best Wishes

Phillip


[1] https://lore.kernel.org/git/xmqq1qawr6p4.fsf@gitster.g/

>>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>>> ---
>>>    Documentation/git-for-each-ref.txt |  3 ++-
>>>    builtin/for-each-ref.c             | 21 +++++++++++++++++-
>>>    ref-filter.c                       | 13 ++++++++++--
>>>    ref-filter.h                       |  4 +++-
>>>    t/t6302-for-each-ref-filter.sh     | 34 ++++++++++++++++++++++++++++++
>>>    5 files changed, 70 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>> index be9543f684..b1cb482bf5 100644
>>> --- a/Documentation/git-for-each-ref.txt
>>> +++ b/Documentation/git-for-each-ref.txt
>>> @@ -32,7 +32,8 @@ OPTIONS
>>>        If one or more patterns are given, only refs are shown that
>>>        match against at least one pattern, either using fnmatch(3) or
>>>        literally, in the latter case matching completely or from the
>>> -     beginning up to a slash.
>>> +     beginning up to a slash. If an empty string is provided all refs
>>> +     are printed, including HEAD and pseudorefs.
>>
>> I think it would be helpful to clarify that it is all the refs for the
>> current worktree that are printed, not all the refs in the repository -
>> we still don't have a way to iterate over the per-worktree refs from
>> other worktrees
>>
> 
> I agree, based on if we keep the current commits or remove them, I'll
> send in a new patch or amend the current series.
> 
> Thanks!

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Karthik Nayak]

Hello,

On Tue, Feb 6, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Karthik
>
> On 06/02/2024 08:52, Karthik Nayak wrote:
> > On Mon, Feb 5, 2024 at 7:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >> On 29/01/2024 11:35, Karthik Nayak wrote:
> >>> When the user uses an empty string pattern (""), we don't match any refs
> >>> in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
> >>> path based matching and an empty string doesn't match any path.
> >>>
> >>> In this commit we change this behavior by making empty string pattern
> >>> match all references. This is done by introducing a new flag
> >>> `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
> >>> introduced `refs_for_each_all_refs()` function to iterate over all the
> >>> refs in the repository.
> >>
> >> It actually iterates over all the refs in the current worktree, not all
> >> the refs in the repository. I have to say that I find it extremely
> >> unintuitive that "" behaves differently to not giving a pattern. I
> >> wonder if we can find a better UI here - perhaps a command line option
> >> to include pseudorefs?
> >>
> >
> > As Patrick mentioned, this was discussed a while ago and we decided to
> > move forward with the `git for-each-ref ""` syntax.
>
> Thanks I'd missed that discussion. I see that at the end of that
> discussion Junio was concerned that the proposed "" did not account for
> "refs/worktrees/$worktree/*" [1] - how has that been resolved?
>

The current implementation (merged to next) prints all the refs from the current
worktree and doesn't support printing from other worktrees.

    $ git worktree add ../wt-files @~10
    Preparing worktree (detached HEAD 559d5138bc)
    HEAD is now at 559d5138bc Merge branch 'jc/make-libpath-template' into next

    $ cd ../wt-files/
    direnv: unloading

    $ git for-each-ref "" | grep -v "refs"
    559d5138bc8b81354fd1bc3421ace614076e66de commit    HEAD
    559d5138bc8b81354fd1bc3421ace614076e66de commit    ORIG_HEAD

    $ git rebase -i @~3
    Stopped at be65f9ef88...  t/Makefile: get UNIT_TESTS list from C sources
    You can amend the commit now, with

      git commit --amend '-S'

    Once you are satisfied with your changes, run

      git rebase --continue

    $ git for-each-ref "" | grep -v "refs"
    be65f9ef88ff741454dcf10a0af6e384d0bf26cf commit    HEAD
    43f081b9c7dfb9c23e547ffee1778af0f30c5c4e commit    ORIG_HEAD
    be65f9ef88ff741454dcf10a0af6e384d0bf26cf commit    REBASE_HEAD

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Thanks I'd missed that discussion. I see that at the end of that
> discussion Junio was concerned that the proposed "" did not account
> for "refs/worktrees/$worktree/*" [1] - how has that been resolved?

Ah, that is an excellent point.

If we plan to never allow showing refs/worktrees/ hierarchy, then
the "there is a default pattern, refs/, that gets used when there is
no user-specified patterns" model would be sufficient to allow
showing things that are directly beneath $GIT_DIR and are out of
refs/ hierarchy, but that does not explain why refs/worktrees/ is
omitted.  I'll envision a design for a longer term later, but an
easy way out would be to add --include-worktree-refs option for
that, and at that point, adding --include-root-refs option for things
outside the refs/ hierarchy may become a lot more natural solution.

In the longer term, I suspect that we would want something similar
to the negative refspec magic (e.g., "git log ':!Documentation/'"
that shows things outside the named hierarchy) exposed to the API[*],
so that we can say

    $ git for-each-ref --format=... \
	refs/ !refs/heads/ !refs/tags/ !refs/remotes/

to show things under refs/ excluding the commonly used hierarchies,
and at that point, the current behaviour for "no limit" case can
again become explainable as having "refs/ !refs/worktrees/" as the
default.  It still does not explain why "git for-each-ref refs/"
omits the refs/worktrees/ hierchy, unless the default limit pattern
rule were something like "unless you give a positive limit pattern
rule, then we use 'refs/' by default, and unless you give a negative
limit pattern rule, then we use '!refs/worktrees/' by default".

It then gives an easy explanation on the traditional behaviour, with
"" used for "including stuff outside refs/", and is more flexible.

The use of dashed-options to include hierachies that are by default
excluded (e.g. "--include-root-refs" and "--include-worktree-refs")
feels limiting, but should be sufficient for our needs, both current
(i.e. I want to see HEAD and FETCH_HEAD) and in the immediate future
(i.e. I want to see worktree refs from that worktree), and I can buy
that as a good alternative solution, at least in the shorter term.

I still worry that it may make introducing the negative ref patterns
harder, though.  How does --include-worktree-refs=another to include
the worktree refs from another worktree in refs/worktrees/another
interact with a negative pattern that was given from the command
line that overlaps with it?  Whatever interaction rules we define,
can we easily explain it in the documentation?

Just like "an empty string tells Git to include everything" is a
perfectly reasonable approach if we plan to never allow
refs/worktrees/ hierarchy, "dashed-options for selected hierarchies"
is a perfectly reasonable approach if we plan to never allow
negative limit patterns, I suspect.  We should stop complexity at
some point, and the decision to never support negative limit
patterns might be the place to draw that line.  I dunno.


[Footnote]

 * Such an exclusion mechanism already exists and are used to hide
   certain refs from being seen over the network by "git fetch" and
   friends.  I do not think it is plugged to the machinery used by
   for-each-ref and friends, but it smells like a reasonably easy
   thing to do.

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> Thanks I'd missed that discussion. I see that at the end of that
>> discussion Junio was concerned that the proposed "" did not account
>> for "refs/worktrees/$worktree/*" [1] - how has that been resolved?
>
> Ah, that is an excellent point.
> ...
> The use of dashed-options to include hierachies that are by default
> excluded (e.g. "--include-root-refs" and "--include-worktree-refs")
> feels limiting, but should be sufficient for our needs, both current
> (i.e. I want to see HEAD and FETCH_HEAD) and in the immediate future
> (i.e. I want to see worktree refs from that worktree), and I can buy
> that as a good alternative solution, at least in the shorter term.
>
> I still worry that it may make introducing the negative ref patterns
> harder, though.  How does --include-worktree-refs=another to include
> the worktree refs from another worktree in refs/worktrees/another
> interact with a negative pattern that was given from the command
> line that overlaps with it?  Whatever interaction rules we define,
> can we easily explain it in the documentation?
>
> Just like "an empty string tells Git to include everything" is a
> perfectly reasonable approach if we plan to never allow
> refs/worktrees/ hierarchy, "dashed-options for selected hierarchies"
> is a perfectly reasonable approach if we plan to never allow
> negative limit patterns, I suspect.  We should stop complexity at
> some point, and the decision to never support negative limit
> patterns might be the place to draw that line.  I dunno.

For now, let's block the kn/for-all-refs topic until we figure out
the UI issue.  Which means this (and the reftable integration that
started to depend on it) will not be in the upcoming release.

FWIW, I am leaning towards "a set of narrowly targetted command line
options (like "--include-root-refs")" approach over "a empty string
defeats the built-in default of 'refs/' limit".

Thanks.

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>> Thanks I'd missed that discussion. I see that at the end of that
>>> discussion Junio was concerned that the proposed "" did not account
>>> for "refs/worktrees/$worktree/*" [1] - how has that been resolved?
>>
>> Ah, that is an excellent point.
>> ...
>> The use of dashed-options to include hierachies that are by default
>> excluded (e.g. "--include-root-refs" and "--include-worktree-refs")
>> feels limiting, but should be sufficient for our needs, both current
>> (i.e. I want to see HEAD and FETCH_HEAD) and in the immediate future
>> (i.e. I want to see worktree refs from that worktree), and I can buy
>> that as a good alternative solution, at least in the shorter term.
>>
>> I still worry that it may make introducing the negative ref patterns
>> harder, though.  How does --include-worktree-refs=another to include
>> the worktree refs from another worktree in refs/worktrees/another
>> interact with a negative pattern that was given from the command
>> line that overlaps with it?  Whatever interaction rules we define,
>> can we easily explain it in the documentation?
>>
>> Just like "an empty string tells Git to include everything" is a
>> perfectly reasonable approach if we plan to never allow
>> refs/worktrees/ hierarchy, "dashed-options for selected hierarchies"
>> is a perfectly reasonable approach if we plan to never allow
>> negative limit patterns, I suspect.  We should stop complexity at
>> some point, and the decision to never support negative limit
>> patterns might be the place to draw that line.  I dunno.
>
> For now, let's block the kn/for-all-refs topic until we figure out
> the UI issue.  Which means this (and the reftable integration that
> started to depend on it) will not be in the upcoming release.
>

I think it makes sense to remove the kn/for-all-refs topic for now.

I also think that the reftable integration branch can go forward though,
since the difference between v2 and v3 of that series was simply that
v3 was rebased on top of kn/for-all-refs. So we can continue using v2.

> FWIW, I am leaning towards "a set of narrowly targetted command line
> options (like "--include-root-refs")" approach over "a empty string
> defeats the built-in default of 'refs/' limit".

I think this was what the earlier discussion in the RFC series was too,
but Phillip definitely helped consolidate the point.

I'll send a new version of this patch series with `--include-root-refs`
option and we can discuss on top of that.

Thanks all!

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

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> I think this was what the earlier discussion in the RFC series was too,
> but Phillip definitely helped consolidate the point.
>
> I'll send a new version of this patch series with `--include-root-refs`
> option and we can discuss on top of that.

Thanks.

By the way, I am not married to the "root refs" name, but

 - we do not want to say "all refs", as I expect refs from other
   worktrees are not included, and possibly the option when
   combined with explicit patterns, like refs/tags/, may further be
   used to limit the output;

 - we do not want to say "pseudo refs", as I expect we would want to
   show HEAD that is (unfortunately) classified outside "pseudoref"
   class.

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Karthik Nayak]

On Tue, Feb 6, 2024 at 11:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > I think this was what the earlier discussion in the RFC series was too,
> > but Phillip definitely helped consolidate the point.
> >
> > I'll send a new version of this patch series with `--include-root-refs`
> > option and we can discuss on top of that.
>
> Thanks.
>
> By the way, I am not married to the "root refs" name, but
>
>  - we do not want to say "all refs", as I expect refs from other
>    worktrees are not included, and possibly the option when
>    combined with explicit patterns, like refs/tags/, may further be
>    used to limit the output;
>
>  - we do not want to say "pseudo refs", as I expect we would want to
>    show HEAD that is (unfortunately) classified outside "pseudoref"
>    class.
>

I'm thinking "--all-ref-types" might be a good alternative. Mostly because,
"--include-root-refs" seems very specific to the files backend. Also, we don't
include other refs which are not HEAD | pseudorefs, but in the $GIT_DIR.

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> I'm thinking "--all-ref-types" might be a good alternative. Mostly because,
> "--include-root-refs" seems very specific to the files backend. Also, we don't
> include other refs which are not HEAD | pseudorefs, but in the $GIT_DIR.

I strongly disagree wiht the "files backend specific" part of the
comment.  No matter what backend you would use, refs and pseudorefs
have the full refname, which may look like "HEAD", "FETCH_HEAD",
"refs/heads/maint", etc., and you can easily see these full refnames
form a tree structure, with "HEAD", "FETCH_HEAD", "refs/" at the
root level.

I do not understand your "we don't include other refs", either.
There may be "things" that are ignored by your implementation of
"for-each-ref ''" even with the files backend in $GIT_DIR directory.
They are not refs, because the refs are by definition inside "refs/"
hierarchy, unless they are ones that are specifically included from
outside the hierarchy ("pseudorefs" is one class of specific
exception, "HEAD" is another).

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Karthik Nayak]

On Wed, Feb 7, 2024 at 5:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > I'm thinking "--all-ref-types" might be a good alternative. Mostly because,
> > "--include-root-refs" seems very specific to the files backend. Also, we don't
> > include other refs which are not HEAD | pseudorefs, but in the $GIT_DIR.
>
> I strongly disagree wiht the "files backend specific" part of the
> comment.  No matter what backend you would use, refs and pseudorefs
> have the full refname, which may look like "HEAD", "FETCH_HEAD",
> "refs/heads/maint", etc., and you can easily see these full refnames
> form a tree structure, with "HEAD", "FETCH_HEAD", "refs/" at the
> root level.

I conceded to this point, I was thinking "root" here refers to $GIT_DIR
and this structuring comes from the files backend. But I see the flaw there
that irrelevant of the backend, there is a tree hierarchy built up and for refs
without prefixes, it can be considered as "root".

> I do not understand your "we don't include other refs", either.
> There may be "things" that are ignored by your implementation of
> "for-each-ref ''" even with the files backend in $GIT_DIR directory.
> They are not refs, because the refs are by definition inside "refs/"
> hierarchy, unless they are ones that are specifically included from
> outside the hierarchy ("pseudorefs" is one class of specific
> exception, "HEAD" is another).

This is a bit of a grey area, what I mean is that we do allow users to create
non "refs/" prefixed refs:

    $ git update-ref foo @~1

    $ cat .git/foo
    2b52187cd2930931c6d34436371f470bb26eef4f

What I mean to say is that, by saying "--include-root-refs" it seems to imply
that any such refs should be included too, but this simply is not the case.

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> This is a bit of a grey area, what I mean is that we do allow users to create
> non "refs/" prefixed refs:
>
>     $ git update-ref foo @~1
>
>     $ cat .git/foo
>     2b52187cd2930931c6d34436371f470bb26eef4f
>
> What I mean to say is that, by saying "--include-root-refs" it seems to imply
> that any such refs should be included too, but this simply is not the case.

But isn't that a quality of implementation issue?  I'd consider it a
bug once we have and enforce the definition of what pseudorefs are.

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Karthik Nayak]

On Wed, Feb 7, 2024 at 5:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > This is a bit of a grey area, what I mean is that we do allow users to create
> > non "refs/" prefixed refs:
> >
> >     $ git update-ref foo @~1
> >
> >     $ cat .git/foo
> >     2b52187cd2930931c6d34436371f470bb26eef4f
> >
> > What I mean to say is that, by saying "--include-root-refs" it seems to imply
> > that any such refs should be included too, but this simply is not the case.
>
> But isn't that a quality of implementation issue?  I'd consider it a
> bug once we have and enforce the definition of what pseudorefs are.

Yeah, that makes sense. I'll use "--include-root-refs" then.

Thanks

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Patrick Steinhardt]

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

On Wed, Feb 07, 2024 at 06:02:34PM +0100, Karthik Nayak wrote:
> On Wed, Feb 7, 2024 at 5:46 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Karthik Nayak <karthik.188@gmail.com> writes:
> >
> > > This is a bit of a grey area, what I mean is that we do allow users to create
> > > non "refs/" prefixed refs:
> > >
> > >     $ git update-ref foo @~1
> > >
> > >     $ cat .git/foo
> > >     2b52187cd2930931c6d34436371f470bb26eef4f
> > >
> > > What I mean to say is that, by saying "--include-root-refs" it seems to imply
> > > that any such refs should be included too, but this simply is not the case.
> >
> > But isn't that a quality of implementation issue?  I'd consider it a
> > bug once we have and enforce the definition of what pseudorefs are.
> 
> Yeah, that makes sense. I'll use "--include-root-refs" then.

I'd still argue that this is too specific to the files backend. The fact
that we end up only adding root refs to the files backend to me is an
implementation-specific limitation and not a feature. What I really want
is to ask the backend to give me all refs that it knows of for the
current worktree.

The "reftable" backend has this snippet in its iterator:

```
/*
 * The files backend only lists references contained in
 * "refs/". We emulate the same behaviour here and thus skip
 * all references that don't start with this prefix.
 */
if (!starts_with(iter->ref.refname, "refs/"))
    continue;
```

If we add the new `--include-root-refs` flag then we would have to
extend it to query whether the ref either starts with "refs/" or whether
it might look like a pseudo-ref:

```
if (!starts_with(iter->ref.refname, "refs/") &&
    !(flags & INCLUDE_ROOT_REFS || is_pseudoref(iter->ref.refname)))
    continue;
```

The problem I have is that it still wouldn't end up surfacing all refs
which exist in the ref backend while being computationally more
expensive. So the original usecase I had in mind when pitching this
topic isn't actually addressed.

I know that in theory, the reftable backend shouldn't contain refs other
than "refs/" or pseudo-refs anyway. But regardless of that, I think that
formulating this in the form of "root refs" is too restrictive and too
much focussed on the "files" backend. I wouldn't be surprised if there
are ways to have git-update-ref(1) or other tools write refs with
unexpected names, and not being able to learn about those is a
limitation.

Putting this in the context of "Give me all refs which you know of for
the current worktree" is a lot simpler. It would still be equivalent to
"--include-root-refs" for the "files" backend, because the files backend
doesn't have a way to properly track all refs it has ever created. And
for the "reftable" backend it's as simple as this:

```
if (!(iter->flags & DO_FOR_EACH_INCLUDE_ALL_REFS) &&
    !starts_with(iter->ref.refname, "refs/"))
```

I'm not sure about better terminology though. Something like
`--include-all-worktree-refs` could easily lead to confusion because it
might make the user think that they also want to list refs from other
worktrees. The best I can come up with is `--include-non-refs` -- pseudo
refs aren't real refs, and neither are refs which don't start with
"refs/".

Patrick

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

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> ```
> if (!starts_with(iter->ref.refname, "refs/") &&
>     !(flags & INCLUDE_ROOT_REFS || is_pseudoref(iter->ref.refname)))
>     continue;
> ```
>
> The problem I have is that it still wouldn't end up surfacing all refs
> which exist in the ref backend while being computationally more
> expensive. So the original usecase I had in mind when pitching this
> topic isn't actually addressed.

The reftable format, as a database format, may be capable of having
"refs/heads/master" and "refs/heads/master/1" at the same time, but
to be used as a ref backend for Git, it must refrain from surfacing
both at the same time.  I think it is the same deal that it should
only allow "refs/*", "HEAD", and so called pseudorefs to be stored.
So INCLUDE_ROOT_REFS should be sufficient as long as the "ref
creation and update" side is not letting random cruft (e.g.,
"config") in.  Isn't that sufficient?

> I know that in theory, the reftable backend shouldn't contain refs other
> than "refs/" or pseudo-refs anyway. But regardless of that, I think that
> formulating this in the form of "root refs" is too restrictive and too
> much focussed on the "files" backend.

It is not "focused on".  The ref namespace of Git is tree-shaped,
period.  The shape may have originated from its first ref backend
implementation's limitation, but as we gain other backends, we are
not planning to lift such limitations, are we?  So we may still say
"when there is a master branch, you cannot have master/1 branch (due
to D/F conflict)", even if there is no notion of directory or file
in a backend implementation backed by a databasy file format.  "HEAD"
and "CHERRY_PICK_HEAD", unlike "refs/tags/v1.0", are at the "root
level", not only when they are stored in a files backend, but always
when they are presented to end-users, who can tell that they are not
inside "refs/".

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Patrick Steinhardt]

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

On Thu, Feb 08, 2024 at 09:04:54AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > ```
> > if (!starts_with(iter->ref.refname, "refs/") &&
> >     !(flags & INCLUDE_ROOT_REFS || is_pseudoref(iter->ref.refname)))
> >     continue;
> > ```
> >
> > The problem I have is that it still wouldn't end up surfacing all refs
> > which exist in the ref backend while being computationally more
> > expensive. So the original usecase I had in mind when pitching this
> > topic isn't actually addressed.
> 
> The reftable format, as a database format, may be capable of having
> "refs/heads/master" and "refs/heads/master/1" at the same time, but
> to be used as a ref backend for Git, it must refrain from surfacing
> both at the same time.  I think it is the same deal that it should
> only allow "refs/*", "HEAD", and so called pseudorefs to be stored.
> So INCLUDE_ROOT_REFS should be sufficient as long as the "ref
> creation and update" side is not letting random cruft (e.g.,
> "config") in.  Isn't that sufficient?

That's a different problem from the one I have right now. Let's take the
following sequence of commands:

    $ git init repo
    Initialized empty Git repository in /tmp/repo/.git/
    $ git -C repo commit --allow-empty --message message
    [main (root-commit) aa5eec4] message
    $ git -C repo update-ref ref/head/foo HEAD
    $ ls repo/.git/ref/head/foo
    repo/.git/ref/head/foo

Now the fact that you can create "ref/head/foo" is a bug that needs to
be fixed, no arguing there. The problem is that rectifying this problem
with the "files" backend is easy -- you look into the repo, notice that
there's a weird directory, and then "rm -rf" it.

But how do you learn about this ref existing with the "reftable" backend
in the first place? You can't without looking at the binary format --
there doesn't exist a single command that would allow you to list all
refs unfiltered. But that is very much required in order to learn about
misbehaviour and fix it.

As I said -- this is a bug, and I agree that it shouldn't happen. But
bugs happen, and especially with the new reftable format I expect them
to happen. What I look for in this context is to create the tools to fix
problems like this, but `--include-root-refs` doesn't. A flag that
unconditionally returns all refs, regardless of whether they have a bad
name or not, does address the issue. Think of it of more of a debugging
tool.

Spelled out like that it brings me a different idea: maybe I'm just
trying to address this in the wrong tool. I plan to introduce ref
backend specific fsck checks, so that could be a better place to warn
about such refs with bad names. Like this we don't erode the tree-shaped
nature by somehow accepting them in some tools, and we make clear that
this is indeed something that shouldn't happen.

> > I know that in theory, the reftable backend shouldn't contain refs other
> > than "refs/" or pseudo-refs anyway. But regardless of that, I think that
> > formulating this in the form of "root refs" is too restrictive and too
> > much focussed on the "files" backend.
> 
> It is not "focused on".  The ref namespace of Git is tree-shaped,
> period.  The shape may have originated from its first ref backend
> implementation's limitation, but as we gain other backends, we are
> not planning to lift such limitations, are we?  So we may still say
> "when there is a master branch, you cannot have master/1 branch (due
> to D/F conflict)", even if there is no notion of directory or file
> in a backend implementation backed by a databasy file format.  "HEAD"
> and "CHERRY_PICK_HEAD", unlike "refs/tags/v1.0", are at the "root
> level", not only when they are stored in a files backend, but always
> when they are presented to end-users, who can tell that they are not
> inside "refs/".

I agree, and I do not intend to change this.

Patrick

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

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> That's a different problem from the one I have right now. Let's take the
> following sequence of commands:
>
>     $ git init repo
>     Initialized empty Git repository in /tmp/repo/.git/
>     $ git -C repo commit --allow-empty --message message
>     [main (root-commit) aa5eec4] message
>     $ git -C repo update-ref ref/head/foo HEAD
>     $ ls repo/.git/ref/head/foo
>     repo/.git/ref/head/foo
>
> Now the fact that you can create "ref/head/foo" is a bug that needs to
> be fixed, no arguing there. The problem is that rectifying this problem
> with the "files" backend is easy -- you look into the repo, notice that
> there's a weird directory, and then "rm -rf" it.

OK.

> But how do you learn about this ref existing with the "reftable" backend
> in the first place? You can't without looking at the binary format --
> there doesn't exist a single command that would allow you to list all
> refs unfiltered. But that is very much required in order to learn about
> misbehaviour and fix it.

I think I have been saying that it is perfectly OK if reftable
backend, being newer and backed by more experience using Git,
rejected any attempt to create anything under "ref/" (to avoid
confusion to those who are reading from sidelines, it should allow
creating "refs/mytool/" for third-party tools to store their own
pointers).

> As I said -- this is a bug, and I agree that it shouldn't happen. But
> bugs happen, and especially with the new reftable format I expect them
> to happen. What I look for in this context is to create the tools to fix
> problems like this, but `--include-root-refs` doesn't. A flag that
> unconditionally returns all refs, regardless of whether they have a bad
> name or not, does address the issue. Think of it of more of a debugging
> tool.

OK, "--include-all-refs" would be fine.  And without bugs there
should not be a difference.

Where does "all refs in this worktree" you mentioned fit in this
picture?  Should a bogus "ref/foo/bar" be considered to be worktree
specific, or is it an incorrect attempt to create a ref that is
specific to 'foo' worktree that is not the current one and be
filtered out?

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Patrick Steinhardt]

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

On Thu, Feb 08, 2024 at 09:53:24AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > As I said -- this is a bug, and I agree that it shouldn't happen. But
> > bugs happen, and especially with the new reftable format I expect them
> > to happen. What I look for in this context is to create the tools to fix
> > problems like this, but `--include-root-refs` doesn't. A flag that
> > unconditionally returns all refs, regardless of whether they have a bad
> > name or not, does address the issue. Think of it of more of a debugging
> > tool.
> 
> OK, "--include-all-refs" would be fine.  And without bugs there
> should not be a difference.
> 
> Where does "all refs in this worktree" you mentioned fit in this
> picture?  Should a bogus "ref/foo/bar" be considered to be worktree
> specific, or is it an incorrect attempt to create a ref that is
> specific to 'foo' worktree that is not the current one and be
> filtered out?

Good question indeed. The reason I said "all refs in this worktree" is
mostly that I don't think we should also be returning refs from other
worktrees. I consider their refdbs to be separate refdbs, even though it
is possible to access them via the special "worktrees/$wt/refs" syntax.
So I would say that such an option should return all refs of the current
worktree's refdb, plus the common worktree refs.

Another important question here is how the "files" backend should behave
with such a flag. Should it try to read all loose files as refs and
return those which just happen to look like one? That feels really wrong
to me, as we would now have to special case those namespaces where we
know that it's not a proper ref, like "worktrees/", "rebase-apply/" and
others.

The alternative would be to make it behave like `--include-root-refs`,
where we do a best effort and live with the fact that the "files"
backend cannot enumerate all refs it has created. This would mean that
behaviour between the "files" and "reftable" backend is diverging though
because the latter can trivially figure out all refs it contains. The
question is whether we are fine with that or not.

Depending on the answer, I think we can go one of two ways:

  - Accept the diverging behaviour and add `--include-all-refs`. The
    "files" backend does a best effort and only includes root refs, the
    "reftable" backend lists all refs.

  - Double down on the fact that refs must either be pseudo refs or
    start with "refs/" and treat any ref that doesn't fit this bill as
    corrupted. The consequence here would be that we instead go with
    `--include-root-refs` that can be implemented the same for both
    backends. In addition, we add checks to git-fsck(1) to surface and
    flag refs with bogus names for the "reftable" backend.

While I seem to have convinced you that `--include-all-refs` might not
be a bad idea after all, you have convinced me by now that the second
option would be preferable. I'd be okay with either of these options as
both of them address the issue at hand.

Patrick

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

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Depending on the answer, I think we can go one of two ways:
>
>   - Accept the diverging behaviour and add `--include-all-refs`. The
>     "files" backend does a best effort and only includes root refs, the
>     "reftable" backend lists all refs.
>
>   - Double down on the fact that refs must either be pseudo refs or
>     start with "refs/" and treat any ref that doesn't fit this bill as
>     corrupted. The consequence here would be that we instead go with
>     `--include-root-refs` that can be implemented the same for both
>     backends. In addition, we add checks to git-fsck(1) to surface and
>     flag refs with bogus names for the "reftable" backend.
>
> While I seem to have convinced you that `--include-all-refs` might not
> be a bad idea after all, you have convinced me by now that the second
> option would be preferable. I'd be okay with either of these options as
> both of them address the issue at hand.

For now my tentative preference is the latter.  If ref/head/foo is
an end-user mistake with one backend, it is cleaner if it is
considered a mistake with other backends.

Doesn't our ref enumeration/iteration API have "include broken ones
as well" bit?  I wonder if this issue becomes easier to solve by
(re|ab)using that bit.

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Karthik Nayak]

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

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> Depending on the answer, I think we can go one of two ways:
>>
>>   - Accept the diverging behaviour and add `--include-all-refs`. The
>>     "files" backend does a best effort and only includes root refs, the
>>     "reftable" backend lists all refs.
>>
>>   - Double down on the fact that refs must either be pseudo refs or
>>     start with "refs/" and treat any ref that doesn't fit this bill as
>>     corrupted. The consequence here would be that we instead go with
>>     `--include-root-refs` that can be implemented the same for both
>>     backends. In addition, we add checks to git-fsck(1) to surface and
>>     flag refs with bogus names for the "reftable" backend.
>>
>> While I seem to have convinced you that `--include-all-refs` might not
>> be a bad idea after all, you have convinced me by now that the second
>> option would be preferable. I'd be okay with either of these options as
>> both of them address the issue at hand.
>
> For now my tentative preference is the latter.  If ref/head/foo is
> an end-user mistake with one backend, it is cleaner if it is
> considered a mistake with other backends.
>
> Doesn't our ref enumeration/iteration API have "include broken ones
> as well" bit?  I wonder if this issue becomes easier to solve by
> (re|ab)using that bit.

I'll then go ahead with point 2 then.

I'll modify my patch series for now to fit in and will follow up "checks
to git-fsck(1) to surface and flag refs with bogus names for the
"reftable" backend" in a follow up series.

- Karthik

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

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Patrick Steinhardt]

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

On Fri, Feb 09, 2024 at 06:27:39PM +0000, Karthik Nayak wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > Patrick Steinhardt <ps@pks.im> writes:
> >> Depending on the answer, I think we can go one of two ways:
> >>
> >>   - Accept the diverging behaviour and add `--include-all-refs`. The
> >>     "files" backend does a best effort and only includes root refs, the
> >>     "reftable" backend lists all refs.
> >>
> >>   - Double down on the fact that refs must either be pseudo refs or
> >>     start with "refs/" and treat any ref that doesn't fit this bill as
> >>     corrupted. The consequence here would be that we instead go with
> >>     `--include-root-refs` that can be implemented the same for both
> >>     backends. In addition, we add checks to git-fsck(1) to surface and
> >>     flag refs with bogus names for the "reftable" backend.
> >>
> >> While I seem to have convinced you that `--include-all-refs` might not
> >> be a bad idea after all, you have convinced me by now that the second
> >> option would be preferable. I'd be okay with either of these options as
> >> both of them address the issue at hand.
> >
> > For now my tentative preference is the latter.  If ref/head/foo is
> > an end-user mistake with one backend, it is cleaner if it is
> > considered a mistake with other backends.
> >
> > Doesn't our ref enumeration/iteration API have "include broken ones
> > as well" bit?  I wonder if this issue becomes easier to solve by
> > (re|ab)using that bit.
> 
> I'll then go ahead with point 2 then.
> 
> I'll modify my patch series for now to fit in and will follow up "checks
> to git-fsck(1) to surface and flag refs with bogus names for the
> "reftable" backend" in a follow up series.

Thanks. Note that the fsck checks are also proposed as one of the GSoC
projects where you're listed as a mentor. Might be worth it to hold back
until we know whether any student wants to work on it.

Patrick

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

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Phillip Wood]

Hi Juino

On 06/02/2024 22:16, Junio C Hamano wrote:
>   - we do not want to say "pseudo refs", as I expect we would want to
>     show HEAD that is (unfortunately) classified outside "pseudoref"
>     class.

This is a bit of a tangent but I've been wondering what the practical 
benefit of distinguishing between "HEAD" and "pseudoref" is. I don't 
know the history so there may be a good reason but not classifying 
"HEAD" as a "pseudoref" seems to make discussions like this more 
complicated than they would be if "HEAD" were a "pseudoref". Would it be 
beneficial to loosen the definition of "psuedoref" to remove the 
restriction that they may not be symbolic refs or have a reflog?

Best Wishes

Phillip

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> This is a bit of a tangent but I've been wondering what the practical
> benefit of distinguishing between "HEAD" and "pseudoref" is.

I hate that distinction too.  The practical difference they gave me
was that we historically never made FOO_HEAD to be a symref, and we
do not want them to be in the future, but for HEAD, it is perfectly
natural to be a symref.

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Patrick Steinhardt]

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

On Tue, Feb 06, 2024 at 02:10:41PM -0800, Karthik Nayak wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > Junio C Hamano <gitster@pobox.com> writes:
> >> Phillip Wood <phillip.wood123@gmail.com> writes:
[snip]
> > For now, let's block the kn/for-all-refs topic until we figure out
> > the UI issue.  Which means this (and the reftable integration that
> > started to depend on it) will not be in the upcoming release.
> >
> 
> I think it makes sense to remove the kn/for-all-refs topic for now.
> 
> I also think that the reftable integration branch can go forward though,
> since the difference between v2 and v3 of that series was simply that
> v3 was rebased on top of kn/for-all-refs. So we can continue using v2.

I've sent a rebased v4 that evicted the kn/for-all-refs dependency. This
also allowed me to adapt to some fixes for the reftable library which
have been merged down to `master` now so that the corresponding tests
are now with `test_expect_success`.

Patrick

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

Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> I also think that the reftable integration branch can go forward though,
>> since the difference between v2 and v3 of that series was simply that
>> v3 was rebased on top of kn/for-all-refs. So we can continue using v2.
>
> I've sent a rebased v4 that evicted the kn/for-all-refs dependency. This
> also allowed me to adapt to some fixes for the reftable library which
> have been merged down to `master` now so that the corresponding tests
> are now with `test_expect_success`.

Thanks, will take a look.

Re: [PATCH v3 0/4] for-each-ref: print all refs on empty string pattern

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> This is the second version of my patch series to print refs
> when and empty string pattern is used with git-for-each-ref(1).

Thanks.  This is probably the third if I am counting correctly, and
the changes relative to the previous round match what I was
expecting to see.

Will queue.

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

[Karthik Nayak]

This is the forth 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 v3:
1. Move from using 'git for-each-ref ""' to print root refs to adding
the '--include-root-refs' option for git-for-each-ref(1). This provides better
UX for users.
2. Modify `is_pseudoref()` to use `refs_resolve_ref_unsafe`.
3. Includes reftable-backend changes and is now rebased on top of next (ed35d3359).  

Range-diff:

1:  2141a2a62b ! 1:  98130a7ad7 refs: introduce `is_pseudoref()` and `is_headref()`
    @@ Commit message
         related files like 'BISECT_START' to a new directory similar to the
         'rebase-merge' directory.
     
    +    Helped-by: Jeff King <peff@peff.net>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## refs.c ##
    @@ refs.c: static int is_pseudoref_syntax(const char *refname)
     +		return 0;
     +
     +	if (ends_with(refname, "_HEAD")) {
    -+		 read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
    -+		      &oid, NULL);
    -+		 return !is_null_oid(&oid);
    ++		refs_resolve_ref_unsafe(refs, refname,
    ++   					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])) {
    -+			read_ref_full(refname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
    -+						  &oid, NULL);
    ++			refs_resolve_ref_unsafe(refs, refname,
    ++   						RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
    ++   						&oid, NULL);
     +			return !is_null_oid(&oid);
     +		}
     +
2:  c96f0a9c83 = 2:  060ab08af5 refs: extract out `loose_fill_ref_dir_regular_file()`
3:  d165358b83 ! 3:  40d2375ad9 refs: introduce `refs_for_each_all_refs()`
    @@ Metadata
     Author: Karthik Nayak <karthik.188@gmail.com>
     
      ## Commit message ##
    -    refs: introduce `refs_for_each_all_refs()`
    +    refs: introduce `refs_for_each_include_root_refs()`
     
    -    Introduce a new ref iteration flag `DO_FOR_EACH_INCLUDE_ALL_REFS`, which
    -    will be used to iterate over all refs. In the files backend this is
    -    limited to regular refs, pseudorefs and HEAD. For other backends like
    -    the reftable this is the universal set of all refs stored in the
    -    backend.
    +    Introduce a new ref iteration flag `DO_FOR_EACH_INCLUDE_ROOT_REFS`,
    +    which will be used to iterate over regular refs plus pseudorefs and
    +    HEAD.
     
         Refs which fall outside the `refs/` and aren't either pseudorefs or HEAD
         are more of a grey area. This is because we don't block the users from
    -    creating such refs but they are not officially supported. In the files
    -    backend, we can isolate such files from other files.
    +    creating such refs but they are not officially supported.
     
    -    Introduce `refs_for_each_all_refs()` which calls `do_for_each_ref()`
    -    with this newly introduced flag.
    +    Introduce `refs_for_each_include_root_refs()` which calls
    +    `do_for_each_ref()` with this newly introduced flag.
     
         In `refs/files-backend.c`, introduce a new function
         `add_pseudoref_and_head_entries()` to add pseudorefs and HEAD to the
         `ref_dir`. We then finally call `add_pseudoref_and_head_entries()`
    -    whenever the `DO_FOR_EACH_INCLUDE_ALL_REFS` flag is set. Any new ref
    +    whenever the `DO_FOR_EACH_INCLUDE_ROOT_REFS` flag is set. Any new ref
         backend will also have to implement similar changes on its end.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
    @@ refs.c: int for_each_rawref(each_ref_fn fn, void *cb_data)
      	return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data);
      }
      
    -+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
    -+			   void *cb_data)
    ++int refs_for_each_include_root_refs(struct ref_store *refs, each_ref_fn fn,
    ++				    void *cb_data)
     +{
     +	return do_for_each_ref(refs, "", NULL, fn, 0,
    -+			       DO_FOR_EACH_INCLUDE_ALL_REFS, cb_data);
    ++			       DO_FOR_EACH_INCLUDE_ROOT_REFS, cb_data);
     +}
     +
      static int qsort_strcmp(const void *va, const void *vb)
    @@ refs.h: int for_each_namespaced_ref(const char **exclude_patterns,
      int for_each_rawref(each_ref_fn fn, void *cb_data);
      
     +/*
    -+ * Iterates over all ref types, regular, pseudorefs and HEAD.
    ++ * Iterates over all refs including root refs, i.e. pseudorefs and HEAD.
     + */
    -+int refs_for_each_all_refs(struct ref_store *refs, each_ref_fn fn,
    -+			   void *cb_data);
    ++int refs_for_each_include_root_refs(struct ref_store *refs, each_ref_fn fn,
    ++				    void *cb_data);
     +
      /*
       * Normalizes partial refs to their fully qualified form.
    @@ refs/files-backend.c: static struct ref_cache *get_loose_ref_cache(struct files_
      
     +		dir = get_ref_dir(refs->loose->root);
     +
    -+		if (flags & DO_FOR_EACH_INCLUDE_ALL_REFS)
    ++		if (flags & DO_FOR_EACH_INCLUDE_ROOT_REFS)
     +			add_pseudoref_and_head_entries(dir->cache->ref_store, dir,
    -+										   refs->loose->root->name);
    ++						       refs->loose->root->name);
     +
      		/*
      		 * Add an incomplete entry for "refs/" (to be filled
    @@ refs/refs-internal.h: enum do_for_each_ref_flags {
      	DO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2),
     +
     +	/*
    -+	 * Include all refs in the $GIT_DIR in contrast to generally only listing
    -+	 * references having the "refs/" prefix. In the files-backend this is
    -+	 * limited to regular refs, pseudorefs and HEAD.
    ++	 * Include root refs i.e. HEAD and pseudorefs along with the regular
    ++	 * refs.
     +	 */
    -+	DO_FOR_EACH_INCLUDE_ALL_REFS = (1 << 3),
    ++	DO_FOR_EACH_INCLUDE_ROOT_REFS = (1 << 3),
      };
      
      /*
4:  a17983d0ba < -:  ---------- for-each-ref: avoid filtering on empty pattern
-:  ---------- > 4:  b4b9435505 ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
-:  ---------- > 5:  ee99ac41ae for-each-ref: add new option to include root refs

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             |  11 ++-
 ref-filter.c                       |  29 ++++++-
 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, 238 insertions(+), 46 deletions(-)

-- 
2.43.GIT

[PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

[Karthik Nayak]

Introduce two new functions `is_pseudoref()` and `is_headref()`. This
provides the necessary functionality for us to add pseudorefs and HEAD
to the loose ref cache in the files backend, allowing us to build
tooling to print these refs.

The `is_pseudoref()` function internally calls `is_pseudoref_syntax()`
but adds onto it by also checking to ensure that the pseudoref either
ends with a "_HEAD" suffix or matches a list of exceptions. After which
we also parse the contents of the pseudoref to ensure that it conforms
to the ref format.

We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
because the function is also used by `is_current_worktree_ref()` and
making it stricter to match only known pseudorefs might have unintended
consequences due to files like 'BISECT_START' which isn't a pseudoref
but sometimes contains object ID.

Keeping this in mind, we leave `is_pseudoref_syntax()` as is and create
`is_pseudoref()` which is stricter. Ideally we'd want to move the new
syntax checks to `is_pseudoref_syntax()` but a prerequisite for this
would be to actually remove the exception list by converting those
pseudorefs to also contain a '_HEAD' suffix and perhaps move bisect
related files like 'BISECT_START' to a new directory similar to the
'rebase-merge' directory.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c | 41 +++++++++++++++++++++++++++++++++++++++++
 refs.h |  3 +++
 2 files changed, 44 insertions(+)

diff --git a/refs.c b/refs.c
index fff343c256..d8e4cf9a11 100644
--- a/refs.c
+++ b/refs.c
@@ -860,6 +860,47 @@ static int is_pseudoref_syntax(const char *refname)
 	return 1;
 }
 
+int is_pseudoref(struct ref_store *refs, const char *refname)
+{
+	static const char *const irregular_pseudorefs[] = {
+		"AUTO_MERGE",
+		"BISECT_EXPECTED_REV",
+		"NOTES_MERGE_PARTIAL",
+		"NOTES_MERGE_REF",
+		"MERGE_AUTOSTASH",
+	};
+	struct object_id oid;
+	size_t i;
+
+	if (!is_pseudoref_syntax(refname))
+		return 0;
+
+	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);
+	}
+
+	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);
+			return !is_null_oid(&oid);
+		}
+
+	return 0;
+}
+
+int is_headref(struct ref_store *refs, const char *refname)
+{
+	if (!strcmp(refname, "HEAD"))
+		return refs_ref_exists(refs, refname);
+
+	return 0;
+}
+
 static int is_current_worktree_ref(const char *ref) {
 	return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
 }
diff --git a/refs.h b/refs.h
index 303c5fac4d..f66cdd731c 100644
--- a/refs.h
+++ b/refs.h
@@ -1023,4 +1023,7 @@ extern struct ref_namespace_info ref_namespace[NAMESPACE__COUNT];
  */
 void update_ref_namespace(enum ref_namespace namespace, char *ref);
 
+int is_pseudoref(struct ref_store *refs, const char *refname);
+int is_headref(struct ref_store *refs, const char *refname);
+
 #endif /* REFS_H */
-- 
2.43.GIT

Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

[Patrick Steinhardt]

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

On Sun, Feb 11, 2024 at 07:39:19PM +0100, Karthik Nayak wrote:
> Introduce two new functions `is_pseudoref()` and `is_headref()`. This
> provides the necessary functionality for us to add pseudorefs and HEAD
> to the loose ref cache in the files backend, allowing us to build
> tooling to print these refs.
> 
> The `is_pseudoref()` function internally calls `is_pseudoref_syntax()`
> but adds onto it by also checking to ensure that the pseudoref either
> ends with a "_HEAD" suffix or matches a list of exceptions. After which
> we also parse the contents of the pseudoref to ensure that it conforms
> to the ref format.
> 
> We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
> because the function is also used by `is_current_worktree_ref()` and
> making it stricter to match only known pseudorefs might have unintended
> consequences due to files like 'BISECT_START' which isn't a pseudoref
> but sometimes contains object ID.
> 
> Keeping this in mind, we leave `is_pseudoref_syntax()` as is and create
> `is_pseudoref()` which is stricter. Ideally we'd want to move the new
> syntax checks to `is_pseudoref_syntax()` but a prerequisite for this
> would be to actually remove the exception list by converting those
> pseudorefs to also contain a '_HEAD' suffix and perhaps move bisect
> related files like 'BISECT_START' to a new directory similar to the
> 'rebase-merge' directory.
> 
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  refs.h |  3 +++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/refs.c b/refs.c
> index fff343c256..d8e4cf9a11 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -860,6 +860,47 @@ static int is_pseudoref_syntax(const char *refname)
>  	return 1;
>  }
>  
> +int is_pseudoref(struct ref_store *refs, const char *refname)
> +{
> +	static const char *const irregular_pseudorefs[] = {
> +		"AUTO_MERGE",
> +		"BISECT_EXPECTED_REV",
> +		"NOTES_MERGE_PARTIAL",
> +		"NOTES_MERGE_REF",
> +		"MERGE_AUTOSTASH",
> +	};
> +	struct object_id oid;
> +	size_t i;
> +
> +	if (!is_pseudoref_syntax(refname))
> +		return 0;
> +
> +	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);
> +	}

I think it's quite confusing that `is_pseudoref()` not only checks
whether the refname may be a pseudoref, but also whether it actually
exists. Furthermore, why is a pseudoref only considered to exist in case
it's not a symbolic ref? That sounds overly restrictive to me.

So I think this at least needs to be renamed. But I find it really hard
to come up with a proper name here because in my opinion the function
does too much. `is_existing_pseudoref()` feels much too specific to me.
Also, the "reftable" backend wouldn't need to check whether the ref
exists, but only whether a name that it encounters is a pseudoref name
or not.

> +	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);
> +			return !is_null_oid(&oid);
> +		}
> +
> +	return 0;
> +}
> +
> +int is_headref(struct ref_store *refs, const char *refname)
> +{
> +	if (!strcmp(refname, "HEAD"))
> +		return refs_ref_exists(refs, refname);
> +
> +	return 0;
> +}

The same comment applies here, as well.

I also worry a bit about the API we have. It becomes really hard to
figure out which function to call now as the API surface seems to
explode. We have:

  - is_pseudoref_syntax
  - is_pseudoref
  - is_headref
  - check_refname_format
  - refname_is_safe

I wonder whether we can maybe consolidate the interface into one or
maybe even two functions where the behaviour can be tweaked with a flag
field. Something like `refname_is_valid()` with a bunch of flags:

  - REFNAME_ACCEPT_HEAD to accept "HEAD"
  - REFNAME_ACCEPT_PSEUDOREF to accept all of the refs ending with
    "_HEAD" or being one of the irregular pseudorefs.
  - REFNAME_ACCEPT_INVALID_BUT_SAFE to accept refnames which aren't
    valid, but which would pass `refname_is_safe()`.

Another alternative could be something like `classify_refname()` that
accepts a refname and returns an enum saying what kind of ref something
is.

Given that this topic won't be included in Git v2.44 anymore, I think
that opening this can of worms would be sensible now.

Patrick

>  static int is_current_worktree_ref(const char *ref) {
>  	return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
>  }
> diff --git a/refs.h b/refs.h
> index 303c5fac4d..f66cdd731c 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -1023,4 +1023,7 @@ extern struct ref_namespace_info ref_namespace[NAMESPACE__COUNT];
>   */
>  void update_ref_namespace(enum ref_namespace namespace, char *ref);
>  
> +int is_pseudoref(struct ref_store *refs, const char *refname);
> +int is_headref(struct ref_store *refs, const char *refname);
> +
>  #endif /* REFS_H */
> -- 
> 2.43.GIT
> 

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

Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I think it's quite confusing that `is_pseudoref()` not only checks
> whether the refname may be a pseudoref, but also whether it actually
> exists. Furthermore, why is a pseudoref only considered to exist in case
> it's not a symbolic ref? That sounds overly restrictive to me.

I am torn on this, but in favor of the proposed naming, primarily
because is_pseudoref_syntax() was about "does this string look like
the fullref a pseudoref would have?", and the reason why we wanted
to have this new function was we wanted to ask "does this string
name a valid pseudoref?"

 Q: Is CHERRY_PICK_HEAD a pseudoref?
 A: It would have been if it existed, but I see only
    $GIT_DIR/CHERRY_PICK_HEAD that is a symbolic link, and it cannot
    be a pseudoref.

I can certainly see a broken out set of helper functions to check

 - Does this string make a good fullref for a pseudoref?
 - Does a pseudoref with his string as its fullref exist?

independently.  The first one would answer Yes and the second one
would answer No in such a context.

Thanks.

Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

[Karthik Nayak]

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

Hello,

Patrick Steinhardt <ps@pks.im> writes:
>> +
>> +int is_headref(struct ref_store *refs, const char *refname)
>> +{
>> +	if (!strcmp(refname, "HEAD"))
>> +		return refs_ref_exists(refs, refname);
>> +
>> +	return 0;
>> +}
>
> The same comment applies here, as well.
>
> I also worry a bit about the API we have. It becomes really hard to
> figure out which function to call now as the API surface seems to
> explode. We have:
>
>   - is_pseudoref_syntax
>   - is_pseudoref
>   - is_headref
>   - check_refname_format
>   - refname_is_safe
>

I also found `is_head()` in 'reflog.c'.

> I wonder whether we can maybe consolidate the interface into one or
> maybe even two functions where the behaviour can be tweaked with a flag
> field.
>

You do bring up an interesting point about the APIs present and I agree
that it would be best to consolidate them to something much simpler and
nicer.

> Something like `refname_is_valid()` with a bunch of flags:
>
>   - REFNAME_ACCEPT_HEAD to accept "HEAD"
>   - REFNAME_ACCEPT_PSEUDOREF to accept all of the refs ending with
>     "_HEAD" or being one of the irregular pseudorefs.
>   - REFNAME_ACCEPT_INVALID_BUT_SAFE to accept refnames which aren't
>     valid, but which would pass `refname_is_safe()`.
>
> Another alternative could be something like `classify_refname()` that
> accepts a refname and returns an enum saying what kind of ref something
> is.
>
> Given that this topic won't be included in Git v2.44 anymore, I think
> that opening this can of worms would be sensible now.
>

Over the two I do prefer something like the former method of the callee
using flags to state the requirements, this way the function only does
what is necessary between the listed operations.

Junio C Hamano <gitster@pobox.com> writes:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> I think it's quite confusing that `is_pseudoref()` not only checks
>> whether the refname may be a pseudoref, but also whether it actually
>> exists. Furthermore, why is a pseudoref only considered to exist in case
>> it's not a symbolic ref? That sounds overly restrictive to me.
>
> I am torn on this, but in favor of the proposed naming, primarily
> because is_pseudoref_syntax() was about "does this string look like
> the fullref a pseudoref would have?", and the reason why we wanted
> to have this new function was we wanted to ask "does this string
> name a valid pseudoref?"
>
>  Q: Is CHERRY_PICK_HEAD a pseudoref?
>  A: It would have been if it existed, but I see only
>     $GIT_DIR/CHERRY_PICK_HEAD that is a symbolic link, and it cannot
>     be a pseudoref.
>
> I can certainly see a broken out set of helper functions to check
>
>  - Does this string make a good fullref for a pseudoref?
>  - Does a pseudoref with his string as its fullref exist?
>
> independently.  The first one would answer Yes and the second one
> would answer No in such a context.
>

This does work into the flags based mechanism that Patrick mentioned
too. The users of this new function can then only request information as
necessary.

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +	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);
>> +	}
>
> FYI. I see
>
> .git/rebase-apply/patch:31: space before tab in indent.
>    					RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> .git/rebase-apply/patch:32: space before tab in indent.
>    					&oid, NULL);
> .git/rebase-apply/patch:33: space before tab in indent.
>    		return !is_null_oid(&oid);
>
> around here.

Thanks for notifying, Jeff mentioned the same and I thought I fixed it.
I'll have a look at why my editor did this and add some steps to avoid
this in the following patches.

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

Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I wonder whether we can maybe consolidate the interface into one or
> maybe even two functions where the behaviour can be tweaked with a flag
> field. Something like `refname_is_valid()` with a bunch of flags:
>
>   - REFNAME_ACCEPT_HEAD to accept "HEAD"
>   - REFNAME_ACCEPT_PSEUDOREF to accept all of the refs ending with
>     "_HEAD" or being one of the irregular pseudorefs.
>   - REFNAME_ACCEPT_INVALID_BUT_SAFE to accept refnames which aren't
>     valid, but which would pass `refname_is_safe()`.

I am certain we _can_, but it will take an actual patch to see if
such a refactoring makes the callers easier to follow, which is the
real test.  FWIW, I am much less skeptical than hopeful in this
particular case.

Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

[Karthik Nayak]

Hello,

On Tue, Feb 13, 2024 at 8:42 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > I wonder whether we can maybe consolidate the interface into one or
> > maybe even two functions where the behaviour can be tweaked with a flag
> > field. Something like `refname_is_valid()` with a bunch of flags:
> >
> >   - REFNAME_ACCEPT_HEAD to accept "HEAD"
> >   - REFNAME_ACCEPT_PSEUDOREF to accept all of the refs ending with
> >     "_HEAD" or being one of the irregular pseudorefs.
> >   - REFNAME_ACCEPT_INVALID_BUT_SAFE to accept refnames which aren't
> >     valid, but which would pass `refname_is_safe()`.
>
> I am certain we _can_, but it will take an actual patch to see if
> such a refactoring makes the callers easier to follow, which is the
> real test.  FWIW, I am much less skeptical than hopeful in this
> particular case.

I was trying to implement this and realized that the changes sprawl
multiple files and
and have a fair bit of complexity since `check_refname_format()`
implements its own
flags. Overall, adding it to this patch series would overshadow what
we're trying to do here.

I think it would be best to tackle this problem after this series has landed.

Junio, let me know if you want me to reroll for the whitespace issues.
Otherwise, I'll wait
for reviews here.

Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Junio, let me know if you want me to reroll for the whitespace issues.

I think I applied with "am --whitespace=fix", so we should be OK,
but if you can double check the result that would be appreciated.

Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

[Karthik Nayak]

On Wed, Feb 14, 2024 at 5:59 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > Junio, let me know if you want me to reroll for the whitespace issues.
>
> I think I applied with "am --whitespace=fix", so we should be OK,
> but if you can double check the result that would be appreciated.

I did go through the patches and didn't find any other fixes needed. Thanks

Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> +	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);
> +	}

FYI. I see

.git/rebase-apply/patch:31: space before tab in indent.
   					RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
.git/rebase-apply/patch:32: space before tab in indent.
   					&oid, NULL);
.git/rebase-apply/patch:33: space before tab in indent.
   		return !is_null_oid(&oid);

around here.

[PATCH v4 2/5] refs: extract out `loose_fill_ref_dir_regular_file()`

[Karthik Nayak]

Extract out the code for adding a single file to the loose ref dir as
`loose_fill_ref_dir_regular_file()` from `loose_fill_ref_dir()` in
`refs/files-backend.c`.

This allows us to use this function independently in the following
commits where we add code to also add pseudorefs to the ref dir.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c | 62 +++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 75dcc21ecb..65128821a8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -229,6 +229,38 @@ static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dir
 	}
 }
 
+static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
+					    const char *refname,
+					    struct ref_dir *dir)
+{
+	struct object_id oid;
+	int flag;
+
+	if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_READING,
+				     &oid, &flag)) {
+		oidclr(&oid);
+		flag |= REF_ISBROKEN;
+	} else if (is_null_oid(&oid)) {
+		/*
+		 * It is so astronomically unlikely
+		 * that null_oid is the OID of an
+		 * actual object that we consider its
+		 * appearance in a loose reference
+		 * file to be repo corruption
+		 * (probably due to a software bug).
+		 */
+		flag |= REF_ISBROKEN;
+	}
+
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		if (!refname_is_safe(refname))
+			die("loose refname is dangerous: %s", refname);
+		oidclr(&oid);
+		flag |= REF_BAD_NAME | REF_ISBROKEN;
+	}
+	add_entry_to_dir(dir, create_ref_entry(refname, &oid, flag));
+}
+
 /*
  * Read the loose references from the namespace dirname into dir
  * (without recursing).  dirname must end with '/'.  dir must be the
@@ -257,8 +289,6 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 	strbuf_add(&refname, dirname, dirnamelen);
 
 	while ((de = readdir(d)) != NULL) {
-		struct object_id oid;
-		int flag;
 		unsigned char dtype;
 
 		if (de->d_name[0] == '.')
@@ -274,33 +304,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 					 create_dir_entry(dir->cache, refname.buf,
 							  refname.len));
 		} else if (dtype == DT_REG) {
-			if (!refs_resolve_ref_unsafe(&refs->base,
-						     refname.buf,
-						     RESOLVE_REF_READING,
-						     &oid, &flag)) {
-				oidclr(&oid);
-				flag |= REF_ISBROKEN;
-			} else if (is_null_oid(&oid)) {
-				/*
-				 * It is so astronomically unlikely
-				 * that null_oid is the OID of an
-				 * actual object that we consider its
-				 * appearance in a loose reference
-				 * file to be repo corruption
-				 * (probably due to a software bug).
-				 */
-				flag |= REF_ISBROKEN;
-			}
-
-			if (check_refname_format(refname.buf,
-						 REFNAME_ALLOW_ONELEVEL)) {
-				if (!refname_is_safe(refname.buf))
-					die("loose refname is dangerous: %s", refname.buf);
-				oidclr(&oid);
-				flag |= REF_BAD_NAME | REF_ISBROKEN;
-			}
-			add_entry_to_dir(dir,
-					 create_ref_entry(refname.buf, &oid, flag));
+			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
 		}
 		strbuf_setlen(&refname, dirnamelen);
 	}
-- 
2.43.GIT

[PATCH v4 3/5] refs: introduce `refs_for_each_include_root_refs()`

[Karthik Nayak]

Introduce a new ref iteration flag `DO_FOR_EACH_INCLUDE_ROOT_REFS`,
which will be used to iterate over regular refs plus pseudorefs and
HEAD.

Refs which fall outside the `refs/` and aren't either pseudorefs or HEAD
are more of a grey area. This is because we don't block the users from
creating such refs but they are not officially supported.

Introduce `refs_for_each_include_root_refs()` which calls
`do_for_each_ref()` with this newly introduced flag.

In `refs/files-backend.c`, introduce a new function
`add_pseudoref_and_head_entries()` to add pseudorefs and HEAD to the
`ref_dir`. We then finally call `add_pseudoref_and_head_entries()`
whenever the `DO_FOR_EACH_INCLUDE_ROOT_REFS` flag is set. Any new ref
backend will also have to implement similar changes on its end.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c               |  7 +++++
 refs.h               |  6 ++++
 refs/files-backend.c | 65 ++++++++++++++++++++++++++++++++++++++++----
 refs/refs-internal.h |  6 ++++
 4 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index d8e4cf9a11..77f4c1e4c2 100644
--- a/refs.c
+++ b/refs.c
@@ -1765,6 +1765,13 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data);
 }
 
+int refs_for_each_include_root_refs(struct ref_store *refs, each_ref_fn fn,
+				    void *cb_data)
+{
+	return do_for_each_ref(refs, "", NULL, fn, 0,
+			       DO_FOR_EACH_INCLUDE_ROOT_REFS, cb_data);
+}
+
 static int qsort_strcmp(const void *va, const void *vb)
 {
 	const char *a = *(const char **)va;
diff --git a/refs.h b/refs.h
index f66cdd731c..5cfaee6229 100644
--- a/refs.h
+++ b/refs.h
@@ -398,6 +398,12 @@ int for_each_namespaced_ref(const char **exclude_patterns,
 int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
 int for_each_rawref(each_ref_fn fn, void *cb_data);
 
+/*
+ * Iterates over all refs including root refs, i.e. pseudorefs and HEAD.
+ */
+int refs_for_each_include_root_refs(struct ref_store *refs, each_ref_fn fn,
+				    void *cb_data);
+
 /*
  * Normalizes partial refs to their fully qualified form.
  * Will prepend <prefix> to the <pattern> if it doesn't start with 'refs/'.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 65128821a8..9c1c42fe52 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -315,9 +315,59 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 	add_per_worktree_entries_to_dir(dir, dirname);
 }
 
-static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
+/*
+ * Add pseudorefs to the ref dir by parsing the directory for any files
+ * which follow the pseudoref syntax.
+ */
+static void add_pseudoref_and_head_entries(struct ref_store *ref_store,
+					 struct ref_dir *dir,
+					 const char *dirname)
+{
+	struct files_ref_store *refs =
+		files_downcast(ref_store, REF_STORE_READ, "fill_ref_dir");
+	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
+	struct dirent *de;
+	size_t dirnamelen;
+	DIR *d;
+
+	files_ref_path(refs, &path, dirname);
+
+	d = opendir(path.buf);
+	if (!d) {
+		strbuf_release(&path);
+		return;
+	}
+
+	strbuf_addstr(&refname, dirname);
+	dirnamelen = refname.len;
+
+	while ((de = readdir(d)) != NULL) {
+		unsigned char dtype;
+
+		if (de->d_name[0] == '.')
+			continue;
+		if (ends_with(de->d_name, ".lock"))
+			continue;
+		strbuf_addstr(&refname, de->d_name);
+
+		dtype = get_dtype(de, &path, 1);
+		if (dtype == DT_REG && (is_pseudoref(ref_store, de->d_name) ||
+								is_headref(ref_store, de->d_name)))
+			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
+
+		strbuf_setlen(&refname, dirnamelen);
+	}
+	strbuf_release(&refname);
+	strbuf_release(&path);
+	closedir(d);
+}
+
+static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
+					     unsigned int flags)
 {
 	if (!refs->loose) {
+		struct ref_dir *dir;
+
 		/*
 		 * Mark the top-level directory complete because we
 		 * are about to read the only subdirectory that can
@@ -328,12 +378,17 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
 		/* We're going to fill the top level ourselves: */
 		refs->loose->root->flag &= ~REF_INCOMPLETE;
 
+		dir = get_ref_dir(refs->loose->root);
+
+		if (flags & DO_FOR_EACH_INCLUDE_ROOT_REFS)
+			add_pseudoref_and_head_entries(dir->cache->ref_store, dir,
+						       refs->loose->root->name);
+
 		/*
 		 * Add an incomplete entry for "refs/" (to be filled
 		 * lazily):
 		 */
-		add_entry_to_dir(get_ref_dir(refs->loose->root),
-				 create_dir_entry(refs->loose, "refs/", 5));
+		add_entry_to_dir(dir, create_dir_entry(refs->loose, "refs/", 5));
 	}
 	return refs->loose;
 }
@@ -861,7 +916,7 @@ static struct ref_iterator *files_ref_iterator_begin(
 	 * disk, and re-reads it if not.
 	 */
 
-	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
+	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
 					      prefix, ref_store->repo, 1);
 
 	/*
@@ -1222,7 +1277,7 @@ static int files_pack_refs(struct ref_store *ref_store,
 
 	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
 
-	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
+	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL,
 					the_repository, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
 		/*
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 83e0f0bba3..73a8fa18ad 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -260,6 +260,12 @@ enum do_for_each_ref_flags {
 	 * INCLUDE_BROKEN, since they are otherwise not included at all.
 	 */
 	DO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2),
+
+	/*
+	 * Include root refs i.e. HEAD and pseudorefs along with the regular
+	 * refs.
+	 */
+	DO_FOR_EACH_INCLUDE_ROOT_REFS = (1 << 3),
 };
 
 /*
-- 
2.43.GIT

[PATCH v4 4/5] ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'

[Karthik Nayak]

The flag 'FILTER_REFS_ALL' is a bit ambiguous, where ALL doesn't specify
if it means to contain refs from all worktrees or whether all types of
refs (regular, HEAD & pseudorefs) or all of the above.

Since here it is actually referring to all refs with the "refs/" prefix,
let's rename it to 'FILTER_REFS_REGULAR' to indicate that this is
specifically for regular refs.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/for-each-ref.c | 2 +-
 ref-filter.c           | 2 +-
 ref-filter.h           | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3885a9c28e..23d352e371 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -97,7 +97,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	}
 
 	filter.match_as_path = 1;
-	filter_and_format_refs(&filter, FILTER_REFS_ALL, sorting, &format);
+	filter_and_format_refs(&filter, FILTER_REFS_REGULAR, sorting, &format);
 
 	ref_filter_clear(&filter);
 	ref_sorting_release(sorting);
diff --git a/ref-filter.c b/ref-filter.c
index be14b56e32..acb960e35c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -3047,7 +3047,7 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
 			ret = for_each_fullref_in("refs/remotes/", fn, cb_data);
 		else if (filter->kind == FILTER_REFS_TAGS)
 			ret = for_each_fullref_in("refs/tags/", fn, cb_data);
-		else if (filter->kind & FILTER_REFS_ALL)
+		else if (filter->kind & FILTER_REFS_REGULAR)
 			ret = for_each_fullref_in_pattern(filter, fn, cb_data);
 		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
 			head_ref(fn, cb_data);
diff --git a/ref-filter.h b/ref-filter.h
index 07cd6f6da3..5416936800 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -19,10 +19,10 @@
 #define FILTER_REFS_BRANCHES       0x0004
 #define FILTER_REFS_REMOTES        0x0008
 #define FILTER_REFS_OTHERS         0x0010
-#define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
+#define FILTER_REFS_REGULAR        (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
 				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
 #define FILTER_REFS_DETACHED_HEAD  0x0020
-#define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
+#define FILTER_REFS_KIND_MASK      (FILTER_REFS_REGULAR | FILTER_REFS_DETACHED_HEAD)
 
 struct atom_value;
 struct ref_sorting;
-- 
2.43.GIT

[PATCH v4 5/5] for-each-ref: add new option to include root refs

[Karthik Nayak]

The git-for-each-ref(1) command doesn't provide a way to print root refs
i.e pseudorefs and HEAD with the regular "refs/" prefixed refs.

This commit adds a new option "--include-root-refs" to
git-for-each-ref(1). When used this would also print pseudorefs and HEAD
for the current worktree.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  5 ++++-
 builtin/for-each-ref.c             | 11 ++++++++---
 ref-filter.c                       | 27 +++++++++++++++++++++++++-
 ref-filter.h                       |  5 ++++-
 refs/reftable-backend.c            | 11 +++++++----
 t/t6302-for-each-ref-filter.sh     | 31 ++++++++++++++++++++++++++++++
 6 files changed, 80 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 3a9ad91b7a..c1dd12b93c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
 		   [(--sort=<key>)...] [--format=<format>]
-		   [ --stdin | <pattern>... ]
+		   [--include-root-refs] [ --stdin | <pattern>... ]
 		   [--points-at=<object>]
 		   [--merged[=<object>]] [--no-merged[=<object>]]
 		   [--contains[=<object>]] [--no-contains[=<object>]]
@@ -105,6 +105,9 @@ TAB %(refname)`.
 	any excluded pattern(s) are shown. Matching is done using the
 	same rules as `<pattern>` above.
 
+--include-root-refs::
+	List root refs (HEAD and pseudorefs) apart from regular refs.
+
 FIELD NAMES
 -----------
 
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 23d352e371..9ed146dad3 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -20,10 +20,10 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
 	struct ref_sorting *sorting;
 	struct string_list sorting_options = STRING_LIST_INIT_DUP;
-	int icase = 0;
+	int icase = 0, include_root_refs = 0, from_stdin = 0;
 	struct ref_filter filter = REF_FILTER_INIT;
 	struct ref_format format = REF_FORMAT_INIT;
-	int from_stdin = 0;
+	unsigned int flags = FILTER_REFS_REGULAR;
 	struct strvec vec = STRVEC_INIT;
 
 	struct option opts[] = {
@@ -53,6 +53,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_NO_CONTAINS(&filter.no_commit, N_("print only refs which don't contain the commit")),
 		OPT_BOOL(0, "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
 		OPT_BOOL(0, "stdin", &from_stdin, N_("read reference patterns from stdin")),
+		OPT_BOOL(0, "include-root-refs", &include_root_refs, N_("also include HEAD ref and pseudorefs")),
 		OPT_END(),
 	};
 
@@ -96,8 +97,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		filter.name_patterns = argv;
 	}
 
+	if (include_root_refs) {
+		flags |= FILTER_REFS_ROOT_REFS;
+	}
+
 	filter.match_as_path = 1;
-	filter_and_format_refs(&filter, FILTER_REFS_REGULAR, sorting, &format);
+	filter_and_format_refs(&filter, flags, sorting, &format);
 
 	ref_filter_clear(&filter);
 	ref_sorting_release(sorting);
diff --git a/ref-filter.c b/ref-filter.c
index acb960e35c..0e83e29390 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2628,6 +2628,12 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
 				       each_ref_fn cb,
 				       void *cb_data)
 {
+	if (filter->kind == FILTER_REFS_KIND_MASK) {
+		/* 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);
+	}
+
 	if (!filter->match_as_path) {
 		/*
 		 * in this case, the patterns are applied after
@@ -2750,6 +2756,9 @@ static int ref_kind_from_refname(const char *refname)
 			return ref_kind[i].kind;
 	}
 
+	if (is_pseudoref(get_main_ref_store(the_repository), refname))
+		return FILTER_REFS_PSEUDOREFS;
+
 	return FILTER_REFS_OTHERS;
 }
 
@@ -2781,6 +2790,16 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
 
 	/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
 	kind = filter_ref_kind(filter, refname);
+
+	/*
+	 * 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.
+	 */
+	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;
 
@@ -3049,7 +3068,13 @@ 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)
 			ret = for_each_fullref_in_pattern(filter, fn, cb_data);
-		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
+
+		/*
+		 * When printing all ref types, HEAD is already included,
+		 * so we don't want to print HEAD again.
+		 */
+		if (!ret && (filter->kind != FILTER_REFS_KIND_MASK) &&
+		    (filter->kind & FILTER_REFS_DETACHED_HEAD))
 			head_ref(fn, cb_data);
 	}
 
diff --git a/ref-filter.h b/ref-filter.h
index 5416936800..0ca28d2bba 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -22,7 +22,10 @@
 #define FILTER_REFS_REGULAR        (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
 				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
 #define FILTER_REFS_DETACHED_HEAD  0x0020
-#define FILTER_REFS_KIND_MASK      (FILTER_REFS_REGULAR | FILTER_REFS_DETACHED_HEAD)
+#define FILTER_REFS_PSEUDOREFS     0x0040
+#define FILTER_REFS_ROOT_REFS      (FILTER_REFS_DETACHED_HEAD | FILTER_REFS_PSEUDOREFS)
+#define FILTER_REFS_KIND_MASK      (FILTER_REFS_REGULAR | FILTER_REFS_DETACHED_HEAD | \
+				    FILTER_REFS_PSEUDOREFS)
 
 struct atom_value;
 struct ref_sorting;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index a14f2ad7f4..c23a516ac2 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -364,12 +364,15 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
 			break;
 
 		/*
-		 * The files backend only lists references contained in
-		 * "refs/". We emulate the same behaviour here and thus skip
-		 * all references that don't start with this prefix.
+		 * The files backend only lists references contained in "refs/" unless
+		 * the root refs are to be included. We emulate the same behaviour here.
 		 */
-		if (!starts_with(iter->ref.refname, "refs/"))
+		if (!starts_with(iter->ref.refname, "refs/") &&
+		    !(iter->flags & DO_FOR_EACH_INCLUDE_ROOT_REFS &&
+		     (is_pseudoref(&iter->refs->base, iter->ref.refname) ||
+		      is_headref(&iter->refs->base, iter->ref.refname)))) {
 			continue;
+		}
 
 		if (iter->prefix &&
 		    strncmp(iter->prefix, iter->ref.refname, strlen(iter->prefix))) {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 82f3d1ea0f..948f1bb5f4 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -31,6 +31,37 @@ test_expect_success 'setup some history and refs' '
 	git update-ref refs/odd/spot main
 '
 
+test_expect_success '--include-root-refs pattern prints pseudorefs' '
+	cat >expect <<-\EOF &&
+	HEAD
+	ORIG_HEAD
+	refs/heads/main
+	refs/heads/side
+	refs/odd/spot
+	refs/tags/annotated-tag
+	refs/tags/doubly-annotated-tag
+	refs/tags/doubly-signed-tag
+	refs/tags/four
+	refs/tags/one
+	refs/tags/signed-tag
+	refs/tags/three
+	refs/tags/two
+	EOF
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" --include-root-refs >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--include-root-refs with other patterns' '
+	cat >expect <<-\EOF &&
+	HEAD
+	ORIG_HEAD
+	EOF
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" --include-root-refs "*HEAD" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'filtering with --points-at' '
 	cat >expect <<-\EOF &&
 	refs/heads/main
-- 
2.43.GIT

Re: [PATCH v4 5/5] for-each-ref: add new option to include root refs

[Patrick Steinhardt]

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

On Sun, Feb 11, 2024 at 07:39:23PM +0100, Karthik Nayak wrote:
> The git-for-each-ref(1) command doesn't provide a way to print root refs
> i.e pseudorefs and HEAD with the regular "refs/" prefixed refs.
> 
> This commit adds a new option "--include-root-refs" to
> git-for-each-ref(1). When used this would also print pseudorefs and HEAD
> for the current worktree.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt |  5 ++++-
>  builtin/for-each-ref.c             | 11 ++++++++---
>  ref-filter.c                       | 27 +++++++++++++++++++++++++-
>  ref-filter.h                       |  5 ++++-
>  refs/reftable-backend.c            | 11 +++++++----
>  t/t6302-for-each-ref-filter.sh     | 31 ++++++++++++++++++++++++++++++
>  6 files changed, 80 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 3a9ad91b7a..c1dd12b93c 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -10,7 +10,7 @@ SYNOPSIS
>  [verse]
>  'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
>  		   [(--sort=<key>)...] [--format=<format>]
> -		   [ --stdin | <pattern>... ]
> +		   [--include-root-refs] [ --stdin | <pattern>... ]
>  		   [--points-at=<object>]
>  		   [--merged[=<object>]] [--no-merged[=<object>]]
>  		   [--contains[=<object>]] [--no-contains[=<object>]]
> @@ -105,6 +105,9 @@ TAB %(refname)`.
>  	any excluded pattern(s) are shown. Matching is done using the
>  	same rules as `<pattern>` above.
>  
> +--include-root-refs::
> +	List root refs (HEAD and pseudorefs) apart from regular refs.
> +
>  FIELD NAMES
>  -----------
>  
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 23d352e371..9ed146dad3 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -20,10 +20,10 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  {
>  	struct ref_sorting *sorting;
>  	struct string_list sorting_options = STRING_LIST_INIT_DUP;
> -	int icase = 0;
> +	int icase = 0, include_root_refs = 0, from_stdin = 0;
>  	struct ref_filter filter = REF_FILTER_INIT;
>  	struct ref_format format = REF_FORMAT_INIT;
> -	int from_stdin = 0;
> +	unsigned int flags = FILTER_REFS_REGULAR;
>  	struct strvec vec = STRVEC_INIT;
>  
>  	struct option opts[] = {
> @@ -53,6 +53,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  		OPT_NO_CONTAINS(&filter.no_commit, N_("print only refs which don't contain the commit")),
>  		OPT_BOOL(0, "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
>  		OPT_BOOL(0, "stdin", &from_stdin, N_("read reference patterns from stdin")),
> +		OPT_BOOL(0, "include-root-refs", &include_root_refs, N_("also include HEAD ref and pseudorefs")),
>  		OPT_END(),
>  	};
>  
> @@ -96,8 +97,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  		filter.name_patterns = argv;
>  	}
>  
> +	if (include_root_refs) {
> +		flags |= FILTER_REFS_ROOT_REFS;
> +	}

Nit: we don't use braces for single-line blocks.

> +
>  	filter.match_as_path = 1;
> -	filter_and_format_refs(&filter, FILTER_REFS_REGULAR, sorting, &format);
> +	filter_and_format_refs(&filter, flags, sorting, &format);
>  
>  	ref_filter_clear(&filter);
>  	ref_sorting_release(sorting);
> diff --git a/ref-filter.c b/ref-filter.c
> index acb960e35c..0e83e29390 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2628,6 +2628,12 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
>  				       each_ref_fn cb,
>  				       void *cb_data)
>  {
> +	if (filter->kind == FILTER_REFS_KIND_MASK) {
> +		/* in this case, we want to print all refs including root refs. */

Nit: s/in/In, as this is a full sentence.

> +		return refs_for_each_include_root_refs(get_main_ref_store(the_repository),
> +						       cb, cb_data);
> +	}
> +
>  	if (!filter->match_as_path) {
>  		/*
>  		 * in this case, the patterns are applied after
> @@ -2750,6 +2756,9 @@ static int ref_kind_from_refname(const char *refname)
>  			return ref_kind[i].kind;
>  	}
>  
> +	if (is_pseudoref(get_main_ref_store(the_repository), refname))
> +		return FILTER_REFS_PSEUDOREFS;
> +
>  	return FILTER_REFS_OTHERS;
>  }
>  
> @@ -2781,6 +2790,16 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
>  
>  	/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
>  	kind = filter_ref_kind(filter, refname);
> +
> +	/*
> +	 * 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.
> +	 */
> +	if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD)
> +		kind = FILTER_REFS_PSEUDOREFS;

I'm not sure why exactly we need to munge the kind here. Would be great
if the comment explained what the actual difference would be.

> +	else if (!(kind & filter->kind))
> +		return NULL;
> +
>  	if (!(kind & filter->kind))
>  		return NULL;

This condition here is duplicated now, isn't it?

Patrick

> @@ -3049,7 +3068,13 @@ 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)
>  			ret = for_each_fullref_in_pattern(filter, fn, cb_data);
> -		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
> +
> +		/*
> +		 * When printing all ref types, HEAD is already included,
> +		 * so we don't want to print HEAD again.
> +		 */
> +		if (!ret && (filter->kind != FILTER_REFS_KIND_MASK) &&
> +		    (filter->kind & FILTER_REFS_DETACHED_HEAD))
>  			head_ref(fn, cb_data);
>  	}
>  
> diff --git a/ref-filter.h b/ref-filter.h
> index 5416936800..0ca28d2bba 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -22,7 +22,10 @@
>  #define FILTER_REFS_REGULAR        (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
>  				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
>  #define FILTER_REFS_DETACHED_HEAD  0x0020
> -#define FILTER_REFS_KIND_MASK      (FILTER_REFS_REGULAR | FILTER_REFS_DETACHED_HEAD)
> +#define FILTER_REFS_PSEUDOREFS     0x0040
> +#define FILTER_REFS_ROOT_REFS      (FILTER_REFS_DETACHED_HEAD | FILTER_REFS_PSEUDOREFS)
> +#define FILTER_REFS_KIND_MASK      (FILTER_REFS_REGULAR | FILTER_REFS_DETACHED_HEAD | \
> +				    FILTER_REFS_PSEUDOREFS)
>  
>  struct atom_value;
>  struct ref_sorting;
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index a14f2ad7f4..c23a516ac2 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -364,12 +364,15 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
>  			break;
>  
>  		/*
> -		 * The files backend only lists references contained in
> -		 * "refs/". We emulate the same behaviour here and thus skip
> -		 * all references that don't start with this prefix.
> +		 * The files backend only lists references contained in "refs/" unless
> +		 * the root refs are to be included. We emulate the same behaviour here.
>  		 */
> -		if (!starts_with(iter->ref.refname, "refs/"))
> +		if (!starts_with(iter->ref.refname, "refs/") &&
> +		    !(iter->flags & DO_FOR_EACH_INCLUDE_ROOT_REFS &&
> +		     (is_pseudoref(&iter->refs->base, iter->ref.refname) ||
> +		      is_headref(&iter->refs->base, iter->ref.refname)))) {
>  			continue;
> +		}
>  
>  		if (iter->prefix &&
>  		    strncmp(iter->prefix, iter->ref.refname, strlen(iter->prefix))) {
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 82f3d1ea0f..948f1bb5f4 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -31,6 +31,37 @@ test_expect_success 'setup some history and refs' '
>  	git update-ref refs/odd/spot main
>  '
>  
> +test_expect_success '--include-root-refs pattern prints pseudorefs' '
> +	cat >expect <<-\EOF &&
> +	HEAD
> +	ORIG_HEAD
> +	refs/heads/main
> +	refs/heads/side
> +	refs/odd/spot
> +	refs/tags/annotated-tag
> +	refs/tags/doubly-annotated-tag
> +	refs/tags/doubly-signed-tag
> +	refs/tags/four
> +	refs/tags/one
> +	refs/tags/signed-tag
> +	refs/tags/three
> +	refs/tags/two
> +	EOF
> +	git update-ref ORIG_HEAD main &&
> +	git for-each-ref --format="%(refname)" --include-root-refs >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '--include-root-refs with other patterns' '
> +	cat >expect <<-\EOF &&
> +	HEAD
> +	ORIG_HEAD
> +	EOF
> +	git update-ref ORIG_HEAD main &&
> +	git for-each-ref --format="%(refname)" --include-root-refs "*HEAD" >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'filtering with --points-at' '
>  	cat >expect <<-\EOF &&
>  	refs/heads/main
> -- 
> 2.43.GIT
> 

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

Re: [PATCH v4 5/5] for-each-ref: add new option to include root refs

[Karthik Nayak]

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

Patrick Steinhardt <ps@pks.im> writes:
>> @@ -96,8 +97,12 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>>  		filter.name_patterns = argv;
>>  	}
>>
>> +	if (include_root_refs) {
>> +		flags |= FILTER_REFS_ROOT_REFS;
>> +	}
>
> Nit: we don't use braces for single-line blocks.
>

Right, thanks, I always trip on this.

>> diff --git a/ref-filter.c b/ref-filter.c
>> index acb960e35c..0e83e29390 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -2628,6 +2628,12 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
>>  				       each_ref_fn cb,
>>  				       void *cb_data)
>>  {
>> +	if (filter->kind == FILTER_REFS_KIND_MASK) {
>> +		/* in this case, we want to print all refs including root refs. */
>
> Nit: s/in/In, as this is a full sentence.
>

Will change


>> @@ -2781,6 +2790,16 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
>>
>>  	/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
>>  	kind = filter_ref_kind(filter, refname);
>> +
>> +	/*
>> +	 * 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.
>> +	 */
>> +	if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD)
>> +		kind = FILTER_REFS_PSEUDOREFS;
>
> I'm not sure why exactly we need to munge the kind here. Would be great
> if the comment explained what the actual difference would be.
>

So the difference is

$ git for-each-ref --include-root-refs
9eda75497d43f2f9c70c1e14afb865108f9b4b49 commit	FETCH_HEAD
ee99ac41aeb4129866710fc5771f11e1c1742dee commit	HEAD
96c8a0712e569dd2812bf4fb5e72113caf326500 commit	ORIG_HEAD

vs

$ git for-each-ref --include-root-refs | grep -v refs/
b4b94355057280749620c47999a4b45dc60f2681 commit	(HEAD detached at b4b9435505)
9eda75497d43f2f9c70c1e14afb865108f9b4b49 commit	FETCH_HEAD
ee99ac41aeb4129866710fc5771f11e1c1742dee commit	ORIG_HEAD

This is because in `get_refname` we provide head description. But in
git-for-each-ref we don't want this, we want the format to be consistent.

>> +	else if (!(kind & filter->kind))
>> +		return NULL;
>> +
>>  	if (!(kind & filter->kind))
>>  		return NULL;
>
> This condition here is duplicated now, isn't it?
>
> Patrick
>

Seems like a bad merge on my end. Will fix. Thanks for the review. Will
send a new version soon.

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

Re: [PATCH v4 5/5] for-each-ref: add new option to include root refs

[Patrick Steinhardt]

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

On Thu, Feb 22, 2024 at 04:57:04AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> >> @@ -2781,6 +2790,16 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
> >>
> >>  	/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
> >>  	kind = filter_ref_kind(filter, refname);
> >> +
> >> +	/*
> >> +	 * 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.
> >> +	 */
> >> +	if (filter->kind == FILTER_REFS_KIND_MASK && kind == FILTER_REFS_DETACHED_HEAD)
> >> +		kind = FILTER_REFS_PSEUDOREFS;
> >
> > I'm not sure why exactly we need to munge the kind here. Would be great
> > if the comment explained what the actual difference would be.
> >
> 
> So the difference is
> 
> $ git for-each-ref --include-root-refs
> 9eda75497d43f2f9c70c1e14afb865108f9b4b49 commit	FETCH_HEAD
> ee99ac41aeb4129866710fc5771f11e1c1742dee commit	HEAD
> 96c8a0712e569dd2812bf4fb5e72113caf326500 commit	ORIG_HEAD
> 
> vs
> 
> $ git for-each-ref --include-root-refs | grep -v refs/
> b4b94355057280749620c47999a4b45dc60f2681 commit	(HEAD detached at b4b9435505)
> 9eda75497d43f2f9c70c1e14afb865108f9b4b49 commit	FETCH_HEAD
> ee99ac41aeb4129866710fc5771f11e1c1742dee commit	ORIG_HEAD
> 
> This is because in `get_refname` we provide head description. But in
> git-for-each-ref we don't want this, we want the format to be consistent.

Ah, makes sense, thanks for the explanation! Briefly mentioning this in
the comment should help readers to understand this a lot faster.

Patrick

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

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

[Karthik Nayak]

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:

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

[PATCH v5 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

[Karthik Nayak]

Introduce two new functions `is_pseudoref()` and `is_headref()`. This
provides the necessary functionality for us to add pseudorefs and HEAD
to the loose ref cache in the files backend, allowing us to build
tooling to print these refs.

The `is_pseudoref()` function internally calls `is_pseudoref_syntax()`
but adds onto it by also checking to ensure that the pseudoref either
ends with a "_HEAD" suffix or matches a list of exceptions. After which
we also parse the contents of the pseudoref to ensure that it conforms
to the ref format.

We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
because the function is also used by `is_current_worktree_ref()` and
making it stricter to match only known pseudorefs might have unintended
consequences due to files like 'BISECT_START' which isn't a pseudoref
but sometimes contains object ID.

Keeping this in mind, we leave `is_pseudoref_syntax()` as is and create
`is_pseudoref()` which is stricter. Ideally we'd want to move the new
syntax checks to `is_pseudoref_syntax()` but a prerequisite for this
would be to actually remove the exception list by converting those
pseudorefs to also contain a '_HEAD' suffix and perhaps move bisect
related files like 'BISECT_START' to a new directory similar to the
'rebase-merge' directory.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c | 41 +++++++++++++++++++++++++++++++++++++++++
 refs.h |  3 +++
 2 files changed, 44 insertions(+)

diff --git a/refs.c b/refs.c
index fff343c256..3546d90831 100644
--- a/refs.c
+++ b/refs.c
@@ -860,6 +860,47 @@ static int is_pseudoref_syntax(const char *refname)
 	return 1;
 }
 
+int is_pseudoref(struct ref_store *refs, const char *refname)
+{
+	static const char *const irregular_pseudorefs[] = {
+		"AUTO_MERGE",
+		"BISECT_EXPECTED_REV",
+		"NOTES_MERGE_PARTIAL",
+		"NOTES_MERGE_REF",
+		"MERGE_AUTOSTASH",
+	};
+	struct object_id oid;
+	size_t i;
+
+	if (!is_pseudoref_syntax(refname))
+		return 0;
+
+	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);
+	}
+
+	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);
+			return !is_null_oid(&oid);
+		}
+
+	return 0;
+}
+
+int is_headref(struct ref_store *refs, const char *refname)
+{
+	if (!strcmp(refname, "HEAD"))
+		return refs_ref_exists(refs, refname);
+
+	return 0;
+}
+
 static int is_current_worktree_ref(const char *ref) {
 	return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
 }
diff --git a/refs.h b/refs.h
index 303c5fac4d..f66cdd731c 100644
--- a/refs.h
+++ b/refs.h
@@ -1023,4 +1023,7 @@ extern struct ref_namespace_info ref_namespace[NAMESPACE__COUNT];
  */
 void update_ref_namespace(enum ref_namespace namespace, char *ref);
 
+int is_pseudoref(struct ref_store *refs, const char *refname);
+int is_headref(struct ref_store *refs, const char *refname);
+
 #endif /* REFS_H */
-- 
2.43.GIT

[PATCH v5 2/5] refs: extract out `loose_fill_ref_dir_regular_file()`

[Karthik Nayak]

Extract out the code for adding a single file to the loose ref dir as
`loose_fill_ref_dir_regular_file()` from `loose_fill_ref_dir()` in
`refs/files-backend.c`.

This allows us to use this function independently in the following
commits where we add code to also add pseudorefs to the ref dir.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c | 62 +++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 75dcc21ecb..65128821a8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -229,6 +229,38 @@ static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dir
 	}
 }
 
+static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
+					    const char *refname,
+					    struct ref_dir *dir)
+{
+	struct object_id oid;
+	int flag;
+
+	if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_READING,
+				     &oid, &flag)) {
+		oidclr(&oid);
+		flag |= REF_ISBROKEN;
+	} else if (is_null_oid(&oid)) {
+		/*
+		 * It is so astronomically unlikely
+		 * that null_oid is the OID of an
+		 * actual object that we consider its
+		 * appearance in a loose reference
+		 * file to be repo corruption
+		 * (probably due to a software bug).
+		 */
+		flag |= REF_ISBROKEN;
+	}
+
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		if (!refname_is_safe(refname))
+			die("loose refname is dangerous: %s", refname);
+		oidclr(&oid);
+		flag |= REF_BAD_NAME | REF_ISBROKEN;
+	}
+	add_entry_to_dir(dir, create_ref_entry(refname, &oid, flag));
+}
+
 /*
  * Read the loose references from the namespace dirname into dir
  * (without recursing).  dirname must end with '/'.  dir must be the
@@ -257,8 +289,6 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 	strbuf_add(&refname, dirname, dirnamelen);
 
 	while ((de = readdir(d)) != NULL) {
-		struct object_id oid;
-		int flag;
 		unsigned char dtype;
 
 		if (de->d_name[0] == '.')
@@ -274,33 +304,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 					 create_dir_entry(dir->cache, refname.buf,
 							  refname.len));
 		} else if (dtype == DT_REG) {
-			if (!refs_resolve_ref_unsafe(&refs->base,
-						     refname.buf,
-						     RESOLVE_REF_READING,
-						     &oid, &flag)) {
-				oidclr(&oid);
-				flag |= REF_ISBROKEN;
-			} else if (is_null_oid(&oid)) {
-				/*
-				 * It is so astronomically unlikely
-				 * that null_oid is the OID of an
-				 * actual object that we consider its
-				 * appearance in a loose reference
-				 * file to be repo corruption
-				 * (probably due to a software bug).
-				 */
-				flag |= REF_ISBROKEN;
-			}
-
-			if (check_refname_format(refname.buf,
-						 REFNAME_ALLOW_ONELEVEL)) {
-				if (!refname_is_safe(refname.buf))
-					die("loose refname is dangerous: %s", refname.buf);
-				oidclr(&oid);
-				flag |= REF_BAD_NAME | REF_ISBROKEN;
-			}
-			add_entry_to_dir(dir,
-					 create_ref_entry(refname.buf, &oid, flag));
+			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
 		}
 		strbuf_setlen(&refname, dirnamelen);
 	}
-- 
2.43.GIT

[PATCH v5 3/5] refs: introduce `refs_for_each_include_root_refs()`

[Karthik Nayak]

Introduce a new ref iteration flag `DO_FOR_EACH_INCLUDE_ROOT_REFS`,
which will be used to iterate over regular refs plus pseudorefs and
HEAD.

Refs which fall outside the `refs/` and aren't either pseudorefs or HEAD
are more of a grey area. This is because we don't block the users from
creating such refs but they are not officially supported.

Introduce `refs_for_each_include_root_refs()` which calls
`do_for_each_ref()` with this newly introduced flag.

In `refs/files-backend.c`, introduce a new function
`add_pseudoref_and_head_entries()` to add pseudorefs and HEAD to the
`ref_dir`. We then finally call `add_pseudoref_and_head_entries()`
whenever the `DO_FOR_EACH_INCLUDE_ROOT_REFS` flag is set. Any new ref
backend will also have to implement similar changes on its end.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c               |  7 +++++
 refs.h               |  6 ++++
 refs/files-backend.c | 65 ++++++++++++++++++++++++++++++++++++++++----
 refs/refs-internal.h |  6 ++++
 4 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 3546d90831..7d58fe1e09 100644
--- a/refs.c
+++ b/refs.c
@@ -1765,6 +1765,13 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_rawref(get_main_ref_store(the_repository), fn, cb_data);
 }
 
+int refs_for_each_include_root_refs(struct ref_store *refs, each_ref_fn fn,
+				    void *cb_data)
+{
+	return do_for_each_ref(refs, "", NULL, fn, 0,
+			       DO_FOR_EACH_INCLUDE_ROOT_REFS, cb_data);
+}
+
 static int qsort_strcmp(const void *va, const void *vb)
 {
 	const char *a = *(const char **)va;
diff --git a/refs.h b/refs.h
index f66cdd731c..5cfaee6229 100644
--- a/refs.h
+++ b/refs.h
@@ -398,6 +398,12 @@ int for_each_namespaced_ref(const char **exclude_patterns,
 int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
 int for_each_rawref(each_ref_fn fn, void *cb_data);
 
+/*
+ * Iterates over all refs including root refs, i.e. pseudorefs and HEAD.
+ */
+int refs_for_each_include_root_refs(struct ref_store *refs, each_ref_fn fn,
+				    void *cb_data);
+
 /*
  * Normalizes partial refs to their fully qualified form.
  * Will prepend <prefix> to the <pattern> if it doesn't start with 'refs/'.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 65128821a8..9c1c42fe52 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -315,9 +315,59 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
 	add_per_worktree_entries_to_dir(dir, dirname);
 }
 
-static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
+/*
+ * Add pseudorefs to the ref dir by parsing the directory for any files
+ * which follow the pseudoref syntax.
+ */
+static void add_pseudoref_and_head_entries(struct ref_store *ref_store,
+					 struct ref_dir *dir,
+					 const char *dirname)
+{
+	struct files_ref_store *refs =
+		files_downcast(ref_store, REF_STORE_READ, "fill_ref_dir");
+	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
+	struct dirent *de;
+	size_t dirnamelen;
+	DIR *d;
+
+	files_ref_path(refs, &path, dirname);
+
+	d = opendir(path.buf);
+	if (!d) {
+		strbuf_release(&path);
+		return;
+	}
+
+	strbuf_addstr(&refname, dirname);
+	dirnamelen = refname.len;
+
+	while ((de = readdir(d)) != NULL) {
+		unsigned char dtype;
+
+		if (de->d_name[0] == '.')
+			continue;
+		if (ends_with(de->d_name, ".lock"))
+			continue;
+		strbuf_addstr(&refname, de->d_name);
+
+		dtype = get_dtype(de, &path, 1);
+		if (dtype == DT_REG && (is_pseudoref(ref_store, de->d_name) ||
+								is_headref(ref_store, de->d_name)))
+			loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
+
+		strbuf_setlen(&refname, dirnamelen);
+	}
+	strbuf_release(&refname);
+	strbuf_release(&path);
+	closedir(d);
+}
+
+static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
+					     unsigned int flags)
 {
 	if (!refs->loose) {
+		struct ref_dir *dir;
+
 		/*
 		 * Mark the top-level directory complete because we
 		 * are about to read the only subdirectory that can
@@ -328,12 +378,17 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
 		/* We're going to fill the top level ourselves: */
 		refs->loose->root->flag &= ~REF_INCOMPLETE;
 
+		dir = get_ref_dir(refs->loose->root);
+
+		if (flags & DO_FOR_EACH_INCLUDE_ROOT_REFS)
+			add_pseudoref_and_head_entries(dir->cache->ref_store, dir,
+						       refs->loose->root->name);
+
 		/*
 		 * Add an incomplete entry for "refs/" (to be filled
 		 * lazily):
 		 */
-		add_entry_to_dir(get_ref_dir(refs->loose->root),
-				 create_dir_entry(refs->loose, "refs/", 5));
+		add_entry_to_dir(dir, create_dir_entry(refs->loose, "refs/", 5));
 	}
 	return refs->loose;
 }
@@ -861,7 +916,7 @@ static struct ref_iterator *files_ref_iterator_begin(
 	 * disk, and re-reads it if not.
 	 */
 
-	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
+	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
 					      prefix, ref_store->repo, 1);
 
 	/*
@@ -1222,7 +1277,7 @@ static int files_pack_refs(struct ref_store *ref_store,
 
 	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
 
-	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
+	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL,
 					the_repository, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
 		/*
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 83e0f0bba3..73a8fa18ad 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -260,6 +260,12 @@ enum do_for_each_ref_flags {
 	 * INCLUDE_BROKEN, since they are otherwise not included at all.
 	 */
 	DO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2),
+
+	/*
+	 * Include root refs i.e. HEAD and pseudorefs along with the regular
+	 * refs.
+	 */
+	DO_FOR_EACH_INCLUDE_ROOT_REFS = (1 << 3),
 };
 
 /*
-- 
2.43.GIT

[PATCH v5 4/5] ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'

[Karthik Nayak]

The flag 'FILTER_REFS_ALL' is a bit ambiguous, where ALL doesn't specify
if it means to contain refs from all worktrees or whether all types of
refs (regular, HEAD & pseudorefs) or all of the above.

Since here it is actually referring to all refs with the "refs/" prefix,
let's rename it to 'FILTER_REFS_REGULAR' to indicate that this is
specifically for regular refs.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/for-each-ref.c | 2 +-
 ref-filter.c           | 2 +-
 ref-filter.h           | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3885a9c28e..23d352e371 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -97,7 +97,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	}
 
 	filter.match_as_path = 1;
-	filter_and_format_refs(&filter, FILTER_REFS_ALL, sorting, &format);
+	filter_and_format_refs(&filter, FILTER_REFS_REGULAR, sorting, &format);
 
 	ref_filter_clear(&filter);
 	ref_sorting_release(sorting);
diff --git a/ref-filter.c b/ref-filter.c
index be14b56e32..acb960e35c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -3047,7 +3047,7 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
 			ret = for_each_fullref_in("refs/remotes/", fn, cb_data);
 		else if (filter->kind == FILTER_REFS_TAGS)
 			ret = for_each_fullref_in("refs/tags/", fn, cb_data);
-		else if (filter->kind & FILTER_REFS_ALL)
+		else if (filter->kind & FILTER_REFS_REGULAR)
 			ret = for_each_fullref_in_pattern(filter, fn, cb_data);
 		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
 			head_ref(fn, cb_data);
diff --git a/ref-filter.h b/ref-filter.h
index 07cd6f6da3..5416936800 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -19,10 +19,10 @@
 #define FILTER_REFS_BRANCHES       0x0004
 #define FILTER_REFS_REMOTES        0x0008
 #define FILTER_REFS_OTHERS         0x0010
-#define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
+#define FILTER_REFS_REGULAR        (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
 				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
 #define FILTER_REFS_DETACHED_HEAD  0x0020
-#define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
+#define FILTER_REFS_KIND_MASK      (FILTER_REFS_REGULAR | FILTER_REFS_DETACHED_HEAD)
 
 struct atom_value;
 struct ref_sorting;
-- 
2.43.GIT

[PATCH v5 5/5] for-each-ref: add new option to include root refs

[Karthik Nayak]

The git-for-each-ref(1) command doesn't provide a way to print root refs
i.e pseudorefs and HEAD with the regular "refs/" prefixed refs.

This commit adds a new option "--include-root-refs" to
git-for-each-ref(1). When used this would also print pseudorefs and HEAD
for the current worktree.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  5 ++++-
 builtin/for-each-ref.c             | 10 +++++++---
 ref-filter.c                       | 28 +++++++++++++++++++++++++--
 ref-filter.h                       |  5 ++++-
 refs/reftable-backend.c            | 11 +++++++----
 t/t6302-for-each-ref-filter.sh     | 31 ++++++++++++++++++++++++++++++
 6 files changed, 79 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 3a9ad91b7a..c1dd12b93c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
 		   [(--sort=<key>)...] [--format=<format>]
-		   [ --stdin | <pattern>... ]
+		   [--include-root-refs] [ --stdin | <pattern>... ]
 		   [--points-at=<object>]
 		   [--merged[=<object>]] [--no-merged[=<object>]]
 		   [--contains[=<object>]] [--no-contains[=<object>]]
@@ -105,6 +105,9 @@ TAB %(refname)`.
 	any excluded pattern(s) are shown. Matching is done using the
 	same rules as `<pattern>` above.
 
+--include-root-refs::
+	List root refs (HEAD and pseudorefs) apart from regular refs.
+
 FIELD NAMES
 -----------
 
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 23d352e371..919282e12a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -20,10 +20,10 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
 	struct ref_sorting *sorting;
 	struct string_list sorting_options = STRING_LIST_INIT_DUP;
-	int icase = 0;
+	int icase = 0, include_root_refs = 0, from_stdin = 0;
 	struct ref_filter filter = REF_FILTER_INIT;
 	struct ref_format format = REF_FORMAT_INIT;
-	int from_stdin = 0;
+	unsigned int flags = FILTER_REFS_REGULAR;
 	struct strvec vec = STRVEC_INIT;
 
 	struct option opts[] = {
@@ -53,6 +53,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_NO_CONTAINS(&filter.no_commit, N_("print only refs which don't contain the commit")),
 		OPT_BOOL(0, "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
 		OPT_BOOL(0, "stdin", &from_stdin, N_("read reference patterns from stdin")),
+		OPT_BOOL(0, "include-root-refs", &include_root_refs, N_("also include HEAD ref and pseudorefs")),
 		OPT_END(),
 	};
 
@@ -96,8 +97,11 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		filter.name_patterns = argv;
 	}
 
+	if (include_root_refs)
+		flags |= FILTER_REFS_ROOT_REFS;
+
 	filter.match_as_path = 1;
-	filter_and_format_refs(&filter, FILTER_REFS_REGULAR, sorting, &format);
+	filter_and_format_refs(&filter, flags, sorting, &format);
 
 	ref_filter_clear(&filter);
 	ref_sorting_release(sorting);
diff --git a/ref-filter.c b/ref-filter.c
index acb960e35c..0ec29f7385 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2628,6 +2628,12 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
 				       each_ref_fn cb,
 				       void *cb_data)
 {
+	if (filter->kind == FILTER_REFS_KIND_MASK) {
+		/* 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);
+	}
+
 	if (!filter->match_as_path) {
 		/*
 		 * in this case, the patterns are applied after
@@ -2750,6 +2756,9 @@ static int ref_kind_from_refname(const char *refname)
 			return ref_kind[i].kind;
 	}
 
+	if (is_pseudoref(get_main_ref_store(the_repository), refname))
+		return FILTER_REFS_PSEUDOREFS;
+
 	return FILTER_REFS_OTHERS;
 }
 
@@ -2781,7 +2790,16 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
 
 	/* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
 	kind = filter_ref_kind(filter, refname);
-	if (!(kind & filter->kind))
+
+	/*
+	 * 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 (!filter_pattern_match(filter, refname))
@@ -3049,7 +3067,13 @@ 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)
 			ret = for_each_fullref_in_pattern(filter, fn, cb_data);
-		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
+
+		/*
+		 * When printing all ref types, HEAD is already included,
+		 * so we don't want to print HEAD again.
+		 */
+		if (!ret && (filter->kind != FILTER_REFS_KIND_MASK) &&
+		    (filter->kind & FILTER_REFS_DETACHED_HEAD))
 			head_ref(fn, cb_data);
 	}
 
diff --git a/ref-filter.h b/ref-filter.h
index 5416936800..0ca28d2bba 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -22,7 +22,10 @@
 #define FILTER_REFS_REGULAR        (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
 				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
 #define FILTER_REFS_DETACHED_HEAD  0x0020
-#define FILTER_REFS_KIND_MASK      (FILTER_REFS_REGULAR | FILTER_REFS_DETACHED_HEAD)
+#define FILTER_REFS_PSEUDOREFS     0x0040
+#define FILTER_REFS_ROOT_REFS      (FILTER_REFS_DETACHED_HEAD | FILTER_REFS_PSEUDOREFS)
+#define FILTER_REFS_KIND_MASK      (FILTER_REFS_REGULAR | FILTER_REFS_DETACHED_HEAD | \
+				    FILTER_REFS_PSEUDOREFS)
 
 struct atom_value;
 struct ref_sorting;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index a14f2ad7f4..c23a516ac2 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -364,12 +364,15 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
 			break;
 
 		/*
-		 * The files backend only lists references contained in
-		 * "refs/". We emulate the same behaviour here and thus skip
-		 * all references that don't start with this prefix.
+		 * The files backend only lists references contained in "refs/" unless
+		 * the root refs are to be included. We emulate the same behaviour here.
 		 */
-		if (!starts_with(iter->ref.refname, "refs/"))
+		if (!starts_with(iter->ref.refname, "refs/") &&
+		    !(iter->flags & DO_FOR_EACH_INCLUDE_ROOT_REFS &&
+		     (is_pseudoref(&iter->refs->base, iter->ref.refname) ||
+		      is_headref(&iter->refs->base, iter->ref.refname)))) {
 			continue;
+		}
 
 		if (iter->prefix &&
 		    strncmp(iter->prefix, iter->ref.refname, strlen(iter->prefix))) {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 82f3d1ea0f..948f1bb5f4 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -31,6 +31,37 @@ test_expect_success 'setup some history and refs' '
 	git update-ref refs/odd/spot main
 '
 
+test_expect_success '--include-root-refs pattern prints pseudorefs' '
+	cat >expect <<-\EOF &&
+	HEAD
+	ORIG_HEAD
+	refs/heads/main
+	refs/heads/side
+	refs/odd/spot
+	refs/tags/annotated-tag
+	refs/tags/doubly-annotated-tag
+	refs/tags/doubly-signed-tag
+	refs/tags/four
+	refs/tags/one
+	refs/tags/signed-tag
+	refs/tags/three
+	refs/tags/two
+	EOF
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" --include-root-refs >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--include-root-refs with other patterns' '
+	cat >expect <<-\EOF &&
+	HEAD
+	ORIG_HEAD
+	EOF
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" --include-root-refs "*HEAD" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'filtering with --points-at' '
 	cat >expect <<-\EOF &&
 	refs/heads/main
-- 
2.43.GIT

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

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

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

Does #4 refer to this removal?

 	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))
 		return NULL;
 

If filter->kind is MASK and kind is set to filter detached HEAD, we
assign to and change the value of kind to FILTER_REFS_PSEUDOREFS,
so it is unclear if the freestanding "if kind and filter->kind does
not overlap, return NULL" was redundant or outright buggy.

The hunk just stood out to me, but I haven't read other parts of the
series yet.

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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.
>
> Does #4 refer to this removal?
>
>  	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))
>  		return NULL;
>  
>
> If filter->kind is MASK and kind is set to filter detached HEAD, we
> assign to and change the value of kind to FILTER_REFS_PSEUDOREFS,
> so it is unclear if the freestanding "if kind and filter->kind does
> not overlap, return NULL" was redundant or outright buggy.

Ah, of course this is simply redundant.  The assignment to limit the
kind to PSEUDOREFS happens only when filter->kind has all bits, and
after assigning a different non-zero value to kind, (kind & filter->kind)
will still overlap, so a freestanding "if no overlap between kind and
filter-kind then return NULL" will not trigger for such a case.

Thanks.

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

[Patrick Steinhardt]

[-- 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 --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

Thanks, let's mark the topic for 'next'.