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.
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
