Prepare to separate out a packed_ref_store
v2 cover Prepare to separate out a packed_ref_store
This is the second iteration of a patch series that prepares the ground for separating out a packed_ref_store and then for changing packed-refs to be read using mmap(). Thanks to Peff, Junio, Stefan, Brandon, and Johannes for their feedback about v1 [1]. I think I have addressed all of your comments.
Changes since v1:
* Since v1, branch bc/object-id has been merged to next, and it has lots of conflicts with these changes. So I rebased this branch onto a merge of master and bc/object-id. (I hope this makes Junio's job easier.) This unfortunately causes a bit of tbdiff noise between v1 and v2.
* Patch [01/25]: in t3600, register the test_when_finished command before executing chmod a-w.
* Patch [04/25] (new patch): convert a few die("internal error: ...") to die("BUG: ...").
* Patch [05/25]: Use strlen() rather than memchr() to check the trim length, and die() rather than skipping if it is longer than the reference name.
* Patch [08/25]: Name the log message arguments msg for consistency with existing code.
* Patch [10/25]: Rename the new member from packlock to packed_refs_lock.
* Patch [13/25] (new patch): Move the check for valid transaction->state from files_transaction_commit() to ref_transaction_commit().
* Patch [14/25]:
* Add more sanity checks of transaction->state.
* Don't add ref_transaction_finish() to the public API. Instead, teach ref_transaction_commit() to do the right thing whether or not ref_transaction_prepare() has been called.
* Add and improve docstrings.
* Allow ref_transaction_abort() to be called even before ref_transaction_prepare() (in which case it just calls ref_transaction_free()).
* Lots of improvements to commit messages and comments, mostly to clarify points that reviewers asked about.
These changes (along with the merge commit that they are based on) are also available as branch packed-ref-store-prep in my GitHub fork [2]. If you'd like to see a preview of the rest of the changes (which works but is not yet polished), checkout the mmap-packed-refs branch from the same place.
Michael
[1] http://public-inbox.org/git/[email protected]/
[2] https://github.com/mhagger/git
Jeff King (1):
ref-filter: limit traversal to prefix
Michael Haggerty (24):
t3600: clean up permissions test properly
refs.h: clarify docstring for the ref_transaction_update()-related fns ref_iterator_begin_fn(): fix docstring
files-backend: use die("BUG: ..."), not die("internal error: ...")
prefix_ref_iterator: don't trim too much refs_ref_iterator_begin(): don't check prefixes redundantly
refs: use size_t indexes when iterating over ref transaction updates
ref_store: take a msg parameter when deleting references
lockfile: add a new method, is_lock_file_locked()
files-backend: move lock member to files_ref_store
files_ref_store: put the packed files lock directly in this struct files_transaction_cleanup(): new helper function ref_transaction_commit(): check for valid transaction->state ref_transaction_prepare(): new optional step for reference updates ref_update_reject_duplicates(): expose function to whole refs module ref_update_reject_duplicates(): use size_t rather than int ref_update_reject_duplicates(): add a sanity check should_pack_ref(): new function, extracted from files_pack_refs() get_packed_ref_cache(): assume "packed-refs" won't change while locked read_packed_refs(): do more of the work of reading packed refs read_packed_refs(): report unexpected fopen() failures refs_ref_iterator_begin(): handle GIT_REF_PARANOIA create_ref_entry(): remove check_name option cache_ref_iterator_begin(): avoid priming unneeded directories
builtin/fetch.c | 2 +- builtin/remote.c | 4 +- lockfile.h | 8 ++ ref-filter.c | 64 ++++++++- refs.c | 135 ++++++++++++++++-- refs.h | 149 +++++++++++++++----- refs/files-backend.c | 302 +++++++++++++++++++++++++---------------- refs/iterator.c | 18 ++- refs/ref-cache.c | 100 ++++++++++++-- refs/ref-cache.h | 6 +- refs/refs-internal.h | 62 +++++++-- t/helper/test-ref-store.c | 3 +- t/t1405-main-ref-store.sh | 2 +- t/t1406-submodule-ref-store.sh | 2 +- t/t3600-rm.sh | 4 +- 15 files changed, 658 insertions(+), 203 deletions(-)
Signature · 1 line
2.11.0
PATCH v2 1/25 t3600: clean up permissions test properly
The test of failing git rm -f removes the write permissions on the test directory, but fails to restore them if the test fails. This means that the test temporary directory cannot be cleaned up, which means that subsequent attempts to run the test fail mysteriously.
Instead, do the cleanup in a test_when_finished block so that it can't be skipped.
--- t/t3600-rm.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/t3600-rm.sh +2 -2
@@ -97,9 +97,9 @@ test_expect_success FUNNYNAMES \ embedded'" test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' ' + test_when_finished "chmod 775 ." && chmod a-w . && - test_must_fail git rm -f baz && - chmod 775 . + test_must_fail git rm -f baz ' test_expect_success \
Signature · 1 line
2.11.0
PATCH v2 2/25 refs.h: clarify docstring for the ref_transaction_update()-related fns
In reply to Michael HaggertyIn particular, make it clear that they make copies of the sha1 arguments.
--- refs.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
refs.h +13 -0
@@ -427,6 +427,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);* * refname -- the name of the reference to be affected. * +* new_sha1 -- the SHA-1 that should be set to be the new value of +* the reference. Some functions allow this parameter to be +* NULL, meaning that the reference is not changed, or +* null_sha1, meaning that the reference should be deleted. A +* copy of this value is made in the transaction. +* +* old_sha1 -- the SHA-1 value that the reference must have before +* the update. Some functions allow this parameter to be NULL, +* meaning that the old value of the reference is not checked, +* or null_sha1, meaning that the reference must not exist +* before the update. A copy of this value is made in the +* transaction. +* * flags -- flags affecting the update, passed to * update_ref_lock(). Can be REF_NODEREF, which means that * symbolic references should not be followed.
Signature · 1 line
2.11.0
PATCH v2 3/25 ref_iterator_begin_fn(): fix docstring
In reply to Michael HaggertyThe iterator returned by this function only includes references whose names start with the whole prefix, not all of those in find_containing_dir(prefix) as the old docstring claimed. This docstring was probably copy-pasted from old ref-cache code, which had the old specification. But now, cache_ref_iterator_begin() (from which the files reference iterator gets its values) automatically wraps its output using prefix_ref_iterator_begin() when necessary, so it has the stricter behavior.
--- refs/refs-internal.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
refs/refs-internal.h +4 -3
@@ -515,9 +515,10 @@ typedef int rename_ref_fn(struct ref_store *ref_store, const char *logmsg);/* -* Iterate over the references in the specified ref_store that are -* within find_containing_dir(prefix). If prefix is NULL or the empty -* string, iterate over all references in the submodule. +* Iterate over the references in `ref_store` whose names start with +* `prefix`. `prefix` is matched as a literal string, without regard +* for path separators. If prefix is NULL or the empty string, iterate +* over all references in `ref_store`. */ typedef struct ref_iterator *ref_iterator_begin_fn( struct ref_store *ref_store,
Signature · 1 line
2.11.0
PATCH v2 4/25 files-backend: use `die("BUG: ...")`, not `die("internal error: ...")`
In reply to Michael HaggertyThe former is by far more common in our codebase.
--- refs/files-backend.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
refs/files-backend.c +4 -4
@@ -105,7 +105,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)struct packed_ref_cache *packed_refs = refs->packed;if (packed_refs->lock) -die ("internal error: packed-ref cache cleared while locked" ); +die ("BUG: packed-ref cache cleared while locked" ); refs->packed =NULL ;release_packed_ref_cache (packed_refs); }
@@ -397,7 +397,7 @@ static void add_packed_ref(struct files_ref_store *refs,struct packed_ref_cache *packed_ref_cache =get_packed_ref_cache (refs);if (!packed_ref_cache->lock) -die ("internal error: packed refs not locked" ); +die ("BUG: packed refs not locked" );add_ref_entry (get_packed_ref_dir (packed_ref_cache),create_ref_entry (refname, oid,REF_ISPACKED ,1 )); }
@@ -1324,7 +1324,7 @@ static int commit_packed_refs(struct files_ref_store *refs)files_assert_main_repository (refs,"commit_packed_refs" );if (!packed_ref_cache->lock) -die ("internal error: packed-refs not locked" ); +die ("BUG: packed-refs not locked" ); out =fdopen_lock_file (packed_ref_cache->lock,"w" );if (!out)
@@ -1367,7 +1367,7 @@ static void rollback_packed_refs(struct files_ref_store *refs)files_assert_main_repository (refs,"rollback_packed_refs" );if (!packed_ref_cache->lock) -die ("internal error: packed-refs not locked" ); +die ("BUG: packed-refs not locked" );rollback_lock_file (packed_ref_cache->lock); packed_ref_cache->lock =NULL ;release_packed_ref_cache (packed_ref_cache);
Signature · 1 line
2.11.0
PATCH v2 5/25 prefix_ref_iterator: don't trim too much
In reply to Michael HaggertyThe trim parameter can be set independently of prefix. So if some caller were to set trim to be greater than strlen(prefix), we could end up pointing the refname field of the iterator past the NUL of the actual reference name string.
That can't happen currently, because trim is always set either to zero or to strlen(prefix). But even the latter could lead to confusion, if a refname is exactly equal to the prefix, because then we would set the outgoing refname to the empty string.
And we're about to decouple the prefix and trim arguments even more, so let's be cautious here. Report a bug if ever asked to trim a reference whose name is not longer than trim.
--- refs/iterator.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
refs/iterator.c +17 -1
@@ -292,7 +292,23 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)if (!starts_with (iter->iter0->refname, iter->prefix))continue ; - iter->base.refname = iter->iter0->refname + iter->trim; +if (iter->trim) { +/* +* It is nonsense to trim off characters that +* you haven't already checked for via a +* prefix check, whether via this +* `prefix_ref_iterator` or upstream in +* `iter0`). So if there wouldn't be at least +* one character left in the refname after +* trimming, report it as a bug: +*/ +if (strlen (iter->iter0->refname) <= iter->trim) +die ("BUG: attempt to trim too many characters" ); + iter->base.refname = iter->iter0->refname + iter->trim; + }else { + iter->base.refname = iter->iter0->refname; + } + iter->base.oid = iter->iter0->oid; iter->base.flags = iter->iter0->flags;return ITER_OK ;
Signature · 1 line
2.11.0
PATCH v2 6/25 refs_ref_iterator_begin(): don't check prefixes redundantly
In reply to Michael HaggertyThe backend already correctly restricts its output to references whose names start with the prefix. By passing the prefix again to prefix_ref_iterator, we were forcing that iterator to do redundant prefix comparisons. So set it to the empty string.
--- refs.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
refs.c +7 -1
@@ -1247,7 +1247,13 @@ struct ref_iterator *refs_ref_iterator_begin(struct ref_iterator *iter; iter = refs->be->iterator_begin (refs, prefix, flags); - iter =prefix_ref_iterator_begin (iter, prefix, trim); + +/* +* `iterator_begin()` already takes care of prefix, but we +* might need to do some trimming: +*/ +if (trim) + iter =prefix_ref_iterator_begin (iter,"" , trim);return iter; }
Signature · 1 line
2.11.0
PATCH v2 7/25 refs: use `size_t` indexes when iterating over ref transaction updates
In reply to Michael HaggertyEliminate any chance of integer overflow on platforms where the two types have different sizes.
--- refs.c | 2 +- refs/files-backend.c | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-)
refs.c +1 -1
@@ -848,7 +848,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err)void ref_transaction_free (struct ref_transaction *transaction) { -int i; +size_t i;if (!transaction)return ;
refs/files-backend.c +4 -2
@@ -2850,7 +2850,8 @@ static int files_transaction_commit(struct ref_store *ref_store,struct files_ref_store *refs =files_downcast (ref_store,REF_STORE_WRITE ,"ref_transaction_commit" ); -int ret =0 , i; +size_t i; +int ret =0 ;struct string_list refs_to_delete =STRING_LIST_INIT_NODUP ;struct string_list_item *ref_to_delete;struct string_list affected_refnames =STRING_LIST_INIT_NODUP ;
@@ -3057,7 +3058,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,struct files_ref_store *refs =files_downcast (ref_store,REF_STORE_WRITE ,"initial_ref_transaction_commit" ); -int ret =0 , i; +size_t i; +int ret =0 ;struct string_list affected_refnames =STRING_LIST_INIT_NODUP ;assert (err);
Signature · 1 line
2.11.0
PATCH v2 8/25 ref_store: take a `msg` parameter when deleting references
In reply to Michael HaggertyJust because the files backend can't retain reflogs for deleted references is no reason that they shouldn't be supported by the virtual method interface. Also, delete_ref() and refs_delete_ref() have already gained msg parameters. Now let's add them to delete_refs() and refs_delete_refs().
--- builtin/fetch.c | 2 +- builtin/remote.c | 4 ++-- refs.c | 11 ++++++----- refs.h | 12 +++++++----- refs/files-backend.c | 4 ++-- refs/refs-internal.h | 2 +- t/helper/test-ref-store.c | 3 ++- t/t1405-main-ref-store.sh | 2 +- t/t1406-submodule-ref-store.sh | 2 +- 9 files changed, 23 insertions(+), 19 deletions(-)
builtin/fetch.c +1 -1
@@ -941,7 +941,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,for (ref = stale_refs; ref; ref = ref->next)string_list_append (&refnames, ref->name); - result =delete_refs (&refnames,0 ); + result =delete_refs ("fetch: prune" , &refnames,0 );string_list_clear (&refnames,0 ); }
builtin/remote.c +2 -2
@@ -786,7 +786,7 @@ static int rm(int argc, const char **argv)strbuf_release (&buf);if (!result) - result =delete_refs (&branches,REF_NODEREF ); + result =delete_refs ("remote: remove" , &branches,REF_NODEREF );string_list_clear (&branches,0 );if (skipped.nr) {
@@ -1301,7 +1301,7 @@ static int prune_remote(const char *remote, int dry_run)string_list_sort (&refs_to_prune);if (!dry_run) - result |=delete_refs (&refs_to_prune,0 ); + result |=delete_refs ("remote: prune" , &refs_to_prune,0 );for_each_string_list_item (item, &states.stale) {const char *refname = item->util;
refs.c +6 -5
@@ -1902,15 +1902,16 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,return refs->be->initial_transaction_commit (refs, transaction, err); } -int refs_delete_refs (struct ref_store *refs,struct string_list *refnames, -unsigned int flags) +int refs_delete_refs (struct ref_store *refs,const char *msg, +struct string_list *refnames,unsigned int flags) { -return refs->be->delete_refs (refs, refnames, flags); +return refs->be->delete_refs (refs, msg, refnames, flags); } -int delete_refs (struct string_list *refnames,unsigned int flags) +int delete_refs (const char *msg,struct string_list *refnames, +unsigned int flags) { -return refs_delete_refs (get_main_ref_store (), refnames, flags); +return refs_delete_refs (get_main_ref_store (), msg, refnames, flags); } int refs_rename_ref(struct ref_store *refs, const char *oldref,
refs.h +7 -5
@@ -331,7 +331,8 @@ int reflog_exists(const char *refname);* verify that the current value of the reference is old_sha1 before * deleting it. If old_sha1 is NULL, delete the reference if it * exists, regardless of its old value. It is an error for old_sha1 to -* be NULL_SHA1. flags is passed through to ref_transaction_delete(). +* be NULL_SHA1. msg and flags are passed through to +* ref_transaction_delete(). */ int refs_delete_ref(struct ref_store *refs, const char *msg, const char *refname,
@@ -343,12 +344,13 @@ int delete_ref(const char *msg, const char *refname,/* * Delete the specified references. If there are any problems, emit * errors but attempt to keep going (i.e., the deletes are not done in -* an all-or-nothing transaction). flags is passed through to +* an all-or-nothing transaction). msg and flags are passed through to * ref_transaction_delete(). */ -int refs_delete_refs (struct ref_store *refs,struct string_list *refnames, -unsigned int flags); -int delete_refs (struct string_list *refnames,unsigned int flags); +int refs_delete_refs (struct ref_store *refs,const char *msg, +struct string_list *refnames,unsigned int flags); +int delete_refs (const char *msg,struct string_list *refnames, +unsigned int flags);/** Delete a reflog */ int refs_delete_reflog (struct ref_store *refs,const char *refname);
refs/files-backend.c +2 -2
@@ -1595,7 +1595,7 @@ static int repack_without_refs(struct files_ref_store *refs,return ret; } -static int files_delete_refs (struct ref_store *ref_store, +static int files_delete_refs (struct ref_store *ref_store,const char *msg,struct string_list *refnames,unsigned int flags) { struct files_ref_store *refs =
@@ -1627,7 +1627,7 @@ static int files_delete_refs(struct ref_store *ref_store,for (i =0 ; i < refnames->nr; i++) {const char *refname = refnames->items[i].string; -if (refs_delete_ref (&refs->base,NULL , refname,NULL , flags)) +if (refs_delete_ref (&refs->base, msg, refname,NULL , flags)) result |=error (_ ("could not remove reference %s" ), refname); }
refs/refs-internal.h +1 -1
@@ -508,7 +508,7 @@ typedef int create_symref_fn(struct ref_store *ref_store,const char *ref_target, const char *refs_heads_master, const char *logmsg); -typedef int delete_refs_fn (struct ref_store *ref_store, +typedef int delete_refs_fn (struct ref_store *ref_store,const char *msg,struct string_list *refnames,unsigned int flags); typedef int rename_ref_fn(struct ref_store *ref_store, const char *oldref, const char *newref,
t/helper/test-ref-store.c +2 -1
@@ -93,12 +93,13 @@ static int cmd_create_symref(struct ref_store *refs, const char **argv)static int cmd_delete_refs (struct ref_store *refs,const char **argv) {unsigned int flags =arg_flags (*argv++,"flags" ); +const char *msg = *argv++;struct string_list refnames =STRING_LIST_INIT_NODUP ;while (*argv)string_list_append (&refnames, *argv++); -return refs_delete_refs (refs, &refnames, flags); +return refs_delete_refs (refs, msg, &refnames, flags); }static int cmd_rename_ref (struct ref_store *refs,const char **argv)
t/t1405-main-ref-store.sh +1 -1
@@ -31,7 +31,7 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' ' test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' ' git rev-parse FOO -- && git rev-parse refs/tags/new-tag -- && - $RUN delete-refs 0 FOO refs/tags/new-tag && + $RUN delete-refs 0 nothing FOO refs/tags/new-tag && test_must_fail git rev-parse FOO -- && test_must_fail git rev-parse refs/tags/new-tag -- '
t/t1406-submodule-ref-store.sh +1 -1
@@ -31,7 +31,7 @@ test_expect_success 'create_symref() not allowed' ' ' test_expect_success 'delete_refs() not allowed' ' - test_must_fail $RUN delete-refs 0 FOO refs/tags/new-tag + test_must_fail $RUN delete-refs 0 nothing FOO refs/tags/new-tag ' test_expect_success 'rename_refs() not allowed' '
Signature · 1 line
2.11.0
PATCH v2 9/25 lockfile: add a new method, is_lock_file_locked()
In reply to Michael HaggertyIt will soon prove useful.
--- lockfile.h | 8 ++++++++ 1 file changed, 8 insertions(+)
lockfile.h +8 -0
@@ -175,6 +175,14 @@ static inline int hold_lock_file_for_update(return hold_lock_file_for_update_timeout (lk, path, flags,0 ); } +/* +* Return a nonzero value iff `lk` is currently locked. +*/ +static inline int is_lock_file_locked (struct lock_file *lk) +{ +return is_tempfile_active (&lk->tempfile); +} +/* * Append an appropriate error message to `buf` following the failure * of `hold_lock_file_for_update()` to lock `path`. `err` should be the
Signature · 1 line
2.11.0
PATCH v2 10/25 files-backend: move `lock` member to `files_ref_store`
In reply to Michael HaggertyMove the lock member from packed_ref_cache to files_ref_store, since at most one cache can have a locked "packed-refs" file associated with it. Rename it to packed_refs_lock to make its purpose clearer in its new home. More changes are coming here shortly.
--- refs/files-backend.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)
refs/files-backend.c +17 -19
@@ -43,15 +43,6 @@ struct packed_ref_cache {*/ unsigned int referrers; -/* -* Iff the packed-refs file associated with this instance is -* currently locked for writing, this points at the associated -* lock (which is owned by somebody else). The referrer count -* is also incremented when the file is locked and decremented -* when it is unlocked. -*/ -struct lock_file *lock; -/* The metadata from when this packed-refs cache was read */ struct stat_validity validity; };
@@ -70,6 +61,13 @@ struct files_ref_store {struct ref_cache *loose;struct packed_ref_cache *packed; + +/* +* Iff the packed-refs file associated with this instance is +* currently locked for writing, this points at the associated +* lock (which is owned by somebody else). +*/ +struct lock_file *packed_refs_lock; };/* Lock used for the main packed-refs file: */
@@ -104,7 +102,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)if (refs->packed) {struct packed_ref_cache *packed_refs = refs->packed; -if (packed_refs->lock) +if (refs->packed_refs_lock)die ("BUG: packed-ref cache cleared while locked" ); refs->packed =NULL ;release_packed_ref_cache (packed_refs);
@@ -396,7 +394,7 @@ static void add_packed_ref(struct files_ref_store *refs, {struct packed_ref_cache *packed_ref_cache =get_packed_ref_cache (refs); -if (!packed_ref_cache->lock) +if (!refs->packed_refs_lock)die ("BUG: packed refs not locked" );add_ref_entry (get_packed_ref_dir (packed_ref_cache),create_ref_entry (refname, oid,REF_ISPACKED ,1 ));
@@ -1300,7 +1298,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)* the packed-refs file. */ packed_ref_cache =get_packed_ref_cache (refs); - packed_ref_cache->lock = &packlock; + refs->packed_refs_lock = &packlock;/* Increment the reference count to prevent it from being freed: */ acquire_packed_ref_cache (packed_ref_cache);return 0 ;
@@ -1323,10 +1321,10 @@ static int commit_packed_refs(struct files_ref_store *refs)files_assert_main_repository (refs,"commit_packed_refs" ); -if (!packed_ref_cache->lock) +if (!refs->packed_refs_lock)die ("BUG: packed-refs not locked" ); - out =fdopen_lock_file (packed_ref_cache->lock,"w" ); + out =fdopen_lock_file (refs->packed_refs_lock,"w" );if (!out)die_errno ("unable to fdopen packed-refs descriptor" );
@@ -1344,11 +1342,11 @@ static int commit_packed_refs(struct files_ref_store *refs)if (ok !=ITER_DONE )die ("error while iterating over references" ); -if (commit_lock_file (packed_ref_cache->lock)) { +if (commit_lock_file (refs->packed_refs_lock)) { save_errno = errno; error =-1 ; } - packed_ref_cache->lock =NULL ; + refs->packed_refs_lock =NULL ;release_packed_ref_cache (packed_ref_cache); errno = save_errno;return error;
@@ -1366,10 +1364,10 @@ static void rollback_packed_refs(struct files_ref_store *refs)files_assert_main_repository (refs,"rollback_packed_refs" ); -if (!packed_ref_cache->lock) +if (!refs->packed_refs_lock)die ("BUG: packed-refs not locked" ); -rollback_lock_file (packed_ref_cache->lock); - packed_ref_cache->lock =NULL ; +rollback_lock_file (refs->packed_refs_lock); + refs->packed_refs_lock =NULL ;release_packed_ref_cache (packed_ref_cache);clear_packed_ref_cache (refs); }
Signature · 1 line
2.11.0
PATCH v2 11/25 files_ref_store: put the packed files lock directly in this struct
In reply to Michael HaggertyInstead of using a global lock_file instance for the main "packed-refs" file and using a pointer in files_ref_store to keep track of whether it is locked, embed the lock_file instance directly in the files_ref_store struct and use the new is_lock_file_locked() function to keep track of whether it is locked. This keeps related data together and makes the main reference store less of a special case.
--- refs/files-backend.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)
refs/files-backend.c +11 -18
@@ -63,16 +63,12 @@ struct files_ref_store {struct packed_ref_cache *packed;/* -* Iff the packed-refs file associated with this instance is -* currently locked for writing, this points at the associated -* lock (which is owned by somebody else). +* Lock used for the "packed-refs" file. Note that this (and +* thus the enclosing `files_ref_store`) must not be freed. */ -struct lock_file *packed_refs_lock; +struct lock_file packed_refs_lock; }; -/* Lock used for the main packed-refs file: */ -static struct lock_file packlock; -/* * Increment the reference count of *packed_refs. */
@@ -102,7 +98,7 @@ static void clear_packed_ref_cache(struct files_ref_store *refs)if (refs->packed) {struct packed_ref_cache *packed_refs = refs->packed; -if (refs->packed_refs_lock) +if (is_lock_file_locked (&refs->packed_refs_lock))die ("BUG: packed-ref cache cleared while locked" ); refs->packed =NULL ;release_packed_ref_cache (packed_refs);
@@ -394,7 +390,7 @@ static void add_packed_ref(struct files_ref_store *refs, {struct packed_ref_cache *packed_ref_cache =get_packed_ref_cache (refs); -if (!refs->packed_refs_lock) +if (!is_lock_file_locked (&refs->packed_refs_lock))die ("BUG: packed refs not locked" );add_ref_entry (get_packed_ref_dir (packed_ref_cache),create_ref_entry (refname, oid,REF_ISPACKED ,1 ));
@@ -1288,7 +1284,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags) }if (hold_lock_file_for_update_timeout ( - &packlock,files_packed_refs_path (refs), + &refs->packed_refs_lock,files_packed_refs_path (refs), flags, timeout_value) <0 )return -1 ;/*
@@ -1298,7 +1294,6 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)* the packed-refs file. */ packed_ref_cache =get_packed_ref_cache (refs); - refs->packed_refs_lock = &packlock;/* Increment the reference count to prevent it from being freed: */ acquire_packed_ref_cache (packed_ref_cache);return 0 ;
@@ -1321,10 +1316,10 @@ static int commit_packed_refs(struct files_ref_store *refs)files_assert_main_repository (refs,"commit_packed_refs" ); -if (!refs->packed_refs_lock) +if (!is_lock_file_locked (&refs->packed_refs_lock))die ("BUG: packed-refs not locked" ); - out =fdopen_lock_file (refs->packed_refs_lock,"w" ); + out =fdopen_lock_file (&refs->packed_refs_lock,"w" );if (!out)die_errno ("unable to fdopen packed-refs descriptor" );
@@ -1342,11 +1337,10 @@ static int commit_packed_refs(struct files_ref_store *refs)if (ok !=ITER_DONE )die ("error while iterating over references" ); -if (commit_lock_file (refs->packed_refs_lock)) { +if (commit_lock_file (&refs->packed_refs_lock)) { save_errno = errno; error =-1 ; } - refs->packed_refs_lock =NULL ;release_packed_ref_cache (packed_ref_cache); errno = save_errno;return error;
@@ -1364,10 +1358,9 @@ static void rollback_packed_refs(struct files_ref_store *refs)files_assert_main_repository (refs,"rollback_packed_refs" ); -if (!refs->packed_refs_lock) +if (!is_lock_file_locked (&refs->packed_refs_lock))die ("BUG: packed-refs not locked" ); -rollback_lock_file (refs->packed_refs_lock); - refs->packed_refs_lock =NULL ; +rollback_lock_file (&refs->packed_refs_lock);release_packed_ref_cache (packed_ref_cache);clear_packed_ref_cache (refs); }
Signature · 1 line
2.11.0
PATCH v2 12/25 files_transaction_cleanup(): new helper function
In reply to Michael HaggertyExtract the cleanup functionality from files_transaction_commit() into a new function. It will soon have another caller.
Use the common cleanup code even on early exit if the transaction is empty, to reduce code duplication.
--- refs/files-backend.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-)
refs/files-backend.c +24 -9
@@ -2834,6 +2834,27 @@ static int lock_ref_for_update(struct files_ref_store *refs,return 0 ; } +/* +* Unlock any references in `transaction` that are still locked, and +* mark the transaction closed. +*/ +static void files_transaction_cleanup (struct ref_transaction *transaction) +{ +size_t i; + +for (i =0 ; i < transaction->nr; i++) { +struct ref_update *update = transaction->updates[i]; +struct ref_lock *lock = update->backend_data; + +if (lock) { +unlock_ref (lock); + update->backend_data =NULL ; + } + } + + transaction->state =REF_TRANSACTION_CLOSED ; +} +static int files_transaction_commit (struct ref_store *ref_store,struct ref_transaction *transaction,struct strbuf *err)
@@ -2856,10 +2877,8 @@ static int files_transaction_commit(struct ref_store *ref_store,if (transaction->state !=REF_TRANSACTION_OPEN )die ("BUG: commit called for transaction that is not open" ); -if (!transaction->nr) { - transaction->state =REF_TRANSACTION_CLOSED ; -return 0 ; - } +if (!transaction->nr) + goto cleanup;/* * Fail if a refname appears more than once in the
@@ -3005,15 +3024,11 @@ static int files_transaction_commit(struct ref_store *ref_store,clear_loose_ref_cache (refs); cleanup: +files_transaction_cleanup (transaction);strbuf_release (&sb); - transaction->state =REF_TRANSACTION_CLOSED ;for (i =0 ; i < transaction->nr; i++) {struct ref_update *update = transaction->updates[i]; -struct ref_lock *lock = update->backend_data; - -if (lock) -unlock_ref (lock);if (update->flags &REF_DELETED_LOOSE ) {/*
Signature · 1 line
2.11.0
PATCH v2 13/25 ref_transaction_commit(): check for valid `transaction->state`
In reply to Michael HaggertyMove the check that transaction->state is valid from files_transaction_commit() to ref_transaction_commit(), where other future reference backends can benefit from it as well.
--- refs.c | 12 ++++++++++++ refs/files-backend.c | 3 --- 2 files changed, 12 insertions(+), 3 deletions(-)
refs.c +12 -0
@@ -1694,6 +1694,18 @@ int ref_transaction_commit(struct ref_transaction *transaction, {struct ref_store *refs = transaction->ref_store; +switch (transaction->state) { +case REF_TRANSACTION_OPEN : +/* Good. */ +break ; +case REF_TRANSACTION_CLOSED : +die ("BUG: prepare called on a closed reference transaction" ); +break ; +default : +die ("BUG: unexpected reference transaction state" ); +break ; + } +if (getenv (GIT_QUARANTINE_ENVIRONMENT )) {strbuf_addstr (err,_ ("ref updates forbidden inside quarantine environment" ));
refs/files-backend.c +0 -3
@@ -2874,9 +2874,6 @@ static int files_transaction_commit(struct ref_store *ref_store,assert (err); -if (transaction->state !=REF_TRANSACTION_OPEN ) -die ("BUG: commit called for transaction that is not open" ); -if (!transaction->nr) goto cleanup;
Signature · 1 line
2.11.0
PATCH v2 14/25 ref_transaction_prepare(): new optional step for reference updates
In reply to Michael HaggertyIn the future, compound reference stores will sometimes need to modify references in two different reference stores at the same time, meaning that a single logical reference transaction might have to be implemented as two internal sub-transactions. They won't want to call ref_transaction_commit() for the two sub-transactions one after the other, because that wouldn't be atomic (the first commit could succeed and the second one fail). Instead, they will want to prepare both sub-transactions (i.e., obtain any necessary locks and do any pre-checks), and only if both prepare steps succeed, then commit both sub-transactions.
Start preparing for that day by adding a new, optional ref_transaction_prepare() step to the reference transaction sequence, which obtains the locks and does any prechecks, reporting any errors that occur. Also add a ref_transaction_abort() function that can be used to abort a sub-transaction even if it has already been prepared.
That is on the side of the public-facing API. On the side of the ref_store VTABLE, get rid of transaction_commit and instead add methods transaction_prepare, transaction_finish, and transaction_abort. A ref_transaction_commit() now basically calls methods transaction_prepare then transaction_finish.
--- refs.c | 74 ++++++++++++++++++++++++++++-- refs.h | 124 ++++++++++++++++++++++++++++++++++++++++----------- refs/files-backend.c | 63 ++++++++++++++++++++------ refs/refs-internal.h | 45 ++++++++++++++----- 4 files changed, 253 insertions(+), 53 deletions(-)
refs.c +71 -3
@@ -853,6 +853,19 @@ void ref_transaction_free(struct ref_transaction *transaction)if (!transaction)return ; +switch (transaction->state) { +case REF_TRANSACTION_OPEN : +case REF_TRANSACTION_CLOSED : +/* OK */ +break ; +case REF_TRANSACTION_PREPARED : +die ("BUG: free called on a prepared reference transaction" ); +break ; +default : +die ("BUG: unexpected reference transaction state" ); +break ; + } +for (i =0 ; i < transaction->nr; i++) {free (transaction->updates[i]->msg);free (transaction->updates[i]);
@@ -1689,8 +1702,8 @@ int create_symref(const char *ref_target, const char *refs_heads_master, refs_heads_master, logmsg); } -int ref_transaction_commit (struct ref_transaction *transaction, -struct strbuf *err) +int ref_transaction_prepare (struct ref_transaction *transaction, +struct strbuf *err) {struct ref_store *refs = transaction->ref_store;
@@ -1698,6 +1711,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,case REF_TRANSACTION_OPEN :/* Good. */ break ; +case REF_TRANSACTION_PREPARED : +die ("BUG: prepare called twice on reference transaction" ); +break ;case REF_TRANSACTION_CLOSED :die ("BUG: prepare called on a closed reference transaction" );break ;
@@ -1712,7 +1728,59 @@ int ref_transaction_commit(struct ref_transaction *transaction,return -1 ; } -return refs->be->transaction_commit (refs, transaction, err); +return refs->be->transaction_prepare (refs, transaction, err); +} + +int ref_transaction_abort (struct ref_transaction *transaction, +struct strbuf *err) +{ +struct ref_store *refs = transaction->ref_store; +int ret =0 ; + +switch (transaction->state) { +case REF_TRANSACTION_OPEN : +/* No need to abort explicitly. */ +break ; +case REF_TRANSACTION_PREPARED : + ret = refs->be->transaction_abort (refs, transaction, err); +break ; +case REF_TRANSACTION_CLOSED : +die ("BUG: abort called on a closed reference transaction" ); +break ; +default : +die ("BUG: unexpected reference transaction state" ); +break ; + } + +ref_transaction_free (transaction); +return ret; +} + +int ref_transaction_commit (struct ref_transaction *transaction, +struct strbuf *err) +{ +struct ref_store *refs = transaction->ref_store; +int ret; + +switch (transaction->state) { +case REF_TRANSACTION_OPEN : +/* Need to prepare first. */ + ret =ref_transaction_prepare (transaction, err); +if (ret) +return ret; +break ; +case REF_TRANSACTION_PREPARED : +/* Fall through to finish. */ +break ; +case REF_TRANSACTION_CLOSED : +die ("BUG: commit called on a closed reference transaction" ); +break ; +default : +die ("BUG: unexpected reference transaction state" ); +break ; + } + +return refs->be->transaction_finish (refs, transaction, err); } int refs_verify_refname_available(struct ref_store *refs,
refs.h +97 -27
161 lines not on this page · raw message
refs/files-backend.c +50 -13
112 lines not on this page · raw message
refs/refs-internal.h +35 -10
65 lines not on this page · raw message
Signature · 1 line
2.11.0
PATCH v2 15/25 ref_update_reject_duplicates(): expose function to whole refs module
In reply to Michael HaggertyIt will soon have some other users.
--- refs.c | 17 +++++++++++++++++ refs/files-backend.c | 17 ----------------- refs/refs-internal.h | 8 ++++++++ 3 files changed, 25 insertions(+), 17 deletions(-)
refs.c +17 -0
@@ -1702,6 +1702,23 @@ int create_symref(const char *ref_target, const char *refs_heads_master, refs_heads_master, logmsg); } +int ref_update_reject_duplicates (struct string_list *refnames, +struct strbuf *err) +{ +int i, n = refnames->nr; + +assert (err); + +for (i =1 ; i < n; i++) +if (!strcmp (refnames->items[i -1 ].string, refnames->items[i].string)) { +strbuf_addf (err, +"multiple updates for ref '%s' not allowed." , + refnames->items[i].string); +return 1 ; + } +return 0 ; +} +int ref_transaction_prepare (struct ref_transaction *transaction,struct strbuf *err) {
refs/files-backend.c +0 -17
@@ -2512,23 +2512,6 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_streturn ref_iterator; } -static int ref_update_reject_duplicates (struct string_list *refnames, -struct strbuf *err) -{ -int i, n = refnames->nr; - -assert (err); - -for (i =1 ; i < n; i++) -if (!strcmp (refnames->items[i -1 ].string, refnames->items[i].string)) { -strbuf_addf (err, -"multiple updates for ref '%s' not allowed." , - refnames->items[i].string); -return 1 ; - } -return 0 ; -} -/* * If update is a direct update of head_ref (the reference pointed to * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.
refs/refs-internal.h +8 -0
@@ -169,6 +169,14 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, unsigned char *sha1, struct strbuf *referent, unsigned int *type); +/* +* Write an error to `err` and return a nonzero value iff the same +* refname appears multiple times in `refnames`. `refnames` must be +* sorted on entry to this function. +*/ +int ref_update_reject_duplicates (struct string_list *refnames, +struct strbuf *err); +/* * 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 16/25 ref_update_reject_duplicates(): use `size_t` rather than `int`
In reply to Michael HaggertyEliminate a theoretical risk of integer overflow if the two types have different sizes.
--- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
refs.c +1 -1
@@ -1705,7 +1705,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,int ref_update_reject_duplicates (struct string_list *refnames,struct strbuf *err) { -int i, n = refnames->nr; +size_t i, n = refnames->nr;assert (err);
Signature · 1 line
2.11.0
PATCH v2 17/25 ref_update_reject_duplicates(): add a sanity check
In reply to Michael HaggertyIt's pretty cheap to make sure that the caller didn't pass us an unsorted list by accident, so do so.
--- refs.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
refs.c +8 -2
@@ -1709,13 +1709,19 @@ int ref_update_reject_duplicates(struct string_list *refnames,assert (err); -for (i =1 ; i < n; i++) -if (!strcmp (refnames->items[i -1 ].string, refnames->items[i].string)) { +for (i =1 ; i < n; i++) { +int cmp =strcmp (refnames->items[i -1 ].string, + refnames->items[i].string); + +if (!cmp) {strbuf_addf (err,"multiple updates for ref '%s' not allowed." , refnames->items[i].string);return 1 ; + }else if (cmp >0 ) { +die ("BUG: ref_update_reject_duplicates() received unsorted list" ); } + }return 0 ; }
Signature · 1 line
2.11.0
PATCH v2 18/25 should_pack_ref(): new function, extracted from `files_pack_refs()`
In reply to Michael HaggertyExtract a function for deciding whether a reference should be packed. It is a self-contained bit of logic, so splitting it out improves readability.
--- refs/files-backend.c | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-)
refs/files-backend.c +28 -14
@@ -1455,6 +1455,32 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune *r) } } +/* +* Return true if the specified reference should be packed. +*/ +static int should_pack_ref (const char *refname, +const struct object_id *oid,unsigned int ref_flags, +unsigned int pack_flags) +{ +/* Do not pack per-worktree refs: */ +if (ref_type (refname) !=REF_TYPE_NORMAL ) +return 0 ; + +/* Do not pack non-tags unless PACK_REFS_ALL is set: */ +if (!(pack_flags &PACK_REFS_ALL ) && !starts_with (refname,"refs/tags/" )) +return 0 ; + +/* Do not pack symbolic refs: */ +if (ref_flags &REF_ISSYMREF ) +return 0 ; + +/* Do not pack broken refs: */ +if (!ref_resolves_to_object (refname, oid, ref_flags)) +return 0 ; + +return 1 ; +} +static int files_pack_refs (struct ref_store *ref_store,unsigned int flags) { struct files_ref_store *refs =
@@ -1476,21 +1502,9 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)* 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 ; -if (!ref_resolves_to_object (iter->refname, iter->oid, iter->flags)) +if (!should_pack_ref (iter->refname, iter->oid, iter->flags, + flags))continue ;/*
Signature · 1 line
2.11.0
PATCH v2 19/25 get_packed_ref_cache(): assume "packed-refs" won't change while locked
In reply to Michael HaggertyIf we've got the "packed-refs" file locked, then it can't change; there's no need to keep calling stat_validity_check() on it.
--- refs/files-backend.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
refs/files-backend.c +11 -5
@@ -342,13 +342,18 @@ static void files_ref_path(struct files_ref_store *refs,/* * Get the packed_ref_cache for the specified files_ref_store, -* creating it if necessary. +* creating and populating it if it hasn't been read before or if the +* file has been changed (according to its `validity` field) since it +* was last read. On the other hand, if we hold the lock, then assume +* that the file hasn't been changed out from under us, so skip the +* extra `stat()` call in `stat_validity_check()`. */ static struct packed_ref_cache *get_packed_ref_cache (struct files_ref_store *refs) {const char *packed_refs_file =files_packed_refs_path (refs);if (refs->packed && + !is_lock_file_locked (&refs->packed_refs_lock) && !stat_validity_check (&refs->packed->validity, packed_refs_file))clear_packed_ref_cache (refs);
@@ -1288,10 +1293,11 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags) flags, timeout_value) < 0)return -1 ;/* -* Get the current packed-refs while holding the lock. If the -* packed-refs file has been modified since we last read it, -* this will automatically invalidate the cache and re-read -* the packed-refs file. +* Get the current packed-refs while holding the lock. It is +* important that we call `get_packed_ref_cache()` before +* setting `packed_ref_cache->lock`, because otherwise the +* former will see that the file is locked and assume that the +* cache can't be stale. */ packed_ref_cache =get_packed_ref_cache (refs);/* Increment the reference count to prevent it from being freed: */
Signature · 1 line
2.11.0
PATCH v2 20/25 read_packed_refs(): do more of the work of reading packed refs
In reply to Michael HaggertyTeach read_packed_refs() to also
* Allocate and initialize the new packed_ref_cache
* Open and close the packed-refs file
* Update the validity field of the new object
This decreases the coupling between packed_refs_cache and files_ref_store by a little bit.
--- refs/files-backend.c | 40 ++++++++++++++++++++++++---------------- refs/ref-cache.h | 3 ++- 2 files changed, 26 insertions(+), 17 deletions(-)
refs/files-backend.c +24 -16
@@ -209,7 +209,9 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid) }/* -* Read f, which is a packed-refs file, into dir. +* Read from `packed_refs_file` into a newly-allocated +* `packed_ref_cache` and return it. The return value will already +* have its reference count incremented. * * A comment line of the form "# pack-refs with: " may contain zero or * more traits. We interpret the traits as follows:
@@ -235,12 +237,26 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)* compatibility with older clients, but we do not require it * (i.e., "peeled" is a no-op if "fully-peeled" is set). */ -static void read_packed_refs (FILE *f,struct ref_dir *dir) +static struct packed_ref_cache *read_packed_refs (const char *packed_refs_file) { +FILE *f; +struct packed_ref_cache *packed_refs =xcalloc (1 ,sizeof (*packed_refs));struct ref_entry *last =NULL ;struct strbuf line =STRBUF_INIT ;enum {PEELED_NONE ,PEELED_TAGS ,PEELED_FULLY } peeled =PEELED_NONE ; +struct ref_dir *dir; +acquire_packed_ref_cache (packed_refs); + packed_refs->cache =create_ref_cache (NULL ,NULL ); + packed_refs->cache->root->flag &= ~REF_INCOMPLETE ; + + f =fopen (packed_refs_file,"r" ); +if (!f) +return packed_refs; + +stat_validity_update (&packed_refs->validity,fileno (f)); + + dir =get_ref_dir (packed_refs->cache->root);while (strbuf_getwholeline (&line, f,'\n' ) !=EOF ) {struct object_id oid;const char *refname;
@@ -287,7 +303,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) } } +fclose (f);strbuf_release (&line); + +return packed_refs; }static const char *files_packed_refs_path (struct files_ref_store *refs)
@@ -357,20 +376,9 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref !stat_validity_check(&refs->packed->validity, packed_refs_file))clear_packed_ref_cache (refs); -if (!refs->packed) { -FILE *f; - - refs->packed =xcalloc (1 ,sizeof (*refs->packed)); -acquire_packed_ref_cache (refs->packed); - refs->packed->cache =create_ref_cache (&refs->base,NULL ); - 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->cache->root)); -fclose (f); - } - } +if (!refs->packed) + refs->packed =read_packed_refs (packed_refs_file); +return refs->packed; }
refs/ref-cache.h +2 -1
@@ -194,7 +194,8 @@ struct ref_entry *create_ref_entry(const char *refname,* 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. +* `REF_INCOMPLETE` bits) before it is used, and `refs` can be NULL, +* too. */ struct ref_cache *create_ref_cache (struct ref_store *refs,fill_ref_dir_fn *fill_ref_dir);
Signature · 1 line
2.11.0
PATCH v2 21/25 read_packed_refs(): report unexpected fopen() failures
In reply to Michael HaggertyThe old code ignored any errors encountered when trying to fopen the "packed-refs" file, treating all such failures as if the file didn't exist. But it could be that there is some other error opening the file (e.g., permissions problems), and we don't want to silently ignore such problems. So report any failures that are not due to ENOENT.
--- refs/files-backend.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
refs/files-backend.c +12 -2
@@ -251,8 +251,18 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file) packed_refs->cache->root->flag &= ~REF_INCOMPLETE ; f =fopen (packed_refs_file,"r" ); -if (!f) -return packed_refs; +if (!f) { +if (errno ==ENOENT ) { +/* +* This is OK; it just means that no +* "packed-refs" file has been written yet, +* which is equivalent to it being empty. +*/ +return packed_refs; + }else { +die_errno ("couldn't read %s" , packed_refs_file); + } + }stat_validity_update (&packed_refs->validity,fileno (f));
Signature · 1 line
2.11.0
PATCH v2 22/25 refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA`
In reply to Michael HaggertyInstead of handling GIT_REF_PARANOIA in files_ref_iterator_begin(), handle it in refs_ref_iterator_begin(), where it will cover all reference stores.
--- refs.c | 5 +++++ refs/files-backend.c | 11 ++++------- 2 files changed, 9 insertions(+), 7 deletions(-)
refs.c +5 -0
@@ -1259,6 +1259,11 @@ struct ref_iterator *refs_ref_iterator_begin( {struct ref_iterator *iter; +if (ref_paranoia <0 ) + ref_paranoia =git_env_bool ("GIT_REF_PARANOIA" ,0 ); +if (ref_paranoia) + flags |=DO_FOR_EACH_INCLUDE_BROKEN ; + iter = refs->be->iterator_begin (refs, prefix, flags);/*
refs/files-backend.c +4 -7
@@ -1074,15 +1074,12 @@ static struct ref_iterator *files_ref_iterator_begin(struct ref_iterator *loose_iter, *packed_iter;struct files_ref_iterator *iter;struct ref_iterator *ref_iterator; +unsigned int required_flags =REF_STORE_READ ; -if (ref_paranoia <0 ) - ref_paranoia =git_env_bool ("GIT_REF_PARANOIA" ,0 ); -if (ref_paranoia) - flags |=DO_FOR_EACH_INCLUDE_BROKEN ; +if (!(flags &DO_FOR_EACH_INCLUDE_BROKEN )) + required_flags |=REF_STORE_ODB ; - refs =files_downcast (ref_store, -REF_STORE_READ | (ref_paranoia ?0 :REF_STORE_ODB ), -"ref_iterator_begin" ); + refs =files_downcast (ref_store, required_flags,"ref_iterator_begin" ); iter =xcalloc (1 ,sizeof (*iter)); ref_iterator = &iter->base;
Signature · 1 line
2.11.0
PATCH v2 23/25 create_ref_entry(): remove `check_name` option
In reply to Michael HaggertyOnly one caller was using it, so move the check to that caller.
--- refs/files-backend.c | 12 ++++++++---- refs/ref-cache.c | 6 +----- refs/ref-cache.h | 3 +-- 3 files changed, 10 insertions(+), 11 deletions(-)
refs/files-backend.c +8 -4
@@ -291,7 +291,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)oidclr (&oid); flag |=REF_BAD_NAME |REF_ISBROKEN ; } - last =create_ref_entry (refname, &oid, flag,0 ); + last =create_ref_entry (refname, &oid, flag);if (peeled ==PEELED_FULLY || (peeled ==PEELED_TAGS &&starts_with (refname,"refs/tags/" ))) last->flag |=REF_KNOWS_PEELED ;
@@ -415,8 +415,12 @@ static void add_packed_ref(struct files_ref_store *refs,if (!is_lock_file_locked (&refs->packed_refs_lock))die ("BUG: packed refs not locked" ); + +if (check_refname_format (refname,REFNAME_ALLOW_ONELEVEL )) +die ("Reference has invalid format: '%s'" , refname); +add_ref_entry (get_packed_ref_dir (packed_ref_cache), -create_ref_entry (refname, oid,REF_ISPACKED ,1 )); +create_ref_entry (refname, oid,REF_ISPACKED )); }/*
@@ -493,7 +497,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store, flag |=REF_BAD_NAME |REF_ISBROKEN ; }add_entry_to_dir (dir, -create_ref_entry (refname.buf, &oid, flag,0 )); +create_ref_entry (refname.buf, &oid, flag)); }strbuf_setlen (&refname, dirnamelen);strbuf_setlen (&path, path_baselen);
@@ -1541,7 +1545,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)oidcpy (&packed_entry->u.value.oid, iter->oid); }else { packed_entry =create_ref_entry (iter->refname, iter->oid, -REF_ISPACKED ,0 ); +REF_ISPACKED );add_ref_entry (packed_refs, packed_entry); }oidclr (&packed_entry->u.value.peeled);
refs/ref-cache.c +1 -5
@@ -32,14 +32,10 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry) }struct ref_entry *create_ref_entry (const char *refname, -const struct object_id *oid,int flag, -int check_name) +const struct object_id *oid,int flag) {struct ref_entry *ref; -if (check_name && -check_refname_format (refname,REFNAME_ALLOW_ONELEVEL )) -die ("Reference has invalid format: '%s'" , refname);FLEX_ALLOC_STR (ref, name, refname);oidcpy (&ref->u.value.oid, oid);oidclr (&ref->u.value.peeled);
refs/ref-cache.h +1 -2
@@ -185,8 +185,7 @@ struct ref_entry *create_dir_entry(struct ref_cache *cache, int incomplete);struct ref_entry *create_ref_entry (const char *refname, -const struct object_id *oid,int flag, -int check_name); +const struct object_id *oid,int flag);/* * Return a pointer to a new `ref_cache`. Its top-level starts out
Signature · 1 line
2.11.0
PATCH v2 24/25 ref-filter: limit traversal to prefix
In reply to Michael HaggertyFrom: Jeff King <[email protected]>
When we are matching refnames against a pattern, then we know that the beginning of any refname that can match the pattern has to match the part of the pattern up to the first glob character. For example, if the pattern is refs/heads/foo*bar, then it can only match a reference that has the prefix refs/heads/foo.
So pass that prefix to for_each_fullref_in(). This lets the ref code avoid passing us the full set of refs, and in some cases avoid reading them in the first place.
Note that this applies only when the match_as_path flag is set (i.e., when for-each-ref is the caller), as the matching rules for git-branch and git-tag are subtly different.
This could be generalized to the case of multiple patterns, but (a) it probably doesn't come up that often, and (b) it is more awkward to deal with multiple patterns (e.g., the patterns might not be disjoint). So, since this is just an optimization, punt on the case of multiple patterns.
--- ref-filter.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-)
ref-filter.c +63 -1
@@ -1665,6 +1665,68 @@ static int filter_pattern_match(struct ref_filter *filter, const char *refname)return match_pattern (filter, refname); } +/* +* Find the longest prefix of pattern we can pass to +* `for_each_fullref_in()`, namely the part of pattern preceding the +* first glob character. (Note that `for_each_fullref_in()` is +* perfectly happy working with a prefix that doesn't end at a +* pathname component boundary.) +*/ +static void find_longest_prefix (struct strbuf *out,const char *pattern) +{ +const char *p; + +for (p = pattern; *p && !is_glob_special (*p); p++) + ; + +strbuf_add (out, pattern, p - pattern); +} + +/* +* This is the same as for_each_fullref_in(), but it tries to iterate +* only over the patterns we'll care about. Note that it _doesn't_ do a full +* pattern match, so the callback still has to match each ref individually. +*/ +static int for_each_fullref_in_pattern (struct ref_filter *filter, +each_ref_fn cb, +void *cb_data, +int broken) +{ +struct strbuf prefix =STRBUF_INIT ; +int ret; + +if (!filter->match_as_path) { +/* +* in this case, the patterns are applied after +* prefixes like "refs/heads/" etc. are stripped off, +* so we have to look at everything: +*/ +return for_each_fullref_in ("" , cb, cb_data, broken); + } + +if (!filter->name_patterns[0 ]) { +/* no patterns; we have to look at everything */ +return for_each_fullref_in ("" , cb, cb_data, broken); + } + +if (filter->name_patterns[1 ]) { +/* +* multiple patterns; in theory this could still work as long +* as the patterns are disjoint. We'd just make multiple calls +* to for_each_ref(). But if they're not disjoint, we'd end up +* reporting the same ref multiple times. So let's punt on that +* for now. +*/ +return for_each_fullref_in ("" , cb, cb_data, broken); + } + +find_longest_prefix (&prefix, filter->name_patterns[0 ]); + + ret =for_each_fullref_in (prefix.buf, cb, cb_data, broken); +strbuf_release (&prefix); +return ret; +} +/* * Given a ref (sha1, refname), check if the ref belongs to the array * of sha1s. If the given ref is a tag, check if the given tag points
@@ -1911,7 +1973,7 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned intelse if (filter->kind ==FILTER_REFS_TAGS ) ret =for_each_fullref_in ("refs/tags/" , ref_filter_handler, &ref_cbdata, broken);else if (filter->kind &FILTER_REFS_ALL ) - ret =for_each_fullref_in ("" , ref_filter_handler, &ref_cbdata, broken); + ret =for_each_fullref_in_pattern (filter, ref_filter_handler, &ref_cbdata, broken);if (!ret && (filter->kind &FILTER_REFS_DETACHED_HEAD ))head_ref (ref_filter_handler, &ref_cbdata); }
Signature · 1 line
2.11.0
PATCH v2 25/25 cache_ref_iterator_begin(): avoid priming unneeded directories
In reply to Michael HaggertyWhen iterating over references, reference priming is used to make sure that loose references are read into the ref-cache before packed references, to avoid races. It used to be that the prefix passed to reference iterators almost always ended in /, for example refs/heads/. In that case, the priming code would read all loose references under find_containing_dir("refs/heads/"), which is "refs/heads/". That's just what we want.
But now that ref-filter knows how to pass refname prefixes to for_each_fullref_in(), the prefix might come from user input; for example,
git for-each-ref refs/heads
Since the argument doesn't include a trailing slash, the reference iteration code would prime all of the loose references under find_containing_dir("refs/heads"), which is "refs/". Thus we would unnecessarily read tags, remote-tracking references, etc., when the user is only interested in branches.
It is a bit awkward to get around this problem. We can't just append a slash to the argument, because we don't know ab initio whether an argument like refs/tags/release corresponds to a single tag or to a directory containing tags.
Moreover, until now a prefix_ref_iterator was used to make the final decision about which references fall within the prefix (the cache_ref_iterator only did a rough cut). This is also inefficient, because the prefix_ref_iterator can't know, for example, that while you are in a subdirectory that is completely within the prefix, you don't have to do the prefix check.
So:
* Move the responsibility for doing the prefix check directly to cache_ref_iterator. This means that cache_ref_iterator_begin() never has to wrap its return value in a prefix_ref_iterator.
* Teach cache_ref_iterator_begin() (and prime_ref_dir()) to be stricter about what they iterate over and what directories they prime.
* Teach cache_ref_iterator to keep track of whether the current cache_ref_iterator_level is fully within the prefix. If so, skip the prefix checks entirely.
The main benefit of these optimizations is for loose references, since packed references are always read all at once.
Note that after this change, prefix_ref_iterator is only ever used for its trimming feature and not for its "prefix" feature. But I'm not ripping out the latter yet, because it might be useful for another patch series that I'm working on.
--- refs/ref-cache.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 84 insertions(+), 10 deletions(-)
refs/ref-cache.c +84 -10
@@ -312,11 +312,42 @@ static void sort_ref_dir(struct ref_dir *dir) dir->sorted = dir->nr = i; } +enum prefix_state { +/* All refs within the directory would match prefix: */ +PREFIX_CONTAINS_DIR , + +/* Some, but not all, refs within the directory might match prefix: */ +PREFIX_WITHIN_DIR , + +/* No refs within the directory could possibly match prefix: */ +PREFIX_EXCLUDES_DIR +}; +/* -* Load all of the refs from `dir` (recursively) into our in-memory -* cache. +* Return a `prefix_state` constant describing the relationship +* between the directory with the specified `dirname` and `prefix`. */ -static void prime_ref_dir (struct ref_dir *dir) +static enum prefix_state overlaps_prefix (const char *dirname, +const char *prefix) +{ +while (*prefix && *dirname == *prefix) { + dirname++; + prefix++; + } +if (!*prefix) +return PREFIX_CONTAINS_DIR ; +else if (!*dirname) +return PREFIX_WITHIN_DIR ; +else +return PREFIX_EXCLUDES_DIR ; +} + +/* +* Load all of the refs from `dir` (recursively) that could possibly +* contain references matching `prefix` into our in-memory cache. If +* `prefix` is NULL, prime unconditionally. +*/ +static void prime_ref_dir (struct ref_dir *dir,const char *prefix) {/* * The hard work of loading loose refs is done by get_ref_dir(), so we
@@ -327,8 +358,29 @@ static void prime_ref_dir(struct ref_dir *dir)int i;for (i =0 ; i < dir->nr; i++) {struct ref_entry *entry = dir->entries[i]; -if (entry->flag &REF_DIR ) -prime_ref_dir (get_ref_dir (entry)); +if (!(entry->flag &REF_DIR )) { +/* Not a directory; no need to recurse. */ + }else if (!prefix) { +/* Recurse in any case: */ +prime_ref_dir (get_ref_dir (entry),NULL ); + }else { +switch (overlaps_prefix (entry->name, prefix)) { +case PREFIX_CONTAINS_DIR : +/* +* Recurse, and from here down we +* don't have to check the prefix +* anymore: +*/ +prime_ref_dir (get_ref_dir (entry),NULL ); +break ; +case PREFIX_WITHIN_DIR : +prime_ref_dir (get_ref_dir (entry), prefix); +break ; +case PREFIX_EXCLUDES_DIR : +/* No need to prime this directory. */ +break ; + } + } } }
@@ -343,6 +395,8 @@ struct cache_ref_iterator_level {*/ struct ref_dir *dir; +enum prefix_state prefix_state; +/* * The index of the current entry within dir (which might * itself be a directory). If index == -1, then the iteration
@@ -369,6 +423,13 @@ struct cache_ref_iterator {/* The number of levels that have been allocated on the stack */ size_t levels_alloc; +/* +* Only include references with this prefix in the iteration. +* The prefix is matched textually, without regard for path +* component boundaries. +*/ +const char *prefix; +/* * A stack of levels. levels[0] is the uppermost level that is * being iterated over in this iteration. (This is not
@@ -390,6 +451,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) &iter->levels[iter->levels_nr -1 ];struct ref_dir *dir = level->dir;struct ref_entry *entry; +enum prefix_state entry_prefix_state;if (level->index ==-1 )sort_ref_dir (dir);
@@ -404,6 +466,14 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) entry = dir->entries[level->index]; +if (level->prefix_state ==PREFIX_WITHIN_DIR ) { + entry_prefix_state =overlaps_prefix (entry->name, iter->prefix); +if (entry_prefix_state ==PREFIX_EXCLUDES_DIR ) +continue ; + }else { + entry_prefix_state = level->prefix_state; + } +if (entry->flag &REF_DIR ) {/* push down a level */ ALLOC_GROW(iter->levels, iter->levels_nr + 1,
@@ -411,6 +481,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator) level = &iter->levels[iter->levels_nr++]; level->dir =get_ref_dir (entry); + level->prefix_state = entry_prefix_state; level->index =-1 ; }else { iter->base.refname = entry->name;
@@ -496,10 +567,10 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, dir =find_containing_dir (dir, prefix,0 );if (!dir)/* There's nothing to iterate over. */ -return empty_ref_iterator_begin (); +return empty_ref_iterator_begin ();if (prime_dir) -prime_ref_dir (dir); +prime_ref_dir (dir, prefix); iter =xcalloc (1 ,sizeof (*iter)); ref_iterator = &iter->base;
@@ -511,9 +582,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, level->index =-1 ; level->dir = dir; -if (prefix && *prefix) - ref_iterator =prefix_ref_iterator_begin (ref_iterator, - prefix,0 ); +if (prefix && *prefix) { + iter->prefix =xstrdup (prefix); + level->prefix_state =PREFIX_WITHIN_DIR ; + }else { + level->prefix_state =PREFIX_CONTAINS_DIR ; + }return ref_iterator; }
Signature · 1 line
2.11.0
Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories
Quoted 6 lines · Michael Haggerty
So:
* Move the responsibility for doing the prefix check directly to
cache_ref_iterator. This means thatcache_ref_iterator_begin()never has to wrap its return value in aprefix_ref_iterator.
Quoted 7 lines
* Teach
cache_ref_iterator_begin()(andprime_ref_dir()) to be stricter about what they iterate over and what directories they prime.* Teach
cache_ref_iteratorto keep track of whether the currentcache_ref_iterator_levelis fully within the prefix. If so, skip the prefix checks entirely.
As promised, I came back to this one with a more careful eye. These changes all make sense to me, and the implementation matches.
My only question is:
Review struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, · line 590
Quoted 13 lines
(quoted diff) +6
-3@@ -511,9 +582,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, level->index = -1; level->dir = dir; - if (prefix && *prefix) - ref_iterator = prefix_ref_iterator_begin(ref_iterator, - prefix, 0); + if (prefix && *prefix) { + iter->prefix = xstrdup(prefix); + level->prefix_state = PREFIX_WITHIN_DIR; + } else { + level->prefix_state = PREFIX_CONTAINS_DIR; + }
Who frees the prefix? Does this need:
refs/ref-cache.c +1 -0
@@ -542,6 +542,7 @@ static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)struct cache_ref_iterator *iter = (struct cache_ref_iterator *)ref_iterator; +free (iter->prefix);free (iter->levels);base_ref_iterator_free (ref_iterator);return ITER_DONE ;
-Peff
Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories
Quoted 42 lines · Jeff King
Quoted 6 lines · Michael Haggerty
So:
* Move the responsibility for doing the prefix check directly to
cache_ref_iterator. This means thatcache_ref_iterator_begin()never has to wrap its return value in aprefix_ref_iterator.Quoted 7 lines
* Teach
cache_ref_iterator_begin()(andprime_ref_dir()) to be stricter about what they iterate over and what directories they prime.* Teach
cache_ref_iteratorto keep track of whether the currentcache_ref_iterator_levelis fully within the prefix. If so, skip the prefix checks entirely.As promised, I came back to this one with a more careful eye. These changes all make sense to me, and the implementation matches.
My only question is:
Review struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, · line 590
Quoted 13 lines
(quoted diff) +6
-3@@ -511,9 +582,12 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, level->index = -1; level->dir = dir; - if (prefix && *prefix) - ref_iterator = prefix_ref_iterator_begin(ref_iterator, - prefix, 0); + if (prefix && *prefix) { + iter->prefix = xstrdup(prefix); + level->prefix_state = PREFIX_WITHIN_DIR; + } else { + level->prefix_state = PREFIX_CONTAINS_DIR; + }Who frees the prefix? Does this need:
refs/ref-cache.c +0
-0
refs/ref-cache.c static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator) · line 548
Quoted 7 lines
refs/ref-cache.c +1
-0@@ -542,6 +542,7 @@ static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)struct cache_ref_iterator *iter = (struct cache_ref_iterator *)ref_iterator; +free (iter->prefix);free (iter->levels);base_ref_iterator_free (ref_iterator);return ITER_DONE ;
Yes, you are right. Thanks for catching this.
Note: it has to be
free((char *)iter->prefix);
because prefix is const.
Junio, if a reroll is not needed for other reasons, would you mind squashing this into the last patch of my series?
Michael
Re: [PATCH v2 25/25] cache_ref_iterator_begin(): avoid priming unneeded directories
Quoted 2 lines · Michael Haggerty
Junio, if a reroll is not needed for other reasons, would you mind squashing this into the last patch of my series?
Will do, but won't happen until morning in Tokyo. I've just finished today's integration cycle.
Thanks.
Quoted 48 lines · Michael Haggerty
This is the second iteration of a patch series that prepares the ground for separating out a
packed_ref_storeand then for changingpacked-refsto be read usingmmap(). Thanks to Peff, Junio, Stefan, Brandon, and Johannes for their feedback about v1 [1]. I think I have addressed all of your comments.Changes since v1:
* Since v1, branch
bc/object-idhas been merged tonext, and it has lots of conflicts with these changes. So I rebased this branch onto a merge ofmasterandbc/object-id. (I hope this makes Junio's job easier.) This unfortunately causes a bit of tbdiff noise between v1 and v2.* Patch [01/25]: in t3600, register the
test_when_finishedcommand before executingchmod a-w.* Patch [04/25] (new patch): convert a few
die("internal error: ...")todie("BUG: ...").* Patch [05/25]: Use
strlen()rather thanmemchr()to check the trim length, anddie()rather than skipping if it is longer than the reference name.* Patch [08/25]: Name the log message arguments
msgfor consistency with existing code.* Patch [10/25]: Rename the new member from
packlocktopacked_refs_lock.* Patch [13/25] (new patch): Move the check for valid
transaction->statefromfiles_transaction_commit()toref_transaction_commit().* Patch [14/25]:
* Add more sanity checks of
transaction->state.* Don't add
ref_transaction_finish()to the public API. Instead, teachref_transaction_commit()to do the right thing whether or notref_transaction_prepare()has been called.* Add and improve docstrings.
* Allow
ref_transaction_abort()to be called even beforeref_transaction_prepare()(in which case it just callsref_transaction_free()).
Quoted 8 lines
* Lots of improvements to commit messages and comments, mostly to clarify points that reviewers asked about.
These changes (along with the merge commit that they are based on) are also available as branch
packed-ref-store-prepin my GitHub fork [2]. If you'd like to see a preview of the rest of the changes (which works but is not yet polished), checkout themmap-packed-refsbranch from the same place.
I have read this version and quite like it. I did not have any nits to remark.
Thanks,
Stefan
Quoted 5 lines · Michael Haggerty
* Since v1, branch
bc/object-idhas been merged tonext, and it has lots of conflicts with these changes. So I rebased this branch onto a merge ofmasterandbc/object-id. (I hope this makes Junio's job easier.) This unfortunately causes a bit of tbdiff noise between v1 and v2.
Heh, that gives me an excellent excuse to procrastinate, as I am planning to merge that topic to 'master' by the end of this week ;-)
Jokes aside...
Quoted 13 lines
* Patch [01/25]: in t3600, register the
test_when_finishedcommand before executingchmod a-w.* Patch [04/25] (new patch): convert a few
die("internal error: ...")todie("BUG: ...").* Patch [05/25]: Use
strlen()rather thanmemchr()to check the trim length, anddie()rather than skipping if it is longer than the reference name.* Patch [08/25]: Name the log message arguments
msgfor consistency with existing code.
Quoted 6 lines
* Patch [10/25]: Rename the new member from
packlocktopacked_refs_lock.* Patch [13/25] (new patch): Move the check for valid
transaction->statefromfiles_transaction_commit()toref_transaction_commit().
All of these feel familiar ;-)
Quoted 8 lines · Patch
* Add more sanity checks of
transaction->state.* Don't add
ref_transaction_finish()to the public API. Instead, teachref_transaction_commit()to do the right thing whether or notref_transaction_prepare()has been called.
Quoted 8 lines
* Add and improve docstrings.
* Allow
ref_transaction_abort()to be called even beforeref_transaction_prepare()(in which case it just callsref_transaction_free()).* Lots of improvements to commit messages and comments, mostly to clarify points that reviewers asked about.
Overall this looked like quite a quality work to me.
Quoted 5 lines
These changes (along with the merge commit that they are based on) are also available as branch
packed-ref-store-prepin my GitHub fork [2]. If you'd like to see a preview of the rest of the changes (which works but is not yet polished), checkout themmap-packed-refsbranch from the same place.
Thanks.