Separate `ref_cache` into a separate module
v2 cover Separate `ref_cache` into a separate module
This is v2 of this patch series. Thanks to Peff, Junio, Stefan and Ævar for their comments about v1 [1].
This version literally only contains a few commit message changes and one minor comment changes relative to v1. The code is identical. I wasn't sure whether it is even worth sending this patch series to the ML again; Junio, if you'd prefer I just send a link to a published branch in such cases, please let me know.
* I picked up the "area:" prefixes that Junio added to a couple of commit subject lines.
* I clarified the justification for keeping a pointer to the ref_store in ref_dir.
* I added a long blurb about the removal of an internal consistency check in the commit message for
[18/20] commit_packed_refs(): use reference iteration
* I changed "loose refs cache" to "loose ref cache" in a comment in
[19/20] files_pack_refs(): use reference iteration
This patch series is also available from my GitHub fork [2] as branch "separate-ref-cache". These patches depend on Duy's nd/files-backend-git-dir branch.
[1] http://public-inbox.org/git/[email protected]/
[2] https://github.com/mhagger/git
Michael Haggerty (20):
get_ref_dir(): don't call read_loose_refs() for "refs/bisect" refs_read_raw_ref(): new function refs_ref_iterator_begin(): new function refs_verify_refname_available(): implement once for all backends refs_verify_refname_available(): use function in more places
ref-cache: rename add_ref() to add_ref_entry()
ref-cache: rename find_ref() to find_ref_entry()
ref-cache: rename remove_entry() to remove_entry_from_dir()
refs: split ref_cache code into separate files
ref-cache: introduce a new type, ref_cache
refs: record the ref_store in ref_cache, not ref_dir
ref-cache: use a callback function to fill the cache
refs: handle "refs/bisect/" in loose_fill_ref_dir() do_for_each_entry_in_dir(): eliminate offset argument get_loose_ref_dir(): function renamed from get_loose_refs() get_loose_ref_cache(): new function cache_ref_iterator_begin(): make function smarter commit_packed_refs(): use reference iteration files_pack_refs(): use reference iteration do_for_each_entry_in_dir(): delete function
Makefile | 1 + refs.c | 111 ++++- refs.h | 2 +- refs/files-backend.c | 1229 +++++++------------------------------------------- refs/ref-cache.c | 523 +++++++++++++++++++++ refs/ref-cache.h | 267 +++++++++++ refs/refs-internal.h | 22 +- 7 files changed, 1066 insertions(+), 1089 deletions(-) create mode 100644 refs/ref-cache.c create mode 100644 refs/ref-cache.h
Signature · 1 line
2.11.0
PATCH v2 1/20 get_ref_dir(): don't call read_loose_refs() for "refs/bisect"
Since references under "refs/bisect/" are per-worktree, they have to be sought in the worktree rather than in the main repository. But since loose references are found by traversing directories, the reference iterator won't even get the idea to look for a "refs/bisect/" directory in the worktree if there is not a directory with that name in the main repository. Thus get_ref_dir() manually inserts a dir_entry for "refs/bisect/" whenever it reads the entry for "refs/".
The current code then immediately calls read_loose_refs() on that directory. But since the dir_entry is created with its incomplete flag set, any traversal that gets to this point will read the directory automatically. So there is no need to call read_loose_refs() explicitly; the lazy mechanism suffices.
And in fact, the attempt to read_loose_refs() was broken anyway. That function needs its dirname argument to have a trailing / character, but the invocation here was passing it "refs/bisect" without a trailing slash. So read_loose_refs() would read `$GIT_DIR/refs/bisect" correctly, but if it found an entry "foo" in that directory, it would try to read "$GIT_DIR/refs/bisectfoo". Normally it wouldn't find anything at that path, but the failure was canceled out because get_ref_dir() *also* forgot to reset the REF_INCOMPLETE bit on the dir_entry. So the read was attempted again when it was accessed, via the lazy mechanism, and this time the read was done correctly.
This code has been broken since it was first introduced.
--- refs/files-backend.c | 2 -- 1 file changed, 2 deletions(-)
refs/files-backend.c +0 -2
@@ -191,8 +191,6 @@ static struct ref_dir *get_ref_dir(struct ref_entry *entry)"refs/bisect/" , 12, 1);add_entry_to_dir (dir, child_entry); -read_loose_refs ("refs/bisect" , - &child_entry->u.subdir); } } entry->flag &= ~REF_INCOMPLETE ;
Signature · 1 line
2.11.0
Re: [PATCH v2 01/20] get_ref_dir(): don't call read_loose_refs() for "refs/bisect"
Quoted 15 lines · Michael Haggerty
Since references under "refs/bisect/" are per-worktree, they have to be sought in the worktree rather than in the main repository. But since loose references are found by traversing directories, the reference iterator won't even get the idea to look for a "refs/bisect/" directory in the worktree if there is not a directory with that name in the main repository. Thus
get_ref_dir()manually inserts adir_entryfor "refs/bisect/" whenever it reads the entry for "refs/".The current code then immediately calls
read_loose_refs()on that directory. But since thedir_entryis created with itsincompleteflag set, any traversal that gets to this point will read the directory automatically. So there is no need to callread_loose_refs()explicitly; the lazy mechanism suffices.
Quoted 11 lines
And in fact, the attempt to
read_loose_refs()was broken anyway. That function needs itsdirnameargument to have a trailing/character, but the invocation here was passing it "refs/bisect" without a trailing slash. Soread_loose_refs()would read `$GIT_DIR/refs/bisect" correctly, but if it found an entry "foo" in that directory, it would try to read "$GIT_DIR/refs/bisectfoo". Normally it wouldn't find anything at that path, but the failure was canceled out becauseget_ref_dir()*also* forgot to reset theREF_INCOMPLETEbit on thedir_entry. So the read was attempted again when it was accessed, via the lazy mechanism, and this time the read was done correctly.
With all this retry logic going on, it is hard to add a test demonstrating this is correct now. The description makes sense though.
Thanks,
Stefan
PATCH v2 2/20 refs_read_raw_ref(): new function
In reply to Michael HaggertyExtract a new function from refs_resolve_ref_unsafe(). It will be useful elsewhere.
--- refs.c | 11 +++++++++-- refs/refs-internal.h | 4 ++++ 2 files changed, 13 insertions(+), 2 deletions(-)
refs.c +9 -2
@@ -1326,6 +1326,13 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)return refs_for_each_rawref (get_main_ref_store (), fn, cb_data); } +int refs_read_raw_ref (struct ref_store *ref_store, +const char *refname,unsigned char *sha1, +struct strbuf *referent,unsigned int *type) +{ +return ref_store->be->read_raw_ref (ref_store, refname, sha1, referent, type); +} +/* This function needs to return a meaningful errno on failure */ const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
@@ -1362,8 +1369,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,for (symref_count =0 ; symref_count <SYMREF_MAXDEPTH ; symref_count++) {unsigned int read_flags =0 ; -if (refs->be->read_raw_ref (refs, refname, - sha1, &sb_refname, &read_flags)) { +if (refs_read_raw_ref (refs, refname, + sha1, &sb_refname, &read_flags)) { *flags |= read_flags;if (errno !=ENOENT || (resolve_flags &RESOLVE_REF_READING ))return NULL ;
refs/refs-internal.h +4 -0
@@ -165,6 +165,10 @@ struct ref_update {const char refname[FLEX_ARRAY ]; }; +int refs_read_raw_ref (struct ref_store *ref_store, +const char *refname,unsigned char *sha1, +struct strbuf *referent,unsigned int *type); +/* * Add a ref_update with the specified properties to transaction, and * return a pointer to the new object. This function does not verify
Signature · 1 line
2.11.0
PATCH v2 3/20 refs_ref_iterator_begin(): new function
In reply to Michael HaggertyExtract a new function from do_for_each_ref(). It will be useful elsewhere.
--- refs.c | 15 +++++++++++++-- refs/refs-internal.h | 11 +++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-)
refs.c +13 -2
@@ -1230,6 +1230,18 @@ int head_ref(each_ref_fn fn, void *cb_data)return head_ref_submodule (NULL , fn, cb_data); } +struct ref_iterator *refs_ref_iterator_begin ( +struct ref_store *refs, +const char *prefix,int trim,int flags) +{ +struct ref_iterator *iter; + + iter = refs->be->iterator_begin (refs, prefix, flags); + iter =prefix_ref_iterator_begin (iter, prefix, trim); + +return iter; +} +/* * Call fn for each reference in the specified submodule for which the * refname begins with prefix. If trim is non-zero, then trim that
@@ -1247,8 +1259,7 @@ static int do_for_each_ref(struct ref_store *refs, const char *prefix,if (!refs)return 0 ; - iter = refs->be->iterator_begin (refs, prefix, flags); - iter =prefix_ref_iterator_begin (iter, prefix, trim); + iter =refs_ref_iterator_begin (refs, prefix, trim, flags);return do_for_each_ref_iterator (iter, fn, cb_data); }
refs/refs-internal.h +11 -0
@@ -335,6 +335,17 @@ struct ref_iterator *empty_ref_iterator_begin(void);*/ int is_empty_ref_iterator (struct ref_iterator *ref_iterator); +/* +* Return an iterator that goes over each reference in `refs` for +* which the refname begins with prefix. If trim is non-zero, then +* trim that many characters off the beginning of each refname. flags +* can be DO_FOR_EACH_INCLUDE_BROKEN to include broken references in +* the iteration. +*/ +struct ref_iterator *refs_ref_iterator_begin ( +struct ref_store *refs, +const char *prefix,int trim,int flags); +/* * A callback function used to instruct merge_ref_iterator how to * interleave the entries from iter0 and iter1. The function should
Signature · 1 line
2.11.0
Re: [PATCH v2 03/20] refs_ref_iterator_begin(): new function
Quoted 15 lines · Michael Haggerty
Extract a new function from
do_for_each_ref(). It will be useful elsewhere.Signed-off-by Michael Haggerty <[email protected]>--- refs.c | 15 +++++++++++++-- refs/refs-internal.h | 11 +++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-)refs.c +0
-0@@ -1230,6 +1230,18 @@ int head_ref(each_ref_fn fn, void *cb_data)return head_ref_submodule (NULL , fn, cb_data);
refs.c int head_ref(each_ref_fn fn, void *cb_data) · line 1240
Quoted 10 lines
refs.c +8
-0} +struct ref_iterator *refs_ref_iterator_begin ( +struct ref_store *refs, +const char *prefix,int trim,int flags) +{ +struct ref_iterator *iter; + + iter = refs->be->iterator_begin (refs, prefix, flags); + iter =prefix_ref_iterator_begin (iter, prefix, trim);
Off topic. This code made me wonder if we really need the prefix iterator if prefix is NULL. And in fact we don't since prefix_ref_iterator_begin() will return the old iter in that case. But it's probably better to move that optimization outside. I think it's easier to understand that way, calling prefix_ref_ will always give you a new iterator. Don't call it unless you want to have it.
refs.c int head_ref(each_ref_fn fn, void *cb_data) · line 1247
Quoted 7 lines
refs.c +7
-0+/* +* Return an iterator that goes over each reference in `refs` for +* which the refname begins with prefix. If trim is non-zero, then +* trim that many characters off the beginning of each refname. flags +* can be DO_FOR_EACH_INCLUDE_BROKEN to include broken references in +* the iteration. +*/
Do we need a separate docstring here? I think we document more or less the same for ref_iterator_begin_fn (except the include-broken flag).
Quoted 4 lines
refs.c +4
-0+struct ref_iterator *refs_ref_iterator_begin ( +struct ref_store *refs, +const char *prefix,int trim,int flags); +
Signature · 1 line
Duy
Re: [PATCH v2 03/20] refs_ref_iterator_begin(): new function
Quoted 18 lines · Duy Nguyen
Quoted 17 lines · Michael Haggerty
Extract a new function from
do_for_each_ref(). It will be useful elsewhere.Signed-off-by Michael Haggerty <[email protected]>--- refs.c | 15 +++++++++++++-- refs/refs-internal.h | 11 +++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-)refs.c +0
-0@@ -1230,6 +1230,18 @@ int head_ref(each_ref_fn fn, void *cb_data)return head_ref_submodule (NULL , fn, cb_data); }
Quoted 15 lines
refs.c int head_ref(each_ref_fn fn, void *cb_data) · line 1240
Quoted 8 lines
refs.c +8
-0+struct ref_iterator *refs_ref_iterator_begin ( +struct ref_store *refs, +const char *prefix,int trim,int flags) +{ +struct ref_iterator *iter; + + iter = refs->be->iterator_begin (refs, prefix, flags); + iter =prefix_ref_iterator_begin (iter, prefix, trim);Off topic. This code made me wonder if we really need the prefix iterator if prefix is NULL. And in fact we don't since
prefix_ref_iterator_begin()will return the old iter in that case. But it's probably better to move that optimization outside. I think it's easier to understand that way, callingprefix_ref_will always give you a new iterator. Don't call it unless you want to have it.
I like this aspect of the design because it is more powerful. Consider for example that some reference backend might (e.g., a future packed_ref_store) need to use a prefix_ref_iterator to implement iterator_begin(), while another (e.g., one based on ref_cache) might not. It would be easy to make prefix_ref_iterator_begin() check whether its argument is already a prefix_ref_iterator, and if so, swap it out with a new one with different arguments (to avoid having to stack them up). It would be inappropriate for the caller to make such an optimization, because it (a) shouldn't have to care what type of reference iterator it is dealing with, and (b) shouldn't have to know enough about prefix_ref_iterators to know that the optimization makes sense.
Quoted 10 lines
refs.c int head_ref(each_ref_fn fn, void *cb_data) · line 1247
Quoted 7 lines
refs.c +7
-0+/* +* Return an iterator that goes over each reference in `refs` for +* which the refname begins with prefix. If trim is non-zero, then +* trim that many characters off the beginning of each refname. flags +* can be DO_FOR_EACH_INCLUDE_BROKEN to include broken references in +* the iteration. +*/ Do we need a separate docstring here? I think we document more or less the same for
ref_iterator_begin_fn(except the include-broken flag).
There is a subtle difference: currently, ref_iterator_begin_fn() doesn't use its full prefix argument as prefix, but rather find_containing_dir(prefix). (This is basically an implementation detail of ref_cache leaking out into the virtual function interface.)
refs_ref_iterator_begin(), on the other hand, treats the prefix literally, using starts_with().
I don't like this design and plan to improve it sometime, but for now that's an important difference. The fix, incidentally, would motivate the prefix_ref_iterator_begin() optimization mentioned above.
Michael
PATCH v2 4/20 refs_verify_refname_available(): implement once for all backends
In reply to Michael HaggertyIt turns out that we can now implement refs_verify_refname_available() based on the other virtual functions, so there is no need for it to be defined at the backend level. Instead, define it once in refs.c and remove the files_backend definition.
--- refs.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++-- refs.h | 2 +- refs/files-backend.c | 39 +++++------------------- refs/refs-internal.h | 7 ----- 4 files changed, 92 insertions(+), 41 deletions(-)
refs.c +83 -2
@@ -5,6 +5,7 @@#include "cache.h" #include "hashmap.h" #include "lockfile.h" +#include "iterator.h" #include "refs.h" #include "refs/refs-internal.h" #include "object.h"
@@ -1661,11 +1662,91 @@ int ref_transaction_commit(struct ref_transaction *transaction,int refs_verify_refname_available (struct ref_store *refs,const char *refname, -const struct string_list *extra, +const struct string_list *extras,const struct string_list *skip,struct strbuf *err) { -return refs->be->verify_refname_available (refs, refname, extra, skip, err); +const char *slash; +const char *extra_refname; +struct strbuf dirname =STRBUF_INIT ; +struct strbuf referent =STRBUF_INIT ; +struct object_id oid; +unsigned int type; +struct ref_iterator *iter; +int ok; +int ret =-1 ; + +/* +* For the sake of comments in this function, suppose that +* refname is "refs/foo/bar". +*/ + +assert (err); + +strbuf_grow (&dirname,strlen (refname) +1 ); +for (slash =strchr (refname,'/' ); slash; slash =strchr (slash +1 ,'/' )) { +/* Expand dirname to the new prefix, not including the trailing slash: */ +strbuf_add (&dirname, refname + dirname.len, slash - refname - dirname.len); + +/* +* We are still at a leading dir of the refname (e.g., +* "refs/foo"; if there is a reference with that name, +* it is a conflict, *unless* it is in skip. +*/ +if (skip &&string_list_has_string (skip, dirname.buf)) +continue ; + +if (!refs_read_raw_ref (refs, dirname.buf, oid.hash, &referent, &type)) { +strbuf_addf (err,"'%s' exists; cannot create '%s'" , + dirname.buf, refname); + goto cleanup; + } + +if (extras &&string_list_has_string (extras, dirname.buf)) { +strbuf_addf (err,"cannot process '%s' and '%s' at the same time" , + refname, dirname.buf); + goto cleanup; + } + } + +/* +* We are at the leaf of our refname (e.g., "refs/foo/bar"). +* There is no point in searching for a reference with that +* name, because a refname isn't considered to conflict with +* itself. But we still need to check for references whose +* names are in the "refs/foo/bar/" namespace, because they +* *do* conflict. +*/ +strbuf_addstr (&dirname, refname + dirname.len); +strbuf_addch (&dirname,'/' ); + + iter =refs_ref_iterator_begin (refs, dirname.buf,0 , +DO_FOR_EACH_INCLUDE_BROKEN ); +while ((ok =ref_iterator_advance (iter)) ==ITER_OK ) { +if (skip && +string_list_has_string (skip, iter->refname)) +continue ; + +strbuf_addf (err,"'%s' exists; cannot create '%s'" , + iter->refname, refname); + ok =ref_iterator_abort (iter); + goto cleanup; + } + +if (ok !=ITER_DONE ) +die ("BUG: error while iterating over references" ); + + extra_refname =find_descendant_ref (dirname.buf, extras, skip); +if (extra_refname) +strbuf_addf (err,"cannot process '%s' and '%s' at the same time" , + refname, extra_refname); +else + ret =0 ; + +cleanup: +strbuf_release (&referent); +strbuf_release (&dirname); +return ret; }int refs_for_each_reflog (struct ref_store *refs,each_ref_fn fn,void *cb_data)
refs.h +1 -1
@@ -97,7 +97,7 @@ int read_ref(const char *refname, unsigned char *sha1);int refs_verify_refname_available (struct ref_store *refs,const char *refname, -const struct string_list *extra, +const struct string_list *extras,const struct string_list *skip,struct strbuf *err);
refs/files-backend.c +8 -31
@@ -1725,10 +1725,9 @@ static int lock_raw_ref(struct files_ref_store *refs, goto error_return; }else if (remove_dir_recursively (&ref_file,REMOVE_DIR_EMPTY_ONLY )) { -if (verify_refname_available_dir ( - refname, extras, skip, -get_loose_refs (refs), - err)) { +if (refs_verify_refname_available ( + &refs->base, refname, + extras, skip, err)) {/* * The error message set by * verify_refname_available() is OK.
@@ -2097,9 +2096,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,*/ if (remove_empty_directories (&ref_file)) { last_errno = errno; -if (!verify_refname_available_dir ( - refname, extras, skip, -get_loose_refs (refs), err)) +if (!refs_verify_refname_available ( + &refs->base, + refname, extras, skip, err))strbuf_addf (err,"there are still refs under '%s'" , refname); goto error_return;
@@ -2111,9 +2110,8 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,if (!resolved) { last_errno = errno;if (last_errno !=ENOTDIR || - !verify_refname_available_dir ( - refname, extras, skip, -get_loose_refs (refs), err)) + !refs_verify_refname_available (&refs->base, refname, + extras, skip, err))strbuf_addf (err,"unable to resolve reference '%s': %s" , refname,strerror (last_errno));
@@ -2609,26 +2607,6 @@ static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)return ret; } -static int files_verify_refname_available (struct ref_store *ref_store, -const char *newname, -const struct string_list *extras, -const struct string_list *skip, -struct strbuf *err) -{ -struct files_ref_store *refs = -files_downcast (ref_store,REF_STORE_READ ,"verify_refname_available" ); -struct ref_dir *packed_refs =get_packed_refs (refs); -struct ref_dir *loose_refs =get_loose_refs (refs); - -if (verify_refname_available_dir (newname, extras, skip, - packed_refs, err) || -verify_refname_available_dir (newname, extras, skip, - loose_refs, err)) -return -1 ; - -return 0 ; -} -static int write_ref_to_lockfile (struct ref_lock *lock,const unsigned char *sha1,struct strbuf *err); static int commit_ref_update(struct files_ref_store *refs,
@@ -4291,7 +4269,6 @@ struct ref_storage_be refs_be_files = { files_ref_iterator_begin, files_read_raw_ref, - files_verify_refname_available, files_reflog_iterator_begin, files_for_each_reflog_ent,
refs/refs-internal.h +0 -7
@@ -590,12 +590,6 @@ typedef int read_raw_ref_fn(struct ref_store *ref_store, const char *refname, unsigned char *sha1, struct strbuf *referent, unsigned int *type); -typedef int verify_refname_available_fn (struct ref_store *ref_store, -const char *newname, -const struct string_list *extras, -const struct string_list *skip, -struct strbuf *err); -struct ref_storage_be {struct ref_storage_be *next;const char *name;
@@ -612,7 +606,6 @@ struct ref_storage_be {ref_iterator_begin_fn *iterator_begin;read_raw_ref_fn *read_raw_ref; -verify_refname_available_fn *verify_refname_available;reflog_iterator_begin_fn *reflog_iterator_begin;for_each_reflog_ent_fn *for_each_reflog_ent;
Signature · 1 line
2.11.0
Re: [PATCH v2 04/20] refs_verify_refname_available(): implement once for all backends
Quoted 11 lines · Michael Haggerty
It turns out that we can now implement
refs_verify_refname_available()based on the other virtual functions, so there is no need for it to be defined at the backend level. Instead, define it once inrefs.cand remove thefiles_backenddefinition.Signed-off-by Michael Haggerty <[email protected]>--- refs.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++-- refs.h | 2 +- refs/files-backend.c | 39 +++++-------------------
Much appreciated. This will make future backends simpler to implement as well.
refs.c int ref_transaction_commit(struct ref_transaction *transaction, · line 1732
Quoted 10 lines
refs.c +10
-0+ iter =refs_ref_iterator_begin (refs, dirname.buf,0 , +DO_FOR_EACH_INCLUDE_BROKEN ); +while ((ok =ref_iterator_advance (iter)) ==ITER_OK ) { +if (skip && +string_list_has_string (skip, iter->refname)) +continue ; + +strbuf_addf (err,"'%s' exists; cannot create '%s'" , + iter->refname, refname); + ok =ref_iterator_abort (iter);
Saving the return code in "ok" seems redundant because you don't use it. Or did you want to check that ok == ITER_DONE or die() too?
Quoted 18 lines
refs.c +17
-0+ goto cleanup; + } + +if (ok !=ITER_DONE ) +die ("BUG: error while iterating over references" ); + + extra_refname =find_descendant_ref (dirname.buf, extras, skip); +if (extra_refname) +strbuf_addf (err,"cannot process '%s' and '%s' at the same time" , + refname, extra_refname); +else + ret =0 ; + +cleanup: +strbuf_release (&referent); +strbuf_release (&dirname); +return ret; }
Signature · 1 line
Duy
Re: [PATCH v2 04/20] refs_verify_refname_available(): implement once for all backends
Quoted 26 lines · Duy Nguyen
Quoted 11 lines · Michael Haggerty
It turns out that we can now implement
refs_verify_refname_available()based on the other virtual functions, so there is no need for it to be defined at the backend level. Instead, define it once inrefs.cand remove thefiles_backenddefinition.Signed-off-by Michael Haggerty <[email protected]>--- refs.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++-- refs.h | 2 +- refs/files-backend.c | 39 +++++-------------------Much appreciated. This will make future backends simpler to implement as well.
refs.c int ref_transaction_commit(struct ref_transaction *transaction, · line 1732
Quoted 10 lines
refs.c +10
-0+ iter =refs_ref_iterator_begin (refs, dirname.buf,0 , +DO_FOR_EACH_INCLUDE_BROKEN ); +while ((ok =ref_iterator_advance (iter)) ==ITER_OK ) { +if (skip && +string_list_has_string (skip, iter->refname)) +continue ; + +strbuf_addf (err,"'%s' exists; cannot create '%s'" , + iter->refname, refname); + ok =ref_iterator_abort (iter);
Quoted 2 lines
Saving the return code in "ok" seems redundant because you don't use it. Or did you want to check that ok ==
ITER_DONEordie()too?
True, setting ok here is redundant. I don't think there's much point worrying about whether ref_iterator_abort() fails here, since we've already gotten the information that we require.
I'll remove it.
Thanks,
Michael
PATCH v2 5/20 refs_verify_refname_available(): use function in more places
In reply to Michael HaggertyChange lock_raw_ref() and lock_ref_sha1_basic() to use refs_verify_refname_available() instead of verify_refname_available_dir(). This means that those callsites now check for conflicts with all references rather than just packed refs, but the performance cost shouldn't be significant (and will be regained later).
These were the last callers of verify_refname_available_dir(), so also delete that (very complicated) function.
--- refs/files-backend.c | 171 ++++----------------------------------------------- 1 file changed, 11 insertions(+), 160 deletions(-)
refs/files-backend.c +11 -160
@@ -738,152 +738,6 @@ static struct ref_iterator *cache_ref_iterator_begin(struct ref_dir *dir)return ref_iterator; } -struct nonmatching_ref_data { -const struct string_list *skip; -const char *conflicting_refname; -}; - -static int nonmatching_ref_fn (struct ref_entry *entry,void *vdata) -{ -struct nonmatching_ref_data *data = vdata; - -if (data->skip &&string_list_has_string (data->skip, entry->name)) -return 0 ; - - data->conflicting_refname = entry->name; -return 1 ; -} - -/* -* Return 0 if a reference named refname could be created without -* conflicting with the name of an existing reference in dir. -* See verify_refname_available for more information. -*/ -static int verify_refname_available_dir (const char *refname, -const struct string_list *extras, -const struct string_list *skip, -struct ref_dir *dir, -struct strbuf *err) -{ -const char *slash; -const char *extra_refname; -int pos; -struct strbuf dirname =STRBUF_INIT ; -int ret =-1 ; - -/* -* For the sake of comments in this function, suppose that -* refname is "refs/foo/bar". -*/ - -assert (err); - -strbuf_grow (&dirname,strlen (refname) +1 ); -for (slash =strchr (refname,'/' ); slash; slash =strchr (slash +1 ,'/' )) { -/* Expand dirname to the new prefix, not including the trailing slash: */ -strbuf_add (&dirname, refname + dirname.len, slash - refname - dirname.len); - -/* -* We are still at a leading dir of the refname (e.g., -* "refs/foo"; if there is a reference with that name, -* it is a conflict, *unless* it is in skip. -*/ -if (dir) { - pos =search_ref_dir (dir, dirname.buf, dirname.len); -if (pos >=0 && - (!skip || !string_list_has_string (skip, dirname.buf))) { -/* -* We found a reference whose name is -* a proper prefix of refname; e.g., -* "refs/foo", and is not in skip. -*/ -strbuf_addf (err,"'%s' exists; cannot create '%s'" , - dirname.buf, refname); - goto cleanup; - } - } - -if (extras &&string_list_has_string (extras, dirname.buf) && - (!skip || !string_list_has_string (skip, dirname.buf))) { -strbuf_addf (err,"cannot process '%s' and '%s' at the same time" , - refname, dirname.buf); - goto cleanup; - } - -/* -* Otherwise, we can try to continue our search with -* the next component. So try to look up the -* directory, e.g., "refs/foo/". If we come up empty, -* we know there is nothing under this whole prefix, -* but even in that case we still have to continue the -* search for conflicts with extras. -*/ -strbuf_addch (&dirname,'/' ); -if (dir) { - pos =search_ref_dir (dir, dirname.buf, dirname.len); -if (pos <0 ) { -/* -* There was no directory "refs/foo/", -* so there is nothing under this -* whole prefix. So there is no need -* to continue looking for conflicting -* references. But we need to continue -* looking for conflicting extras. -*/ - dir =NULL ; - }else { - dir =get_ref_dir (dir->entries[pos]); - } - } - } - -/* -* We are at the leaf of our refname (e.g., "refs/foo/bar"). -* There is no point in searching for a reference with that -* name, because a refname isn't considered to conflict with -* itself. But we still need to check for references whose -* names are in the "refs/foo/bar/" namespace, because they -* *do* conflict. -*/ -strbuf_addstr (&dirname, refname + dirname.len); -strbuf_addch (&dirname,'/' ); - -if (dir) { - pos =search_ref_dir (dir, dirname.buf, dirname.len); - -if (pos >=0 ) { -/* -* We found a directory named "$refname/" -* (e.g., "refs/foo/bar/"). It is a problem -* iff it contains any ref that is not in -* "skip". -*/ -struct nonmatching_ref_data data; - - data.skip = skip; - data.conflicting_refname =NULL ; - dir =get_ref_dir (dir->entries[pos]); -sort_ref_dir (dir); -if (do_for_each_entry_in_dir (dir,0 , nonmatching_ref_fn, &data)) { -strbuf_addf (err,"'%s' exists; cannot create '%s'" , - data.conflicting_refname, refname); - goto cleanup; - } - } - } - - extra_refname =find_descendant_ref (dirname.buf, extras, skip); -if (extra_refname) -strbuf_addf (err,"cannot process '%s' and '%s' at the same time" , - refname, extra_refname); -else - ret =0 ; - -cleanup: -strbuf_release (&dirname); -return ret; -} -struct packed_ref_cache {struct ref_entry *root;
@@ -1563,7 +1417,7 @@ static void unlock_ref(struct ref_lock *lock)* * If the reference doesn't already exist, verify that refname doesn't * have a D/F conflict with any existing references. extras and skip -* are passed to verify_refname_available_dir() for this check. +* are passed to refs_verify_refname_available() for this check. * * If mustexist is not set and the reference is not found or is * broken, lock the reference anyway but clear sha1.
@@ -1578,7 +1432,7 @@ static void unlock_ref(struct ref_lock *lock)* * but it includes a lot more code to * - Deal with possible races with other processes -* - Avoid calling verify_refname_available_dir() when it can be +* - Avoid calling refs_verify_refname_available() when it can be * avoided, namely if we were successfully able to read the ref * - Generate informative error messages in the case of failure */
@@ -1635,7 +1489,8 @@ static int lock_raw_ref(struct files_ref_store *refs, }else {/* * The error message set by -* verify_refname_available_dir() is OK. +* refs_verify_refname_available() is +* OK. */ ret =TRANSACTION_NAME_CONFLICT ; }
@@ -1759,16 +1614,13 @@ static int lock_raw_ref(struct files_ref_store *refs,/* * If the ref did not exist and we are creating it, -* make sure there is no existing packed ref whose -* name begins with our refname, nor a packed ref -* whose name is a proper prefix of our refname. +* make sure there is no existing ref that conflicts +* with refname: */ -if (verify_refname_available_dir ( - refname, extras, skip, -get_packed_refs (refs), - err)) { +if (refs_verify_refname_available ( + &refs->base, refname, + extras, skip, err)) goto error_return; - } } ret =0 ;
@@ -2125,9 +1977,8 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,* our refname. */ if (is_null_oid (&lock->old_oid) && -verify_refname_available_dir (refname, extras, skip, -get_packed_refs (refs), - err)) { +refs_verify_refname_available (&refs->base, refname, + extras, skip, err)) { last_errno =ENOTDIR ; goto error_return; }
Signature · 1 line
2.11.0
PATCH v2 6/20 ref-cache: rename `add_ref()` to `add_ref_entry()`
In reply to Michael HaggertyThis function's visibility is about to be increased, so give it a more distinctive name.
--- refs/files-backend.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
refs/files-backend.c +4 -4
@@ -455,7 +455,7 @@ static int remove_entry(struct ref_dir *dir, const char *refname)* subdirectories as necessary. dir must represent the top-level * directory. Return 0 on success. */ -static int add_ref (struct ref_dir *dir,struct ref_entry *ref) +static int add_ref_entry (struct ref_dir *dir,struct ref_entry *ref) { dir =find_containing_dir (dir, ref->name,1 );if (!dir)
@@ -994,7 +994,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)if (peeled ==PEELED_FULLY || (peeled ==PEELED_TAGS &&starts_with (refname,"refs/tags/" ))) last->flag |=REF_KNOWS_PEELED ; -add_ref (dir, last); +add_ref_entry (dir, last);continue ; } if (last &&
@@ -1116,7 +1116,7 @@ static void add_packed_ref(struct files_ref_store *refs,if (!packed_ref_cache->lock)die ("internal error: packed refs not locked" ); -add_ref (get_packed_ref_dir (packed_ref_cache), +add_ref_entry (get_packed_ref_dir (packed_ref_cache),create_ref_entry (refname, sha1,REF_ISPACKED ,1 )); }
@@ -2179,7 +2179,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data) }else { packed_entry =create_ref_entry (entry->name, entry->u.value.oid.hash,REF_ISPACKED |REF_KNOWS_PEELED ,0 ); -add_ref (cb->packed_refs, packed_entry); +add_ref_entry (cb->packed_refs, packed_entry); }oidcpy (&packed_entry->u.value.peeled, &entry->u.value.peeled);
Signature · 1 line
2.11.0
PATCH v2 7/20 ref-cache: rename `find_ref()` to `find_ref_entry()`
In reply to Michael HaggertyThis function's visibility is about to be increased, so give it a more distinctive name.
--- refs/files-backend.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
refs/files-backend.c +3 -3
@@ -385,7 +385,7 @@ static struct ref_dir *find_containing_dir(struct ref_dir *dir,* and recursing into subdirectories as necessary. If the name is not * found or it corresponds to a directory entry, return NULL. */ -static struct ref_entry *find_ref (struct ref_dir *dir,const char *refname) +static struct ref_entry *find_ref_entry (struct ref_dir *dir,const char *refname) {int entry_index;struct ref_entry *entry;
@@ -1227,7 +1227,7 @@ static struct ref_dir *get_loose_refs(struct files_ref_store *refs)static struct ref_entry *get_packed_ref (struct files_ref_store *refs,const char *refname) { -return find_ref (get_packed_refs (refs), refname); +return find_ref_entry (get_packed_refs (refs), refname); }/*
@@ -2171,7 +2171,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)if (peel_status !=PEEL_PEELED && peel_status !=PEEL_NON_TAG )die ("internal error peeling reference %s (%s)" , entry->name,oid_to_hex (&entry->u.value.oid)); - packed_entry =find_ref (cb->packed_refs, entry->name); + packed_entry =find_ref_entry (cb->packed_refs, entry->name);if (packed_entry) {/* Overwrite existing packed entry with info from loose entry */ packed_entry->flag =REF_ISPACKED |REF_KNOWS_PEELED ;
Signature · 1 line
2.11.0
PATCH v2 8/20 ref-cache: rename `remove_entry()` to `remove_entry_from_dir()`
In reply to Michael HaggertyThis function's visibility is about to be increased, so give it a more distinctive name.
--- refs/files-backend.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
refs/files-backend.c +2 -2
@@ -413,7 +413,7 @@ static struct ref_entry *find_ref_entry(struct ref_dir *dir, const char *refname* empty by the removal. dir must represent the top-level directory * and must already be complete. */ -static int remove_entry (struct ref_dir *dir,const char *refname) +static int remove_entry_from_dir (struct ref_dir *dir,const char *refname) {int refname_len =strlen (refname);int entry_index;
@@ -2338,7 +2338,7 @@ static int repack_without_refs(struct files_ref_store *refs,/* Remove refnames from the cache */ for_each_string_list_item (refname, refnames) -if (remove_entry (packed, refname->string) !=-1 ) +if (remove_entry_from_dir (packed, refname->string) !=-1 ) removed =1 ;if (!removed) {/*
Signature · 1 line
2.11.0
PATCH v2 9/20 refs: split `ref_cache` code into separate files
In reply to Michael HaggertyThe ref_cache code is currently too tightly coupled to files-backend, making the code harder to understand and making it awkward for new code to use ref_cache (as we indeed have planned). Start loosening that coupling by splitting ref_cache into a separate module.
This commit moves code, adds declarations, and changes the visibility of some functions, but doesn't change any code.
The modules are still too tightly coupled, but the situation will be improved in subsequent commits.
--- Makefile | 1 + refs/files-backend.c | 736 +-------------------------------------------------- refs/ref-cache.c | 512 +++++++++++++++++++++++++++++++++++ refs/ref-cache.h | 251 ++++++++++++++++++ 4 files changed, 767 insertions(+), 733 deletions(-) create mode 100644 refs/ref-cache.c create mode 100644 refs/ref-cache.h
Makefile +1 -0
@@ -807,6 +807,7 @@ LIB_OBJS += reflog-walk.o LIB_OBJS += refs.o LIB_OBJS += refs/files-backend.o LIB_OBJS += refs/iterator.o +LIB_OBJS += refs/ref-cache.o LIB_OBJS += ref-filter.o LIB_OBJS += remote.o LIB_OBJS += replace_object.o
refs/files-backend.c +3 -733
772 lines not on this page · raw message
refs/ref-cache.c new file +512 -0
512 lines not on this page · raw message
refs/ref-cache.h new file +251 -0
251 lines not on this page · raw message
Signature · 1 line
2.11.0
PATCH v2 10/20 ref-cache: introduce a new type, ref_cache
In reply to Michael HaggertyFor now, it just wraps a ref_entry * that points at the root of the tree. Soon it will hold more information.
Add two new functions, create_ref_cache() and free_ref_cache(). Make free_ref_entry() private.
Change files-backend to use this type to hold its caches.
--- refs/files-backend.c | 28 +++++++++++++++++----------- refs/ref-cache.c | 16 +++++++++++++++- refs/ref-cache.h | 15 ++++++++++++++- 3 files changed, 46 insertions(+), 13 deletions(-)
refs/files-backend.c +17 -11
@@ -44,7 +44,7 @@ static int entry_resolves_to_object(struct ref_entry *entry) }struct packed_ref_cache { -struct ref_entry *root; +struct ref_cache *cache;/* * Count of references to the data structure in this instance,
@@ -79,7 +79,7 @@ struct files_ref_store {char *gitcommondir;char *packed_refs_path; -struct ref_entry *loose; +struct ref_cache *loose;struct packed_ref_cache *packed; };
@@ -101,7 +101,7 @@ static void acquire_packed_ref_cache(struct packed_ref_cache *packed_refs)static int release_packed_ref_cache (struct packed_ref_cache *packed_refs) {if (!--packed_refs->referrers) { -free_ref_entry (packed_refs->root); +free_ref_cache (packed_refs->cache);stat_validity_clear (&packed_refs->validity);free (packed_refs);return 1 ;
@@ -125,7 +125,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)static void clear_loose_ref_cache (struct files_ref_store *refs) {if (refs->loose) { -free_ref_entry (refs->loose); +free_ref_cache (refs->loose); refs->loose =NULL ; } }
@@ -387,11 +387,12 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref refs->packed =xcalloc (1 ,sizeof (*refs->packed));acquire_packed_ref_cache (refs->packed); - refs->packed->root =create_dir_entry (refs,"" ,0 ,0 ); + refs->packed->cache =create_ref_cache (refs); + refs->packed->cache->root->flag &= ~REF_INCOMPLETE ; f =fopen (packed_refs_file,"r" );if (f) {stat_validity_update (&refs->packed->validity,fileno (f)); -read_packed_refs (f,get_ref_dir (refs->packed->root)); +read_packed_refs (f,get_ref_dir (refs->packed->cache->root));fclose (f); } }
@@ -400,7 +401,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refstatic struct ref_dir *get_packed_ref_dir (struct packed_ref_cache *packed_ref_cache) { -return get_ref_dir (packed_ref_cache->root); +return get_ref_dir (packed_ref_cache->cache->root); }static struct ref_dir *get_packed_refs (struct files_ref_store *refs)
@@ -515,14 +516,19 @@ static struct ref_dir *get_loose_refs(struct files_ref_store *refs)* are about to read the only subdirectory that can * hold references: */ - refs->loose =create_dir_entry (refs,"" ,0 ,0 ); + refs->loose =create_ref_cache (refs); + +/* We're going to fill the top level ourselves: */ + refs->loose->root->flag &= ~REF_INCOMPLETE ; +/* -* Create an incomplete entry for "refs/": +* Add an incomplete entry for "refs/" (to be filled +* lazily): */ -add_entry_to_dir (get_ref_dir (refs->loose), +add_entry_to_dir (get_ref_dir (refs->loose->root),create_dir_entry (refs,"refs/" ,5 ,1 )); } -return get_ref_dir (refs->loose); +return get_ref_dir (refs->loose->root); }/*
refs/ref-cache.c +15 -1
@@ -63,9 +63,17 @@ struct ref_entry *create_ref_entry(const char *refname,return ref; } +struct ref_cache *create_ref_cache (struct files_ref_store *refs) +{ +struct ref_cache *ret =xcalloc (1 ,sizeof (*ret)); + + ret->root =create_dir_entry (refs,"" ,0 ,1 ); +return ret; +} +static void clear_ref_dir (struct ref_dir *dir); -void free_ref_entry (struct ref_entry *entry) +static void free_ref_entry (struct ref_entry *entry) {if (entry->flag &REF_DIR ) {/*
@@ -77,6 +85,12 @@ void free_ref_entry(struct ref_entry *entry)free (entry); } +void free_ref_cache (struct ref_cache *cache) +{ +free_ref_entry (cache->root); +free (cache); +} +/* * Clear and free all entries in dir, recursively. */
refs/ref-cache.h +14 -1
@@ -1,6 +1,10 @@#ifndef REFS_REF_CACHE_H #define REFS_REF_CACHE_H +struct ref_cache { +struct ref_entry *root; +}; +/* * Information used (along with the information in ref_entry) to * describe a single cached reference. This data structure only
@@ -165,7 +169,16 @@ struct ref_entry *create_ref_entry(const char *refname, const unsigned char *sha1, int flag, int check_name); -void free_ref_entry (struct ref_entry *entry); +/* +* Return a pointer to a new `ref_cache`. Its top-level starts out +* marked incomplete. +*/ +struct ref_cache *create_ref_cache (struct files_ref_store *refs); + +/* +* Free the `ref_cache` and all of its associated data. +*/ +void free_ref_cache (struct ref_cache *cache);/* * Add a ref_entry to the end of dir (unsorted). Entry is always
Signature · 1 line
2.11.0
Re: [PATCH v2 10/20] ref-cache: introduce a new type, ref_cache
refs/ref-cache.c void free_ref_entry(struct ref_entry *entry) · line 92
Quoted 5 lines · Michael Haggerty
refs/ref-cache.c +5
-0+void free_ref_cache (struct ref_cache *cache) +{ +free_ref_entry (cache->root); +free (cache); +}
free(NULL) is no-op (and safe). Maybe we should follow the same pattern for free_ref_cache(). It's really up to you.
Signature · 1 line
Duy
Re: [PATCH v2 10/20] ref-cache: introduce a new type, ref_cache
Quoted 9 lines · Duy Nguyen
refs/ref-cache.c void free_ref_entry(struct ref_entry *entry) · line 92
Quoted 5 lines · Michael Haggerty
refs/ref-cache.c +5
-0+void free_ref_cache (struct ref_cache *cache) +{ +free_ref_entry (cache->root); +free (cache); +}free(NULL) is no-op (and safe). Maybe we should follow the same pattern for
free_ref_cache(). It's really up to you.
If I look at other free_*() functions in our codebase, it doesn't seem that many of them handle NULL, and that feature wouldn't help the callers of this function. So I think I'll leave it the way that it is.
Michael
PATCH v2 11/20 refs: record the ref_store in ref_cache, not ref_dir
In reply to Michael HaggertyInstead of keeping a pointer to the ref_store in every ref_dir entry, store it once in struct ref_cache, and change struct ref_dir to include a pointer to its containing ref_cache instead. This makes it easier to add to the information that is accessible from a ref_dir without increasing the size of every ref_dir instance.
--- refs/files-backend.c | 6 +++--- refs/ref-cache.c | 12 +++++++----- refs/ref-cache.h | 9 ++++++--- 3 files changed, 16 insertions(+), 11 deletions(-)
refs/files-backend.c +3 -3
@@ -433,7 +433,7 @@ static void add_packed_ref(struct files_ref_store *refs,*/ void read_loose_refs (const char *dirname,struct ref_dir *dir) { -struct files_ref_store *refs = dir->ref_store; +struct files_ref_store *refs = dir->cache->ref_store;DIR *d;struct dirent *de;int dirnamelen =strlen (dirname);
@@ -469,7 +469,7 @@ void read_loose_refs(const char *dirname, struct ref_dir *dir) } else if (S_ISDIR(st.st_mode)) {strbuf_addch (&refname,'/' );add_entry_to_dir (dir, -create_dir_entry (refs, refname.buf, +create_dir_entry (dir->cache, refname.buf, refname.len,1 )); } else { if (!refs_resolve_ref_unsafe(&refs->base,
@@ -526,7 +526,7 @@ static struct ref_dir *get_loose_refs(struct files_ref_store *refs)* lazily): */ add_entry_to_dir (get_ref_dir (refs->loose->root), -create_dir_entry (refs,"refs/" ,5 ,1 )); +create_dir_entry (refs->loose,"refs/" ,5 ,1 )); }return get_ref_dir (refs->loose->root); }
refs/ref-cache.c +7 -5
@@ -36,7 +36,7 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry)int pos =search_ref_dir (dir,"refs/bisect/" ,12 );if (pos <0 ) {struct ref_entry *child_entry; - child_entry =create_dir_entry (dir->ref_store, + child_entry =create_dir_entry (dir->cache,"refs/bisect/" ,12 ,1 );add_entry_to_dir (dir, child_entry);
@@ -67,7 +67,8 @@ struct ref_cache *create_ref_cache(struct files_ref_store *refs) {struct ref_cache *ret =xcalloc (1 ,sizeof (*ret)); - ret->root =create_dir_entry (refs,"" ,0 ,1 ); + ret->ref_store = refs; + ret->root =create_dir_entry (ret,"" ,0 ,1 );return ret; }
@@ -104,13 +105,14 @@ static void clear_ref_dir(struct ref_dir *dir) dir->entries =NULL ; } -struct ref_entry *create_dir_entry (struct files_ref_store *ref_store, +struct ref_entry *create_dir_entry (struct ref_cache *cache,const char *dirname,size_t len,int incomplete) {struct ref_entry *direntry; +FLEX_ALLOC_MEM (direntry, name, dirname, len); - direntry->u.subdir.ref_store = ref_store; + direntry->u.subdir.cache = cache; direntry->flag =REF_DIR | (incomplete ?REF_INCOMPLETE :0 );return direntry; }
@@ -181,7 +183,7 @@ static struct ref_dir *search_for_subdir(struct ref_dir *dir,* therefore, create an empty record for it but mark * the record complete. */ - entry =create_dir_entry (dir->ref_store, subdirname, len,0 ); + entry =create_dir_entry (dir->cache, subdirname, len,0 );add_entry_to_dir (dir, entry); }else { entry = dir->entries[entry_index];
refs/ref-cache.h +6 -3
@@ -3,6 +3,9 @@struct ref_cache {struct ref_entry *root; + +/* A pointer to the files_ref_store whose cache this is: */ +struct files_ref_store *ref_store; };/*
@@ -66,8 +69,8 @@ struct ref_dir {*/ int sorted; -/* A pointer to the files_ref_store that contains this ref_dir. */ -struct files_ref_store *ref_store; +/* The ref_cache containing this entry: */ +struct ref_cache *cache;struct ref_entry **entries; };
@@ -161,7 +164,7 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry);* dirname is the name of the directory with a trailing slash (e.g., * "refs/heads/") or "" for the top-level directory. */ -struct ref_entry *create_dir_entry (struct files_ref_store *ref_store, +struct ref_entry *create_dir_entry (struct ref_cache *cache,const char *dirname,size_t len,int incomplete);
Signature · 1 line
2.11.0
Re: [PATCH v2 11/20] refs: record the ref_store in ref_cache, not ref_dir
Quoted 5 lines · Michael Haggerty
Instead of keeping a pointer to the
ref_storein everyref_direntry, store it once instruct ref_cache, and changestruct ref_dirto include a pointer to its containingref_cacheinstead. This makes it easier to add to the information that is accessible from aref_dirwithout increasing the size of everyref_dirinstance.
...
refs/files-backend.c static struct ref_dir *get_loose_refs(struct files_ref_store *refs) · line 529
Quoted 6 lines
refs/files-backend.c +1
-1@@ -526,7 +526,7 @@ static struct ref_dir *get_loose_refs(struct files_ref_store *refs)* lazily): */ add_entry_to_dir (get_ref_dir (refs->loose->root), -create_dir_entry (refs,"refs/" ,5 ,1 )); +create_dir_entry (refs->loose,"refs/" ,5 ,1 ));
The commit message mentions nothing about this change. Is it intended?
Signature · 1 line
Duy
Re: [PATCH v2 11/20] refs: record the ref_store in ref_cache, not ref_dir
Quoted 7 lines · Duy Nguyen
Quoted 5 lines · Michael Haggerty
Instead of keeping a pointer to the
ref_storein everyref_direntry, store it once instruct ref_cache, and changestruct ref_dirto include a pointer to its containingref_cacheinstead. This makes it easier to add to the information that is accessible from aref_dirwithout increasing the size of everyref_dirinstance....
Quoted 8 lines
refs/files-backend.c static struct ref_dir *get_loose_refs(struct files_ref_store *refs) · line 529
Quoted 6 lines
refs/files-backend.c +1
-1@@ -526,7 +526,7 @@ static struct ref_dir *get_loose_refs(struct files_ref_store *refs)* lazily): */ add_entry_to_dir (get_ref_dir (refs->loose->root), -create_dir_entry (refs,"refs/" ,5 ,1 )); +create_dir_entry (refs->loose,"refs/" ,5 ,1 ));The commit message mentions nothing about this change. Is it intended?
The old create_dir_entry() took a files_ref_store as its first parameter, because that is what needed to be stored into the old dir_entry struct. The new version takes a ref_cache, because that is what the new dir_entry struct requires. This hunk is a logical consequence of that change.
I'll improve the commit message to explain this better.
Thanks,
Michael
PATCH v2 12/20 ref-cache: use a callback function to fill the cache
In reply to Michael HaggertyIt is a leveling violation for ref_cache to know about files_ref_store or that it should call read_loose_refs() to lazily fill cache directories. So instead, have its constructor take as an argument a callback function that it should use for lazy-filling, and change files_ref_store to supply a pointer to function read_loose_refs (renamed to loose_fill_ref_dir) when creating the ref cache for its loose refs.
This means that we can generify the type of the back-pointer in struct ref_cache from files_ref_store to ref_store.
--- refs/files-backend.c | 10 ++++++---- refs/ref-cache.c | 12 +++++++----- refs/ref-cache.h | 29 +++++++++++++++++++++++++---- 3 files changed, 38 insertions(+), 13 deletions(-)
refs/files-backend.c +6 -4
@@ -387,7 +387,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref refs->packed =xcalloc (1 ,sizeof (*refs->packed));acquire_packed_ref_cache (refs->packed); - refs->packed->cache =create_ref_cache (refs); + refs->packed->cache =create_ref_cache (&refs->base,NULL ); refs->packed->cache->root->flag &= ~REF_INCOMPLETE ; f =fopen (packed_refs_file,"r" );if (f) {
@@ -431,9 +431,11 @@ static void add_packed_ref(struct files_ref_store *refs,* (without recursing). dirname must end with '/'. dir must be the * directory entry corresponding to dirname. */ -void read_loose_refs (const char *dirname,struct ref_dir *dir) +static void loose_fill_ref_dir (struct ref_store *ref_store, +struct ref_dir *dir,const char *dirname) { -struct files_ref_store *refs = dir->cache->ref_store; +struct files_ref_store *refs = +files_downcast (ref_store,REF_STORE_READ ,"fill_ref_dir" );DIR *d;struct dirent *de;int dirnamelen =strlen (dirname);
@@ -516,7 +518,7 @@ static struct ref_dir *get_loose_refs(struct files_ref_store *refs)* are about to read the only subdirectory that can * hold references: */ - refs->loose =create_ref_cache (refs); + refs->loose =create_ref_cache (&refs->base, loose_fill_ref_dir);/* We're going to fill the top level ourselves: */ refs->loose->root->flag &= ~REF_INCOMPLETE ;
refs/ref-cache.c +7 -5
@@ -4,9 +4,6 @@#include "ref-cache.h" #include "../iterator.h" -/* FIXME: This declaration shouldn't be here */ -void read_loose_refs (const char *dirname,struct ref_dir *dir); -void add_entry_to_dir (struct ref_dir *dir,struct ref_entry *entry) {ALLOC_GROW (dir->entries, dir->nr +1 , dir->alloc);
@@ -25,7 +22,10 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry)assert (entry->flag &REF_DIR ); dir = &entry->u.subdir;if (entry->flag &REF_INCOMPLETE ) { -read_loose_refs (entry->name, dir); +if (!dir->cache->fill_ref_dir) +die ("BUG: incomplete ref_store without fill_ref_dir function" ); + + dir->cache->fill_ref_dir (dir->cache->ref_store, dir, entry->name);/* * Manually add refs/bisect, which, being
@@ -63,11 +63,13 @@ struct ref_entry *create_ref_entry(const char *refname,return ref; } -struct ref_cache *create_ref_cache (struct files_ref_store *refs) +struct ref_cache *create_ref_cache (struct ref_store *refs, +fill_ref_dir_fn *fill_ref_dir) {struct ref_cache *ret =xcalloc (1 ,sizeof (*ret)); ret->ref_store = refs; + ret->fill_ref_dir = fill_ref_dir; ret->root =create_dir_entry (ret,"" ,0 ,1 );return ret; }
refs/ref-cache.h +25 -4
@@ -1,11 +1,27 @@#ifndef REFS_REF_CACHE_H #define REFS_REF_CACHE_H +struct ref_dir ; + +/* +* If this ref_cache is filled lazily, this function is used to load +* information into the specified ref_dir (shallow or deep, at the +* option of the ref_store). dirname includes a trailing slash. +*/ +typedef void fill_ref_dir_fn (struct ref_store *ref_store, +struct ref_dir *dir,const char *dirname); +struct ref_cache {struct ref_entry *root; -/* A pointer to the files_ref_store whose cache this is: */ -struct files_ref_store *ref_store; +/* A pointer to the ref_store whose cache this is: */ +struct ref_store *ref_store; + +/* +* Function used (if necessary) to lazily-fill cache. May be +* NULL. +*/ +fill_ref_dir_fn *fill_ref_dir; };/*
@@ -174,9 +190,14 @@ struct ref_entry *create_ref_entry(const char *refname,/* * Return a pointer to a new `ref_cache`. Its top-level starts out -* marked incomplete. +* marked incomplete. If `fill_ref_dir` is non-NULL, it is the +* function called to fill in incomplete directories in the +* `ref_cache` when they are accessed. If it is NULL, then the whole +* `ref_cache` must be filled (including clearing its directories' +* `REF_INCOMPLETE` bits) before it is used. */ -struct ref_cache *create_ref_cache (struct files_ref_store *refs); +struct ref_cache *create_ref_cache (struct ref_store *refs, +fill_ref_dir_fn *fill_ref_dir);/* * Free the `ref_cache` and all of its associated data.
Signature · 1 line
2.11.0
PATCH v2 13/20 refs: handle "refs/bisect/" in `loose_fill_ref_dir()`
In reply to Michael HaggertyThat "refs/bisect/" has to be handled specially when filling the ref_cache for loose references is a peculiarity of the files backend, and the ref-cache code shouldn't need to know about it. So move this code to the callback function, loose_fill_ref_dir().
--- refs/files-backend.c | 15 +++++++++++++++ refs/ref-cache.c | 16 ---------------- 2 files changed, 15 insertions(+), 16 deletions(-)
refs/files-backend.c +15 -0
@@ -508,6 +508,21 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,strbuf_release (&refname);strbuf_release (&path);closedir (d); + +/* +* Manually add refs/bisect, which, being per-worktree, might +* not appear in the directory listing for refs/ in the main +* repo. +*/ +if (!strcmp (dirname,"refs/" )) { +int pos =search_ref_dir (dir,"refs/bisect/" ,12 ); + +if (pos <0 ) { +struct ref_entry *child_entry =create_dir_entry ( + dir->cache,"refs/bisect/" ,12 ,1 ); +add_entry_to_dir (dir, child_entry); + } + } }static struct ref_dir *get_loose_refs (struct files_ref_store *refs)
refs/ref-cache.c +0 -16
@@ -26,22 +26,6 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry)die ("BUG: incomplete ref_store without fill_ref_dir function" ); dir->cache->fill_ref_dir (dir->cache->ref_store, dir, entry->name); - -/* -* Manually add refs/bisect, which, being -* per-worktree, might not appear in the directory -* listing for refs/ in the main repo. -*/ -if (!strcmp (entry->name,"refs/" )) { -int pos =search_ref_dir (dir,"refs/bisect/" ,12 ); -if (pos <0 ) { -struct ref_entry *child_entry; - child_entry =create_dir_entry (dir->cache, -"refs/bisect/" , -12 ,1 ); -add_entry_to_dir (dir, child_entry); - } - } entry->flag &= ~REF_INCOMPLETE ; }return dir;
Signature · 1 line
2.11.0
PATCH v2 14/20 do_for_each_entry_in_dir(): eliminate `offset` argument
In reply to Michael HaggertyIt was never used.
--- refs/files-backend.c | 4 ++-- refs/ref-cache.c | 6 +++--- refs/ref-cache.h | 11 +++++------ 3 files changed, 10 insertions(+), 11 deletions(-)
refs/files-backend.c +2 -2
@@ -1390,7 +1390,7 @@ static int commit_packed_refs(struct files_ref_store *refs)fprintf_or_die (out,"%s" ,PACKED_REFS_HEADER );do_for_each_entry_in_dir (get_packed_ref_dir (packed_ref_cache), -0 , write_packed_entry_fn, out); + write_packed_entry_fn, out);if (commit_lock_file (packed_ref_cache->lock)) { save_errno = errno;
@@ -1584,7 +1584,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)lock_packed_refs (refs,LOCK_DIE_ON_ERROR ); cbdata.packed_refs =get_packed_refs (refs); -do_for_each_entry_in_dir (get_loose_refs (refs),0 , +do_for_each_entry_in_dir (get_loose_refs (refs), pack_if_possible_fn, &cbdata);if (commit_packed_refs (refs))
refs/ref-cache.c +3 -3
@@ -307,18 +307,18 @@ static void sort_ref_dir(struct ref_dir *dir) dir->sorted = dir->nr = i; } -int do_for_each_entry_in_dir (struct ref_dir *dir,int offset, +int do_for_each_entry_in_dir (struct ref_dir *dir,each_ref_entry_fn fn,void *cb_data) {int i;assert (dir->sorted == dir->nr); -for (i = offset; i < dir->nr; i++) { +for (i =0 ; i < dir->nr; i++) {struct ref_entry *entry = dir->entries[i];int retval;if (entry->flag &REF_DIR ) {struct ref_dir *subdir =get_ref_dir (entry);sort_ref_dir (subdir); - retval =do_for_each_entry_in_dir (subdir,0 , fn, cb_data); + retval =do_for_each_entry_in_dir (subdir, fn, cb_data); }else { retval =fn (entry, cb_data); }
refs/ref-cache.h +5 -6
@@ -258,13 +258,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_dir *dir);typedef int each_ref_entry_fn (struct ref_entry *entry,void *cb_data);/* -* Call fn for each reference in dir that has index in the range -* offset <= index < dir->nr. Recurse into subdirectories that are in -* that index range, sorting them before iterating. This function -* does not sort dir itself; it should be sorted beforehand. fn is -* called for all references, including broken ones. +* Call `fn` for each reference in `dir`. Recurse into subdirectories, +* sorting them before iterating. This function does not sort `dir` +* itself; it should be sorted beforehand. `fn` is called for all +* references, including broken ones. */ -int do_for_each_entry_in_dir (struct ref_dir *dir,int offset, +int do_for_each_entry_in_dir (struct ref_dir *dir,each_ref_entry_fn fn,void *cb_data);/*
Signature · 1 line
2.11.0
PATCH v2 15/20 get_loose_ref_dir(): function renamed from get_loose_refs()
In reply to Michael HaggertyThe new name is more analogous to get_packed_ref_dir().
--- refs/files-backend.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
refs/files-backend.c +3 -3
@@ -525,7 +525,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store, } } -static struct ref_dir *get_loose_refs (struct files_ref_store *refs) +static struct ref_dir *get_loose_ref_dir (struct files_ref_store *refs) {if (!refs->loose) {/*
@@ -1113,7 +1113,7 @@ static struct ref_iterator *files_ref_iterator_begin(* date with what is on disk, and re-reads it if not. */ - loose_dir =get_loose_refs (refs); + loose_dir =get_loose_ref_dir (refs);if (prefix && *prefix) loose_dir =find_containing_dir (loose_dir, prefix,0 );
@@ -1584,7 +1584,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)lock_packed_refs (refs,LOCK_DIE_ON_ERROR ); cbdata.packed_refs =get_packed_refs (refs); -do_for_each_entry_in_dir (get_loose_refs (refs), +do_for_each_entry_in_dir (get_loose_ref_dir (refs), pack_if_possible_fn, &cbdata);if (commit_packed_refs (refs))
Signature · 1 line
2.11.0
PATCH v2 16/20 get_loose_ref_cache(): new function
In reply to Michael HaggertyExtract a new function, get_loose_ref_cache(), from get_loose_ref_dir(). The function returns the ref_cache for the loose refs of a files_ref_store.
--- refs/files-backend.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
refs/files-backend.c +7 -2
@@ -525,7 +525,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store, } } -static struct ref_dir *get_loose_ref_dir (struct files_ref_store *refs) +static struct ref_cache *get_loose_ref_cache (struct files_ref_store *refs) {if (!refs->loose) {/*
@@ -545,7 +545,12 @@ static struct ref_dir *get_loose_ref_dir(struct files_ref_store *refs)add_entry_to_dir (get_ref_dir (refs->loose->root),create_dir_entry (refs->loose,"refs/" ,5 ,1 )); } -return get_ref_dir (refs->loose->root); +return refs->loose; +} + +static struct ref_dir *get_loose_ref_dir (struct files_ref_store *refs) +{ +return get_ref_dir (get_loose_ref_cache (refs)->root); }/*
Signature · 1 line
2.11.0
PATCH v2 17/20 cache_ref_iterator_begin(): make function smarter
In reply to Michael HaggertyChange cache_ref_iterator_begin() to take two new arguments:
* prefix -- to iterate only over references with the specified prefix.
* prime_dir -- to "prime" (i.e., pre-load) the cache before starting the iteration.
The new functionality makes it possible for files_ref_iterator_begin() to be made more ignorant of the internals of ref_cache, and find_containing_dir() and prime_ref_dir() to be made private.
--- refs/files-backend.c | 44 +++++++++++++------------------------------- refs/ref-cache.c | 38 ++++++++++++++++++++++++++++++++++---- refs/ref-cache.h | 27 +++++++++------------------ 3 files changed, 56 insertions(+), 53 deletions(-)
refs/files-backend.c +13 -31
@@ -1083,7 +1083,6 @@ static struct ref_iterator *files_ref_iterator_begin( const char *prefix, unsigned int flags) {struct files_ref_store *refs; -struct ref_dir *loose_dir, *packed_dir;struct ref_iterator *loose_iter, *packed_iter;struct files_ref_iterator *iter;struct ref_iterator *ref_iterator;
@@ -1109,41 +1108,24 @@ static struct ref_iterator *files_ref_iterator_begin(* condition if loose refs are migrated to the packed-refs * file by a simultaneous process, but our in-memory view is * from before the migration. We ensure this as follows: -* First, we call prime_ref_dir(), which pre-reads the loose -* references for the subtree into the cache. (If they've -* already been read, that's OK; we only need to guarantee -* that they're read before the packed refs, not *how much* -* before.) After that, we call get_packed_ref_cache(), which -* internally checks whether the packed-ref cache is up to -* date with what is on disk, and re-reads it if not. +* First, we call start the loose refs iteration with its +* `prime_ref` argument set to true. This causes the loose +* references in the subtree to be pre-read into the cache. +* (If they've already been read, that's OK; we only need to +* guarantee that they're read before the packed refs, not +* *how much* before.) After that, we call +* get_packed_ref_cache(), which internally checks whether the +* packed-ref cache is up to date with what is on disk, and +* re-reads it if not. */ - loose_dir =get_loose_ref_dir (refs); - -if (prefix && *prefix) - loose_dir =find_containing_dir (loose_dir, prefix,0 ); - -if (loose_dir) { -prime_ref_dir (loose_dir); - loose_iter =cache_ref_iterator_begin (loose_dir); - }else { -/* There's nothing to iterate over. */ - loose_iter =empty_ref_iterator_begin (); - } + loose_iter =cache_ref_iterator_begin (get_loose_ref_cache (refs), + prefix,1 ); iter->packed_ref_cache =get_packed_ref_cache (refs);acquire_packed_ref_cache (iter->packed_ref_cache); - packed_dir =get_packed_ref_dir (iter->packed_ref_cache); - -if (prefix && *prefix) - packed_dir =find_containing_dir (packed_dir, prefix,0 ); - -if (packed_dir) { - packed_iter =cache_ref_iterator_begin (packed_dir); - }else { -/* There's nothing to iterate over. */ - packed_iter =empty_ref_iterator_begin (); - } + packed_iter =cache_ref_iterator_begin (iter->packed_ref_cache->cache, + prefix,0 ); iter->iter0 =overlay_ref_iterator_begin (loose_iter, packed_iter); iter->flags = flags;
refs/ref-cache.c +34 -4
@@ -177,8 +177,17 @@ static struct ref_dir *search_for_subdir(struct ref_dir *dir,return get_ref_dir (entry); } -struct ref_dir *find_containing_dir (struct ref_dir *dir, -const char *refname,int mkdir) +/* +* If refname is a reference name, find the ref_dir within the dir +* tree that should hold refname. If refname is a directory name +* (i.e., it ends in '/'), then return that ref_dir itself. dir must +* represent the top-level directory and must already be complete. +* Sort ref_dirs and recurse into subdirectories as necessary. If +* mkdir is set, then create any missing directories; otherwise, +* return NULL if the desired directory cannot be found. +*/ +static struct ref_dir *find_containing_dir (struct ref_dir *dir, +const char *refname,int mkdir) {const char *slash;for (slash =strchr (refname,'/' ); slash; slash =strchr (slash +1 ,'/' )) {
@@ -328,7 +337,11 @@ int do_for_each_entry_in_dir(struct ref_dir *dir,return 0 ; } -void prime_ref_dir (struct ref_dir *dir) +/* +* Load all of the refs from `dir` (recursively) into our in-memory +* cache. +*/ +static void prime_ref_dir (struct ref_dir *dir) {/* * The hard work of loading loose refs is done by get_ref_dir(), so we
@@ -494,12 +507,25 @@ static struct ref_iterator_vtable cache_ref_iterator_vtable = { cache_ref_iterator_abort }; -struct ref_iterator *cache_ref_iterator_begin (struct ref_dir *dir) +struct ref_iterator *cache_ref_iterator_begin (struct ref_cache *cache, +const char *prefix, +int prime_dir) { +struct ref_dir *dir;struct cache_ref_iterator *iter;struct ref_iterator *ref_iterator;struct cache_ref_iterator_level *level; + dir =get_ref_dir (cache->root); +if (prefix && *prefix) + dir =find_containing_dir (dir, prefix,0 ); +if (!dir) +/* There's nothing to iterate over. */ +return empty_ref_iterator_begin (); + +if (prime_dir) +prime_ref_dir (dir); + iter =xcalloc (1 ,sizeof (*iter)); ref_iterator = &iter->base;base_ref_iterator_init (ref_iterator, &cache_ref_iterator_vtable);
@@ -510,5 +536,9 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_dir *dir) level->index =-1 ; level->dir = dir; +if (prefix && *prefix) + ref_iterator =prefix_ref_iterator_begin (ref_iterator, + prefix,0 ); +return ref_iterator; }
refs/ref-cache.h +9 -18
@@ -234,18 +234,6 @@ int remove_entry_from_dir(struct ref_dir *dir, const char *refname);*/ int add_ref_entry (struct ref_dir *dir,struct ref_entry *ref); -/* -* If refname is a reference name, find the ref_dir within the dir -* tree that should hold refname. If refname is a directory name -* (i.e., it ends in '/'), then return that ref_dir itself. dir must -* represent the top-level directory and must already be complete. -* Sort ref_dirs and recurse into subdirectories as necessary. If -* mkdir is set, then create any missing directories; otherwise, -* return NULL if the desired directory cannot be found. -*/ -struct ref_dir *find_containing_dir (struct ref_dir *dir, -const char *refname,int mkdir); -/* * Find the value entry with the given name in dir, sorting ref_dirs * and recursing into subdirectories as necessary. If the name is not
@@ -253,7 +241,15 @@ struct ref_dir *find_containing_dir(struct ref_dir *dir,*/ struct ref_entry *find_ref_entry (struct ref_dir *dir,const char *refname); -struct ref_iterator *cache_ref_iterator_begin (struct ref_dir *dir); +/* +* Start iterating over references in `cache`. If `prefix` is +* specified, only include references whose names start with that +* prefix. If `prime_dir` is true, then fill any incomplete +* directories before beginning the iteration. +*/ +struct ref_iterator *cache_ref_iterator_begin (struct ref_cache *cache, +const char *prefix, +int prime_dir);typedef int each_ref_entry_fn (struct ref_entry *entry,void *cb_data);
@@ -279,9 +275,4 @@ int do_for_each_entry_in_dir(struct ref_dir *dir,*/ enum peel_status peel_entry (struct ref_entry *entry,int repeel); -/* -* Load all of the refs from `dir` into our in-memory cache. -*/ -void prime_ref_dir (struct ref_dir *dir); -#endif /* REFS_REF_CACHE_H */
Signature · 1 line
2.11.0
PATCH v2 18/20 commit_packed_refs(): use reference iteration
In reply to Michael HaggertyUse reference iteration rather than do_for_each_entry_in_dir() in the definition of commit_packed_refs().
Note that an internal consistency check that was previously done in write_packed_entry_fn() is not there anymore. This is actually an improvement:
The old error message was emitted when there is an entry in the packed-ref cache that is not REF_KNOWS_PEELED, and when we attempted to peel the reference, the result was PEEL_INVALID, PEEL_IS_SYMREF, or PEEL_BROKEN. Since a packed ref cannot be a symref, PEEL_IS_SYMREF and PEEL_BROKEN can be ruled out. So we're left with PEEL_INVALID.
An entry without REF_KNOWS_PEELED can get into the packed-refs cache in the following two ways:
* The reference was read from a packed-refs file that didn't have the fully-peeled attribute. In that case, we *don't want* to emit an error, because the broken value is presumably a stale value of the reference that is now masked by a loose version of the same reference (which we just don't happen to be packing this time). This is a perfectly legitimate situation and doesn't indicate that the repository is corrupt. The old code incorrectly emits an error message in this case. (It was probably never reported as a bug because this scenario is rare.)
* The reference was a loose reference that was just added to the packed ref cache by files_packed_refs() via pack_if_possible_fn() in preparation for being packed. The latter function refuses to pack a reference for which entry_resolves_to_object() returns false, and otherwise calls peel_entry() itself and checks the return value. So an entry added this way should always have REF_KNOWS_PEELED and shouldn't trigger the error message in either the old code or the new.
--- refs/files-backend.c | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-)
refs/files-backend.c +17 -21
@@ -1293,30 +1293,15 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,* Write an entry to the packed-refs file for the specified refname. * If peeled is non-NULL, write it as the entry's peeled value. */ -static void write_packed_entry (FILE *fh,char *refname,unsigned char *sha1, -unsigned char *peeled) +static void write_packed_entry (FILE *fh,const char *refname, +const unsigned char *sha1, +const unsigned char *peeled) {fprintf_or_die (fh,"%s %s\n" ,sha1_to_hex (sha1), refname);if (peeled)fprintf_or_die (fh,"^%s\n" ,sha1_to_hex (peeled)); } -/* -* An each_ref_entry_fn that writes the entry to a packed-refs file. -*/ -static int write_packed_entry_fn (struct ref_entry *entry,void *cb_data) -{ -enum peel_status peel_status =peel_entry (entry,0 ); - -if (peel_status !=PEEL_PEELED && peel_status !=PEEL_NON_TAG ) -error ("internal error: %s is not a valid packed reference!" , - entry->name); -write_packed_entry (cb_data, entry->name, entry->u.value.oid.hash, - peel_status ==PEEL_PEELED ? - entry->u.value.peeled.hash :NULL ); -return 0 ; -} -/* * Lock the packed-refs file for writing. Flags is passed to * hold_lock_file_for_update(). Return 0 on success. On errors, set
@@ -1362,9 +1347,10 @@ static int commit_packed_refs(struct files_ref_store *refs) {struct packed_ref_cache *packed_ref_cache =get_packed_ref_cache (refs); -int error =0 ; +int ok, error =0 ;int save_errno =0 ;FILE *out; +struct ref_iterator *iter;files_assert_main_repository (refs,"commit_packed_refs" );
@@ -1376,8 +1362,18 @@ static int commit_packed_refs(struct files_ref_store *refs)die_errno ("unable to fdopen packed-refs descriptor" );fprintf_or_die (out,"%s" ,PACKED_REFS_HEADER ); -do_for_each_entry_in_dir (get_packed_ref_dir (packed_ref_cache), - write_packed_entry_fn, out); + + iter =cache_ref_iterator_begin (packed_ref_cache->cache,NULL ,0 ); +while ((ok =ref_iterator_advance (iter)) ==ITER_OK ) { +struct object_id peeled; +int peel_error =ref_iterator_peel (iter, &peeled); + +write_packed_entry (out, iter->refname, iter->oid->hash, + peel_error ?NULL : peeled.hash); + } + +if (ok !=ITER_DONE ) +die ("error while iterating over references" );if (commit_lock_file (packed_ref_cache->lock)) { save_errno = errno;
Signature · 1 line
2.11.0
PATCH v2 19/20 files_pack_refs(): use reference iteration
In reply to Michael HaggertyUse reference iteration rather than do_for_each_entry_in_dir() in the definition of files_pack_refs().
--- refs/files-backend.c | 143 +++++++++++++++++++++------------------------------ 1 file changed, 60 insertions(+), 83 deletions(-)
refs/files-backend.c +60 -83
@@ -32,17 +32,6 @@ static int ref_resolves_to_object(const char *refname,return 1 ; } -/* -* Return true if the reference described by entry can be resolved to -* an object in the database; otherwise, emit a warning and return -* false. -*/ -static int entry_resolves_to_object (struct ref_entry *entry) -{ -return ref_resolves_to_object (entry->name, - &entry->u.value.oid, entry->flag); -} -struct packed_ref_cache {struct ref_cache *cache;
@@ -548,11 +537,6 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)return refs->loose; } -static struct ref_dir *get_loose_ref_dir (struct files_ref_store *refs) -{ -return get_ref_dir (get_loose_ref_cache (refs)->root); -} -/* * Return the ref_entry for the given refname from the packed * references. If it does not exist, return NULL.
@@ -1411,65 +1395,6 @@ struct ref_to_prune {char name[FLEX_ARRAY ]; }; -struct pack_refs_cb_data { -unsigned int flags; -struct ref_dir *packed_refs; -struct ref_to_prune *ref_to_prune; -}; - -/* -* An each_ref_entry_fn that is run over loose references only. If -* the loose reference can be packed, add an entry in the packed ref -* cache. If the reference should be pruned, also add it to -* ref_to_prune in the pack_refs_cb_data. -*/ -static int pack_if_possible_fn (struct ref_entry *entry,void *cb_data) -{ -struct pack_refs_cb_data *cb = cb_data; -enum peel_status peel_status; -struct ref_entry *packed_entry; -int is_tag_ref =starts_with (entry->name,"refs/tags/" ); - -/* Do not pack per-worktree refs: */ -if (ref_type (entry->name) !=REF_TYPE_NORMAL ) -return 0 ; - -/* ALWAYS pack tags */ -if (!(cb->flags &PACK_REFS_ALL ) && !is_tag_ref) -return 0 ; - -/* Do not pack symbolic or broken refs: */ -if ((entry->flag &REF_ISSYMREF ) || !entry_resolves_to_object (entry)) -return 0 ; - -/* Add a packed ref cache entry equivalent to the loose entry. */ - peel_status =peel_entry (entry,1 ); -if (peel_status !=PEEL_PEELED && peel_status !=PEEL_NON_TAG ) -die ("internal error peeling reference %s (%s)" , - entry->name,oid_to_hex (&entry->u.value.oid)); - packed_entry =find_ref_entry (cb->packed_refs, entry->name); -if (packed_entry) { -/* Overwrite existing packed entry with info from loose entry */ - packed_entry->flag =REF_ISPACKED |REF_KNOWS_PEELED ; -oidcpy (&packed_entry->u.value.oid, &entry->u.value.oid); - }else { - packed_entry =create_ref_entry (entry->name, entry->u.value.oid.hash, -REF_ISPACKED |REF_KNOWS_PEELED ,0 ); -add_ref_entry (cb->packed_refs, packed_entry); - } -oidcpy (&packed_entry->u.value.peeled, &entry->u.value.peeled); - -/* Schedule the loose reference for pruning if requested. */ -if ((cb->flags &PACK_REFS_PRUNE )) { -struct ref_to_prune *n; -FLEX_ALLOC_STR (n, name, entry->name); -hashcpy (n->sha1, entry->u.value.oid.hash); - n->next = cb->ref_to_prune; - cb->ref_to_prune = n; - } -return 0 ; -} -enum {REMOVE_EMPTY_PARENTS_REF =0x01 ,REMOVE_EMPTY_PARENTS_REFLOG =0x02
@@ -1559,21 +1484,73 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)struct files_ref_store *refs =files_downcast (ref_store,REF_STORE_WRITE |REF_STORE_ODB ,"pack_refs" ); -struct pack_refs_cb_data cbdata; - -memset (&cbdata,0 ,sizeof (cbdata)); - cbdata.flags = flags; +struct ref_iterator *iter; +struct ref_dir *packed_refs; +int ok; +struct ref_to_prune *refs_to_prune =NULL ;lock_packed_refs (refs,LOCK_DIE_ON_ERROR ); - cbdata.packed_refs =get_packed_refs (refs); + packed_refs =get_packed_refs (refs); + + iter =cache_ref_iterator_begin (get_loose_ref_cache (refs),NULL ,0 ); +while ((ok =ref_iterator_advance (iter)) ==ITER_OK ) { +/* +* If the loose reference can be packed, add an entry +* in the packed ref cache. If the reference should be +* pruned, also add it to refs_to_prune. +*/ +struct ref_entry *packed_entry; +int is_tag_ref =starts_with (iter->refname,"refs/tags/" ); + +/* Do not pack per-worktree refs: */ +if (ref_type (iter->refname) !=REF_TYPE_NORMAL ) +continue ; + +/* ALWAYS pack tags */ +if (!(flags &PACK_REFS_ALL ) && !is_tag_ref) +continue ; + +/* Do not pack symbolic or broken refs: */ +if (iter->flags &REF_ISSYMREF ) +continue ; -do_for_each_entry_in_dir (get_loose_ref_dir (refs), - pack_if_possible_fn, &cbdata); +if (!ref_resolves_to_object (iter->refname, iter->oid, iter->flags)) +continue ; + +/* +* Create an entry in the packed-refs cache equivalent +* to the one from the loose ref cache, except that +* we don't copy the peeled status, because we want it +* to be re-peeled. +*/ + packed_entry =find_ref_entry (packed_refs, iter->refname); +if (packed_entry) { +/* Overwrite existing packed entry with info from loose entry */ + packed_entry->flag =REF_ISPACKED ; +oidcpy (&packed_entry->u.value.oid, iter->oid); + }else { + packed_entry =create_ref_entry (iter->refname, iter->oid->hash, +REF_ISPACKED ,0 ); +add_ref_entry (packed_refs, packed_entry); + } +oidclr (&packed_entry->u.value.peeled); + +/* Schedule the loose reference for pruning if requested. */ +if ((flags &PACK_REFS_PRUNE )) { +struct ref_to_prune *n; +FLEX_ALLOC_STR (n, name, iter->refname); +hashcpy (n->sha1, iter->oid->hash); + n->next = refs_to_prune; + refs_to_prune = n; + } + } +if (ok !=ITER_DONE ) +die ("error while iterating over references" );if (commit_packed_refs (refs))die_errno ("unable to overwrite old ref-pack file" ); -prune_refs (refs, cbdata.ref_to_prune); +prune_refs (refs, refs_to_prune);return 0 ; }
Signature · 1 line
2.11.0
Re: [PATCH v2 19/20] files_pack_refs(): use reference iteration
Quoted 2 lines · Michael Haggerty
Use reference iteration rather than
do_for_each_entry_in_dir()in the definition offiles_pack_refs().
A "why" is missing here. My guess is readability/maintainability because it's easier to follow the code with iterator design, than the callback one? No maintaining data in struct pack_refs_cb_data is also very nice.
Signature · 1 line
Duy
Re: [PATCH v2 19/20] files_pack_refs(): use reference iteration
Quoted 8 lines · Duy Nguyen
Quoted 2 lines · Michael Haggerty
Use reference iteration rather than
do_for_each_entry_in_dir()in the definition offiles_pack_refs().A "why" is missing here. My guess is readability/maintainability because it's easier to follow the code with iterator design, than the callback one? No maintaining data in struct
pack_refs_cb_datais also very nice.
You're right. I'll improve the commit message to something like
files_pack_refs(): use reference iteration
Use reference iteration rather than do_for_each_entry_in_dir() in the definition of files_pack_refs(). This makes the code shorter and easier to follow, because the logic can be inline rather than spread between the main function and a callback function, and it removes the need to use pack_refs_cb_data to preserve intermediate state.
This removes the last callers of entry_resolves_to_object() and get_loose_ref_dir(), so delete those functions.
Thanks,
Michael
PATCH v2 20/20 do_for_each_entry_in_dir(): delete function
In reply to Michael HaggertyIts only remaining caller was itself.
--- refs/ref-cache.c | 21 --------------------- refs/ref-cache.h | 11 ----------- 2 files changed, 32 deletions(-)
refs/ref-cache.c +0 -21
@@ -316,27 +316,6 @@ static void sort_ref_dir(struct ref_dir *dir) dir->sorted = dir->nr = i; } -int do_for_each_entry_in_dir (struct ref_dir *dir, -each_ref_entry_fn fn,void *cb_data) -{ -int i; -assert (dir->sorted == dir->nr); -for (i =0 ; i < dir->nr; i++) { -struct ref_entry *entry = dir->entries[i]; -int retval; -if (entry->flag &REF_DIR ) { -struct ref_dir *subdir =get_ref_dir (entry); -sort_ref_dir (subdir); - retval =do_for_each_entry_in_dir (subdir, fn, cb_data); - }else { - retval =fn (entry, cb_data); - } -if (retval) -return retval; - } -return 0 ; -} -/* * Load all of the refs from `dir` (recursively) into our in-memory * cache.
refs/ref-cache.h +0 -11
@@ -251,17 +251,6 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,const char *prefix, int prime_dir); -typedef int each_ref_entry_fn (struct ref_entry *entry,void *cb_data); - -/* -* Call `fn` for each reference in `dir`. Recurse into subdirectories, -* sorting them before iterating. This function does not sort `dir` -* itself; it should be sorted beforehand. `fn` is called for all -* references, including broken ones. -*/ -int do_for_each_entry_in_dir (struct ref_dir *dir, -each_ref_entry_fn fn,void *cb_data); -/* * Peel the entry (if possible) and return its new peel_status. If * repeel is true, re-peel the entry even if there is an old peeled
Signature · 1 line
2.11.0
Quoted 5 lines · Michael Haggerty
This version literally only contains a few commit message changes and one minor comment changes relative to v1. The code is identical. I wasn't sure whether it is even worth sending this patch series to the ML again; Junio, if you'd prefer I just send a link to a published branch in such cases, please let me know.
The review on the list is not about letting me pick it up, but is about giving reviewing contributors a chance to comment. I think for a series this important ;-) it is good that you are giving it multiple exposures so that people who were offline the last time can have a chance to look at it, even if the update is minimum.
Quoted 3 lines
This patch series is also available from my GitHub fork [2] as branch "separate-ref-cache". These patches depend on Duy's nd/files-backend-git-dir branch.
I am getting the impression that the files-backend thing as well as this topic are ready for 'next'. Please stop me if I missed something in these topics (especially the other one) that needs updating before that happens.
Thanks.
Quoted 14 lines · Junio C Hamano
Quoted 5 lines · Michael Haggerty
This version literally only contains a few commit message changes and one minor comment changes relative to v1. The code is identical. I wasn't sure whether it is even worth sending this patch series to the ML again; Junio, if you'd prefer I just send a link to a published branch in such cases, please let me know.
The review on the list is not about letting me pick it up, but is about giving reviewing contributors a chance to comment. I think for a series this important ;-) it is good that you are giving it multiple exposures so that people who were offline the last time can have a chance to look at it, even if the update is minimum.
Quoted 8 lines
Quoted 3 lines
This patch series is also available from my GitHub fork [2] as branch "separate-ref-cache". These patches depend on Duy's nd/files-backend-git-dir branch.
I am getting the impression that the files-backend thing as well as this topic are ready for 'next'. Please stop me if I missed something in these topics (especially the other one) that needs updating before that happens.
I don't know of any remaining problems with these two patch series (aside from a couple of cosmetic problems that I just pointed out about v7 of nd/files-backend-git-dir). I'm pretty confident about both of them.
Duy, have you looked over my patch series? Since you've been working in the area, your feedback would be very welcome, if you have the time for it.
Michael
Quoted 2 lines · Michael Haggerty
Duy, have you looked over my patch series? Since you've been working in the area, your feedback would be very welcome, if you have the time for it.
You probably have guessed my answer based on my lack of response :) Tomorrow is holiday though, will have a look. But I doubt I would find anything given that both you and Jeff are involved in this.
Quoted 2 lines · Ramsay Jones
I sent Duy some comments on that branch (an unused function and a 'plain integer used as NULL pointer' warning).
I still remember :) Or at least I vaguely remember something I need to fix from you, and made sure comments in all my re-rolls are addressed. nd/prune-in-worktree should be the next one, soon.
Signature · 1 line
Duy
Quoted 7 lines · Duy Nguyen
Quoted 2 lines · Michael Haggerty
Duy, have you looked over my patch series? Since you've been working in the area, your feedback would be very welcome, if you have the time for it.
You probably have guessed my answer based on my lack of response :) Tomorrow is holiday though, will have a look. But I doubt I would find anything given that both you and Jeff are involved in this.
Overall, very nice. I made a comment here and there, but they're more questions for my education than actual code review ) At least there's another set of eyes on this series now.
Signature · 1 line
Duy
Quoted 5 lines · Duy Nguyen
Quoted 4 lines · Duy Nguyen
Quoted 2 lines · Michael Haggerty
Duy, have you looked over my patch series? Since you've been working in the area, your feedback would be very welcome, if you have the time for it.
Quoted 7 lines
Quoted 3 lines
You probably have guessed my answer based on my lack of response :) Tomorrow is holiday though, will have a look. But I doubt I would find anything given that both you and Jeff are involved in this.
Overall, very nice. I made a comment here and there, but they're more questions for my education than actual code review ) At least there's another set of eyes on this series now.
Thanks for your review! I'll submit v3 shortly to address your feedback.
Michael
Quoted 14 lines · Junio C Hamano
Quoted 5 lines · Michael Haggerty
This version literally only contains a few commit message changes and one minor comment changes relative to v1. The code is identical. I wasn't sure whether it is even worth sending this patch series to the ML again; Junio, if you'd prefer I just send a link to a published branch in such cases, please let me know.
The review on the list is not about letting me pick it up, but is about giving reviewing contributors a chance to comment. I think for a series this important ;-) it is good that you are giving it multiple exposures so that people who were offline the last time can have a chance to look at it, even if the update is minimum.
Quoted 8 lines
Quoted 3 lines
This patch series is also available from my GitHub fork [2] as branch "separate-ref-cache". These patches depend on Duy's nd/files-backend-git-dir branch.
I am getting the impression that the files-backend thing as well as this topic are ready for 'next'. Please stop me if I missed something in these topics (especially the other one) that needs updating before that happens.
Hmm, are these branches 'tangled' with nd/prune-in-worktree? (I think they were at one point, but maybe not now?).
I sent Duy some comments on that branch (an unused function and a 'plain integer used as NULL pointer' warning).
ATB,
Ramsay Jones
Quoted 7 lines · Ramsay Jones
Quoted 4 lines
I am getting the impression that the files-backend thing as well as this topic are ready for 'next'. Please stop me if I missed something in these topics (especially the other one) that needs updating before that happens.
Hmm, are these branches 'tangled' with nd/prune-in-worktree? (I think they were at one point, but maybe not now?).
Michael's mh/separate-ref-cache builds directly on top of nd/files-backend-git-dir topic.
nd/prune-in-worktree builds directly on top of nd/worktree-kill-parse-ref, which in turn builds directly on top of nd/files-backend-git-dir.
In that sense, Michael's series and Duy's later two series are "tangled" (i.e. shares some common commits that are still not in 'master'). If nd/files-backend-git-dir that is shared among them is ever rebased, all of them need to be rebased on top of it consistently.
But if either of nd/prune-in-worktree and nd/worktree-kill-parse-ref needs to be rerolled, that can be done independently from Michael's series, as long as nd/files-backend-git-dir is solid and unchanging.
Quoted 23 lines · Junio C Hamano
Quoted 7 lines · Ramsay Jones
Quoted 4 lines
I am getting the impression that the files-backend thing as well as this topic are ready for 'next'. Please stop me if I missed something in these topics (especially the other one) that needs updating before that happens.
Hmm, are these branches 'tangled' with nd/prune-in-worktree? (I think they were at one point, but maybe not now?).
Michael's mh/separate-ref-cache builds directly on top of nd/files-backend-git-dir topic.
nd/prune-in-worktree builds directly on top of nd/worktree-kill-parse-ref, which in turn builds directly on top of nd/files-backend-git-dir.
In that sense, Michael's series and Duy's later two series are "tangled" (i.e. shares some common commits that are still not in 'master'). If nd/files-backend-git-dir that is shared among them is ever rebased, all of them need to be rebased on top of it consistently.
Quoted 3 lines
But if either of nd/prune-in-worktree and nd/worktree-kill-parse-ref needs to be rerolled, that can be done independently from Michael's series, as long as nd/files-backend-git-dir is solid and unchanging.
I suspected as much (hence the 'maybe not now' comment), but I noticed that all four branches were merged into 'jch'. So, it seemed possible to me that they were all being considered for merging into next.
Thanks!
ATB,
Ramsay Jones
Quoted 6 lines · Ramsay Jones
Quoted 6 lines
In that sense, Michael's series and Duy's later two series are "tangled" (i.e. shares some common commits that are still not in 'master'). If nd/files-backend-git-dir that is shared among them is ever rebased, all of them need to be rebased on top of it consistently.
Quoted 7 lines
Quoted 3 lines
But if either of nd/prune-in-worktree and nd/worktree-kill-parse-ref needs to be rerolled, that can be done independently from Michael's series, as long as nd/files-backend-git-dir is solid and unchanging.
I suspected as much (hence the 'maybe not now' comment), but I noticed that all four branches were merged into 'jch'. So, it seemed possible to me that they were all being considered for merging into next.
Yes they were (and still are, but I do not expect they will be moving to 'next' while I am offline ;-). What you can do to help is to review and say things like "nd/files-backend-git-dir and Michael's one on top of it looked good, no opinion on others", "the others have this and that issues that need a reroll", etc.
Thanks.
Quoted 4 lines · Michael Haggerty
* I added a long blurb about the removal of an internal consistency check in the commit message for
[18/20]
commit_packed_refs(): use reference iteration
Thanks, the explanation there makes sense.
I think this version addresses all of my comments.
-Peff