push: fix local refs update if already up-to-date

git push normally updates local refs only after a successful push. If the remote already has the updates -- pushed indirectly through another repository, for example -- we forget to update local tracking refs.

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.

+test_expect_success 'push updates local refs (2)' '

Quoted 3 lines

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.

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.

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:

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

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.

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

Quoted 4 lines

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.

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.

Quoted 8 lines

Quoted 4 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:

Quoted 4 lines

        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.

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.

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

Quoted 4 lines

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.

Quoted 6 lines · Clemens Buchacher

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

Quoted 4 lines

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.

Considering that the ref is initialized to the null_sha1, do you think it would be Ok to do the following instead?

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

Nit: Just reading the test, it is hard to see what is interesting about it (though obviously I can blame it back to your commit :) ). Maybe a more descriptive title like 'push updates uptodate local refs' would make sense.

Quoted 4 lines · Jeff King

Nit: Just reading the test, it is hard to see what is interesting about it (though obviously I can blame it back to your commit :) ). Maybe a more descriptive title like 'push updates uptodate local refs' would make sense.

That is all I changed in this update. Pending an Ack/Nack from Jeff I feel that I'm done.

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

Quoted 4 lines · Jeff King

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

It is either (1) the user pays the cost of finding what remote tracking branch we are mirroring when you push for all up-to-date refs, like you did in your "here is an improvement" patch; or (2) the user pays the cost of fetching from there, immediately after pushing. I'd imagine that the cost to do (1) would be smaller than (2). The question is if seeing stale tracking branches is such a big deal, as next "git fetch" from there will update them anyway. If it is a big deal, (1) would be a price worth paying.

Clemens, care to reroll the patch?