| Message ID | 20240119142705.139374-5-karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | for-each-ref: print all refs on empty string pattern | expand |
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.
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.
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.
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), }; /*
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(-)