path: root/refs
diff options
authorSun Chao <>2019-07-31 18:35:44 (GMT)
committerJunio C Hamano <>2019-08-02 16:59:05 (GMT)
commita613d4f8176593e560628b174c3e10cfac3f6d58 (patch)
tree930656d2e840ef540e2cb7f2eb94e2a8dd6ad3a0 /refs
parent4d8ec15c66f32e29e4dfcdf64d0826c425d8ad7d (diff)
pack-refs: always refresh after taking the lock file
When a packed ref is deleted, the whole packed-refs file is rewritten to omit the ref that no longer exists. However if another gc command is running and calls `pack-refs --all` simultaneously, there is a chance that a ref that was just updated lose the newly created commits. Through these steps, losing commits on newly updated refs can be demonstrated: # step 1: compile git without `USE_NSEC` option Some kernel releases do enable it by default while some do not. And if we compile git without `USE_NSEC`, it will be easier demonstrated by the following steps. # step 2: setup a repository and add the first commit git init repo && (cd repo && git config core.logallrefupdates true && git commit --allow-empty -m foo) # step 3: in one terminal, repack the refs repeatedly cd repo && while true do git pack-refs --all done # step 4: in another terminal, simultaneously update the # master with update-ref, and create and delete an # unrelated ref also with update-ref cd repo && while true do us=$(git commit-tree -m foo -p HEAD HEAD^{tree}) && git update-ref refs/heads/newbranch $us && git update-ref refs/heads/master $us && git update-ref -d refs/heads/newbranch && them=$(git rev-parse master) && if test "$them" != "$us" then echo >&2 "lost commit: $us" exit 1 fi # eye candy printf . done Though we have the packed-refs lock file and loose refs lock files to avoid updating conflicts, a ref will lost its newly commits if racy stat-validity of `packed-refs` file happens (which is quite same as the racy-git described in `Documentation/technical/racy-git.txt`), the following specific set of operations demonstrates the problem: 1. Call `pack-refs --all` to pack all the loose refs to packed-refs, and let say the modify time of the packed-refs is DATE_M. 2. Call `update-ref` to update a new commit to master while it is already packed. the old value (let us call it OID_A) remains in the packed-refs file and write the new value (let us call it OID_B) to $GIT_DIR/refs/heads/master. 3. Call `update-ref -d` within the same DATE_M from the 1th step to delete a different ref newbranch which is packed in the packed-refs file. It check newbranch's oid from packed-refs file without locking it. Meanwhile it keeps a snapshot of the packed-refs file in memory and record the file's attributes with the snapshot. The oid of master in the packed-refs's snapshot is OID_A. 4. Call a new `pack-refs --all` to pack the loose refs, the oid of master in packe-refs file is OID_B, and the loose refs $GIT_DIR/refs/heads/master is removed. Let's say the `pack-refs --all` is very quickly done and the new packed-refs file's modify time is still DATE_M, and it has the same file size, even the same inode. 5. 3th step now goes on after checking the newbranch, it begin to rewrite the packed-refs file. After get the lock file of packed-ref file, it checks it's on-disk file attributes with the snapshot, suck as the timestamp, the file size and the inode value. If they are both the same values, and the snapshot is not refreshed. Because the loose ref of master is removed by 4th step, `update-ref -d` will updates the new packed-ref to disk which contains master with the oid OID_A. So now the newly commit OID_B of master is lost. The best path forward is just always refreshing after take the lock file of `packed-refs` file. Traditionally we avoided that because refreshing it implied parsing the whole file. But these days we mmap it, so it really is just an extra open()/mmap() and a quick read of the header. That doesn't seem like an outrageous cost to pay when we're already taking the lock. Signed-off-by: Sun Chao <> Signed-off-by: Jeff King <> Signed-off-by: Sun Chao <> Signed-off-by: Junio C Hamano <>
Diffstat (limited to 'refs')
1 files changed, 16 insertions, 7 deletions
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index c01c7f5..4458a0f 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1012,14 +1012,23 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
- * Now that we hold the `packed-refs` lock, make sure that our
- * snapshot matches the current version of the file. Normally
- * `get_snapshot()` does that for us, but that function
- * assumes that when the file is locked, any existing snapshot
- * is still valid. We've just locked the file, but it might
- * have changed the moment *before* we locked it.
+ * There is a stat-validity problem might cause `update-ref -d`
+ * lost the newly commit of a ref, because a new `packed-refs`
+ * file might has the same on-disk file attributes such as
+ * timestamp, file size and inode value, but has a changed
+ * ref value.
+ *
+ * This could happen with a very small chance when
+ * `update-ref -d` is called and at the same time another
+ * `pack-refs --all` process is running.
+ *
+ * Now that we hold the `packed-refs` lock, it is important
+ * to make sure we could read the latest version of
+ * `packed-refs` file no matter we have just mmap it or not.
+ * So what need to do is clear the snapshot if we hold it
+ * already.
- validate_snapshot(refs);
+ clear_snapshot(refs);
* Now make sure that the packed-refs file as it exists in the