On Mon, May 06, 2013 at 02:41:22PM -0400, Jeff King wrote:

> That is a weaker guarantee, and I think we can provide it with:
>   1. Load all loose refs into cache for a particular enumeration.
>   2. Make sure the packed-refs cache is up-to-date (by checking its
>      stat() information and reloading if necessary).
>   3. Run the usual iteration over the loose/packed ref caches.

This does seem to work in my experiments. With stock git, I can trigger
the race reliably with:

  # base load, in one terminal, as before
  git init -q repo &&
  cd repo &&
  git commit -q --allow-empty -m one &&
  one=`git rev-parse HEAD` &&
  git commit -q --allow-empty -m two &&
  two=`git rev-parse HEAD` &&
  sha1=$one &&
  while true; do
    # this re-creates the loose ref in .git/refs/heads/master
    if test "$sha1" = "$one"; then
    fi &&
    git update-ref refs/heads/master $sha1 &&

    # we can remove packed-refs safely, as we know that
    # its only value is now stale. Real git would not do
    # this, but we are simulating the case that "master"
    # simply wasn't included in the last packed-refs file.
    rm -f .git/packed-refs &&

    # and now we repack, which will create an up-to-date
    # packed-refs file, and then delete the loose ref
    git pack-refs --all --prune

  # in another terminal, enumerate and make sure we never miss the ref
  cd repo &&
  while true; do
    refs=`git.compile for-each-ref --format='%(refname)'`
    echo "==> $refs"
    test -z "$refs" && break

It usually takes about 30 seconds to hit a problem, though I measured
failures at up to 90 seconds. With the patch below (on top of the one I
posted the other day, which refreshes the packed-refs cache in
get_packed_refs), it has been running fine for 15 minutes.

The "noop_each_fn" is a little gross. I could also just reimplement the
recursion from do_for_each_ref_in_dir (except we don't care about flags,
trim, base, etc, and would just be calling get_ref_dir recursively).
It's a slight repetition of code, but it would be less subtle than what
I have written below (which uses a no-op callback for the side effect
that it primes the loose ref cache). Which poison do you prefer?

diff --git a/refs.c b/refs.c
index 45a7ee6..59ae7e4 100644
--- a/refs.c
+++ b/refs.c
@@ -1363,19 +1363,38 @@ static int do_for_each_ref(const char *submodule, const 
char *base, each_ref_fn
        for_each_rawref(warn_if_dangling_symref, &data);
+static int noop_each_fn(const char *ref, const unsigned char *sha1, int flags,
+                       void *data)
+       return 0;
 static int do_for_each_ref(const char *submodule, const char *base, 
each_ref_fn fn,
                           int trim, int flags, void *cb_data)
        struct ref_cache *refs = get_ref_cache(submodule);
-       struct ref_dir *packed_dir = get_packed_refs(refs);
-       struct ref_dir *loose_dir = get_loose_refs(refs);
+       struct ref_dir *packed_dir;
+       struct ref_dir *loose_dir;
        int retval = 0;
-       if (base && *base) {
-               packed_dir = find_containing_dir(packed_dir, base, 0);
+       /*
+        * Prime the loose ref cache; we must make sure the packed ref cache is
+        * uptodate after we read the loose refs in order to avoid race
+        * conditions with a simultaneous "pack-refs --prune".
+        */
+       loose_dir = get_loose_refs(refs);
+       if (base && *base)
                loose_dir = find_containing_dir(loose_dir, base, 0);
+       if (loose_dir) {
+               sort_ref_dir(loose_dir);
+               do_for_each_ref_in_dir(loose_dir, 0, base, noop_each_fn, 0, 0,
+                                      NULL);
+       packed_dir = get_packed_refs(refs);
+       if (base && *base)
+               packed_dir = find_containing_dir(packed_dir, base, 0);
        if (packed_dir && loose_dir) {

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to