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

From: Clemens Buchacher <drizzd@aon.at>
Date: Nov 4, 2008, 08:56 UTC
Message-ID: <20081104085630.GA22530@localhost>
In-Reply-To: <20081104042643.GA31276@coredump.intra.peff.net>

On Mon, Nov 03, 2008 at 11:26:44PM -0500, Jeff King wrote:

> > The hashcpy for new_ref is now executed more often than absolutely
> > necessary. But this is not a critical path, right? So I decided to keep
> > things simple.
>

[...]

Show 8 quoted lines

> Your patch makes ref->new_sha1 "valid" for every status case. Ordinarily
> I would be in favor of that, since it reduces coupling with other parts
> of the code (which have to know _which_ status flags provide a useful
> value in ->new_sha1). But in this case, I think the value we would be
> sticking in is not necessarily useful for every status flag we end up
> setting; so any consumers of the ref structure still need to know which
> flags set it. So even though it has a defined value, it is not really
> "valid" in all cases.

The other status flags are REF_STATUS_REJECT_NODELETE and REF_STATUS_REJECT_NONFASTFORWARD. So in these cases the "new sha1" is going to be the "old sha1". The default for new_sha1 is the null sha1. So while the sha1 we're trying to push may not be more valid than the null sha1, it's not less valid either, is it? And it even makes sense if you interpret new_sha1 as the sha1 the client attempts to push.

Show 7 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.
> 
> So I think we need a check to make sure we aren't just updating with the
> same value. Something like:

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;
        }

> Though I am not happy that we have to look up the tracking ref for every
> uptodate ref. I think it shouldn't be a big performance problem with
> packed refs, though, since they are cached (i.e., we pay only to compare
> the hashes, not touch the filesystem for each ref).

I don't think we can avoid that, though.

I agree with your other comments.