On Tue, Nov 04, 2008 at 09:56:30AM +0100, Clemens Buchacher wrote:
> 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.
I have to admit I did not exhaustively look at all of the status cases
when I reviewed earlier, and there are fewer than I realized. So I think
your change is reasonable.
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. Like this (on top of your patch):
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 4c17f48..d139eba 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -435,16 +435,13 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
*/
new_refs = 0;
for (ref = remote_refs; ref; ref = ref->next) {
- const unsigned char *new_sha1;
-
if (!ref->peer_ref) {
if (!args.send_mirror)
continue;
- new_sha1 = null_sha1;
+ hashcpy(ref->new_sha1, null_sha1);
}
else
- new_sha1 = ref->peer_ref->new_sha1;
- hashcpy(ref->new_sha1, new_sha1);
+ hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
ref->deletion = is_null_sha1(ref->new_sha1);
if (ref->deletion && !allow_deleting_refs) {
> > 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.
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.
> > 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.
No, you can't avoid it (without totally giving up on your patch's goal,
which I think is a good one). So I think it is worth it, and I was just
being paranoid about hurting performance. Even with packed refs, I think
we do still end up stat()ing for each ref, but we will have to live with
it. I was thinking we might be able to do something clever with values
we had already read for the push, but it is impossible: we have read the
refs we are going to _push_, but we have not looked at the remote
tracking branches, which are what contain the interesting information.
-Peff
--
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
