Re: [PATCH 00/20] Read `packed-refs` using mmap()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Michael,

On Wed, 13 Sep 2017, Michael Haggerty wrote:

> * `mmap()` the whole file rather than `read()`ing it.

On Windows, a memory-mapped file cannot be renamed. As a consequence, the
following tests fail on `pu`:

t1400-update-ref.sh
t1404-update-ref-errors.sh
t1405-main-ref-store.sh
t1408-packed-refs.sh
t1410-reflog.sh
t1430-bad-ref-name.sh
t1450-fsck.sh
t1507-rev-parse-upstream.sh
t2018-checkout-branch.sh
t2020-checkout-detach.sh
t2024-checkout-dwim.sh
t3200-branch.sh
t3204-branch-name-interpretation.sh
t3210-pack-refs.sh
t3211-peel-ref.sh
t3306-notes-prune.sh
t3400-rebase.sh
t3404-rebase-interactive.sh
t3420-rebase-autostash.sh
t3903-stash.sh
t3905-stash-include-untracked.sh
t4151-am-abort.sh
t5304-prune.sh
t5312-prune-corruption.sh
t5400-send-pack.sh
t5404-tracking-branches.sh
t5500-fetch-pack.sh
t5505-remote.sh
t5510-fetch.sh
t5514-fetch-multiple.sh
t5515-fetch-merge-logic.sh
t5516-fetch-push.sh
t5520-pull.sh
t5533-push-cas.sh
t5572-pull-submodule.sh
t5600-clone-fail-cleanup.sh
t5607-clone-bundle.sh
t6016-rev-list-graph-simplify-history.sh
t6030-bisect-porcelain.sh
t6032-merge-large-rename.sh
t6040-tracking-info.sh
t6050-replace.sh
t6500-gc.sh
t6501-freshen-objects.sh
t7003-filter-branch.sh
t7004-tag.sh
t7102-reset.sh
t7201-co.sh
t7406-submodule-update.sh
t7504-commit-msg-hook.sh
t7701-repack-unpack-unreachable.sh
t9300-fast-import.sh
t9902-completion.sh
t9903-bash-prompt.sh

This diff:

-- snip --
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f9c5e771bdd..ee8b3603624 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1306,13 +1308,13 @@ static int packed_transaction_finish(struct
ref_store *ref_store,
 	char *packed_refs_path;
 
 	packed_refs_path = get_locked_file_path(&refs->lock);
+	clear_snapshot(refs);
 	if (rename_tempfile(&refs->tempfile, packed_refs_path)) {
 		strbuf_addf(err, "error replacing %s: %s",
 			    refs->path, strerror(errno));
 		goto cleanup;
 	}
 
-	clear_snapshot(refs);
 	ret = 0;
 
 cleanup:
-- snap --

reduces the failed tests to

t1410-reflog.counts.sh
t3210-pack-refs.counts.sh
t3211-peel-ref.counts.sh
t5505-remote.counts.sh
t5510-fetch.counts.sh
t5600-clone-fail-cleanup.counts.sh

However, much as I tried to wrap my head around it, I could not even start
to come up with a solution for e.g. the t1410 regression. The failure
happens in `git branch -d one/two`.

The culprit: there is yet another unreleased snapshot while we try to
finish the transaction.

It is the snapshot of the main_worktree_ref_store as acquired by
add_head_info() (which is called from get_main_worktree(), which in turn
was called from get_worktrees(), in turn having been called from
find_shared_symref() that was called from delete_branches()), and it seems
to me that there was never any plan to have that released, including its
snapshot.

And the snapshot gets initialized upon add_head_info() calling
refs_resolve_ref_unsafe().

Further down in the delete_branches() function, however, we call
delete_ref() which in turn wants to overwrite the packed-refs file via an
atomic rename. That rename fails now because there is still that main
worktree's ref_store that has the snapshot mmap()ed .

I imagine that the rest of the test failures stems from the same root
cause.

Do you have any idea how to ensure that all mmap()ed packed refs are
released before attempting to finish a transaction?

Ciao,
Dscho