patch, 5 partsfor-each-ref: print all refs on empty string pattern

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

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

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

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.

On Fri, Jan 19, 2024 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote:

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

Thanks!

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

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

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.

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.

Hi Karthik

On 19/01/2024 14:27, Karthik Nayak wrote:

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

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

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 agree that it is something we need to fix.

Yup.  The rationale may want to be explained better.

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.

Hi Junio

On 22/01/2024 20:22, Junio C Hamano wrote:

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?

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

Hello Phillip,

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

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

Hi Karthik

On 23/01/2024 12:49, Karthik Nayak wrote:

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

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.

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

Best Wishes

Phillip

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

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.

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

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.

On Mon, Jan 22, 2024 at 12:22:49PM -0800, Junio C Hamano wrote:

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

Hi Patrick

On 23/01/2024 11:16, Patrick Steinhardt wrote:

Thank you, I'd missed that

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 Steinhardt <ps@pks.im> writes:

Fair enough.

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

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.

On Tue, Jan 23, 2024 at 09:44:21AM -0800, Junio C Hamano wrote:

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

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

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

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

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

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.

On Fri, Jan 19, 2024 at 9:57 PM Junio C Hamano <gitster@pobox.com> wrote:

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.

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

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.

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

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

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

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

Well described.

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.

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

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/

Hello Junio,

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

Sure, will add!

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

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

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.

On Thu, Jan 25, 2024 at 5:28 PM Junio C Hamano <gitster@pobox.com> wrote:

AH! Totally misunderstood, thanks for reiterating.

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

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

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

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

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

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

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

Jeff King <peff@peff.net> writes:

Thanks Jeff, makes sense, will squash this in.

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

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

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

Hi Karthik

On 29/01/2024 11:35, Karthik Nayak wrote:

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

On Mon, Feb 05, 2024 at 06:48:52PM +0000, Phillip Wood wrote:

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

Hi Patrick

On 06/02/2024 05:33, Patrick Steinhardt wrote:

Thanks for that, I'd missed that discussion

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

Hello,

On Mon, Feb 5, 2024 at 7:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

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

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!

Hi Karthik

On 06/02/2024 08:52, Karthik Nayak wrote:

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/

Hello,

On Tue, Feb 6, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

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

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

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.

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

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.

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

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

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

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.

On Tue, Feb 6, 2024 at 11:16 PM Junio C Hamano <gitster@pobox.com> wrote:

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.

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

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

On Wed, Feb 7, 2024 at 5:00 PM Junio C Hamano <gitster@pobox.com> wrote:

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

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.

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

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.

On Wed, Feb 7, 2024 at 5:46 PM Junio C Hamano <gitster@pobox.com> wrote:

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

Thanks

On Wed, Feb 07, 2024 at 06:02:34PM +0100, Karthik Nayak wrote:

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

Patrick Steinhardt <ps@pks.im> writes:

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?

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

On Thu, Feb 08, 2024 at 09:04:54AM -0800, Junio C Hamano wrote:

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 agree, and I do not intend to change this.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

OK.

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

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?

On Thu, Feb 08, 2024 at 09:53:24AM -0800, Junio C Hamano wrote:

[snip]

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

Patrick Steinhardt <ps@pks.im> writes:

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.

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

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

On Fri, Feb 09, 2024 at 06:27:39PM +0000, Karthik Nayak wrote:

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

Hi Juino

On 06/02/2024 22:16, Junio C Hamano wrote:

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

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

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.

On Tue, Feb 06, 2024 at 02:10:41PM -0800, Karthik Nayak wrote:

[snip]

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

Patrick Steinhardt <ps@pks.im> writes:

Thanks, will take a look.

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

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.

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

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

On Sun, Feb 11, 2024 at 07:39:19PM +0100, Karthik Nayak wrote:

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.

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

Patrick Steinhardt <ps@pks.im> writes:

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.

Hello,

Patrick Steinhardt <ps@pks.im> writes:

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

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.

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:

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:

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.

Patrick Steinhardt <ps@pks.im> writes:

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.

Hello,

On Tue, Feb 13, 2024 at 8:42 PM Junio C Hamano <gitster@pobox.com> wrote:

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.

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

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.

On Wed, Feb 14, 2024 at 5:59 PM Junio C Hamano <gitster@pobox.com> wrote:

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

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

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.

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

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

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

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

On Sun, Feb 11, 2024 at 07:39:23PM +0100, Karthik Nayak wrote:

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

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

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.

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

Patrick

Patrick Steinhardt <ps@pks.im> writes:

Right, thanks, I always trip on this.

Will change

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.

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

On Thu, Feb 22, 2024 at 04:57:04AM -0800, Karthik Nayak wrote:

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

Patrick

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

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

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

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

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

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

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

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.

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

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.

On Fri, Feb 23, 2024 at 11:01:07AM +0100, Karthik Nayak wrote:

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

Patrick

Patrick Steinhardt <ps@pks.im> writes:

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