Re: [PATCH] push: fix local refs update if already up-to-date

On Tue, Nov 04, 2008 at 09:49:32PM -0500, Jeff King wrote:
[...]

However, I would like to make one additional request.  Since you are
killing off all usage of new_sha1 initial assignment, I think it makes
sense to just get rid of the variable entirely, so it cannot create
confusion later.

Ok, I can live with that.

quoted

quoted

Hmm. I was hoping to see more in update_tracking_ref. With your patch,
we end up calling update_ref for _every_ uptodate ref, which results in
writing a new unpacked ref file for each one. And that _is_ a
performance problem for people with large numbers of refs.

[...]
I think update_ref already takes care of that. See this check in
write_ref_sha1:

        if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
                unlock_ref(lock);
                return 0;
        }

Nope. That check is a concurrency safeguard. It checks that when we are
moving the ref from "A" to "B", that the ref still _is_ "A" when we lock
it.

I think you are confusing this with verify_lock(). The code in
write_ref_sha1() really does compare with the new sha1.

But more importantly, it is easy to demonstrate the problem with your
patch:

  mkdir parent &&
    (cd parent &&
       git init && touch file && git add file && git commit -m one) &&
  git clone parent child &&
    (cd child &&
       echo BEFORE: && ls -l .git/refs/remotes/origin &&
       git push &&
       echo AFTER:  && ls -l .git/refs/remotes/origin)

I get:

  BEFORE:
  -rw-r--r-- 1 peff peff 32 2008-11-04 21:43 HEAD
  Everything up-to-date
  AFTER:
  -rw-r--r-- 1 peff peff 32 2008-11-04 21:43 HEAD
  -rw-r--r-- 1 peff peff 41 2008-11-04 21:43 master

Oops. With the patch snippet I posted in my previous message, the
'master' ref is not created by the uptodate push.

The reason it doesn't work is a bug in lock_ref_sha1_basic(). Dating back to
pre-"pack-refs" times, this code forces a write if the ref file does not
exist. I will resubmit the patch including your above testcase and a bugfix
for lock_ref_sha1_basic().

Clemens