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

From: Clemens Buchacher <drizzd@aon.at>
Date: Nov 5, 2008, 20:28 UTC
Message-ID: <20081105202849.GA9484@localhost>
In-Reply-To: <20081105024932.GA20907@coredump.intra.peff.net>

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.

Show 16 quoted lines

> > > 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.

Show 23 quoted lines

> 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