{"thread":{"id":"16158","subject":"[PATCH] push: fix local refs update if already up-to-date","startedAt":"2008-11-04T00:07:45Z","lastAt":"2008-11-05T22:44:51Z","messageCount":14,"participants":["Clemens Buchacher","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"94829","messageId":"20081104000745.GA28480@localhost","threadId":"16158","inReplyTo":null,"subject":"[PATCH] push: fix local refs update if already up-to-date","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-11-04T00:07:45Z","receivedAt":"2008-11-04T00:07:45Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"git push normally updates local refs only after a successful push. If the\nremote already has the updates -- pushed indirectly through another repository,\nfor example -- we forget to update local tracking refs.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nThe hashcpy for new_ref is now executed more often than absolutely\nnecessary. But this is not a critical path, right? So I decided to keep\nthings simple.\n\n builtin-send-pack.c   |   11 +++++------\n t/t5516-fetch-push.sh |   18 ++++++++++++++++++\n 2 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 910db92..66ad492 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -224,7 +224,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)\n {\n \tstruct refspec rs;\n \n-\tif (ref->status != REF_STATUS_OK)\n+\tif (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)\n \t\treturn;\n \n \trs.src = ref->name;\n@@ -438,15 +438,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\t}\n \t\telse\n \t\t\tnew_sha1 = ref->peer_ref->new_sha1;\n+\t\thashcpy(ref->new_sha1, new_sha1);\n \n-\n-\t\tref->deletion = is_null_sha1(new_sha1);\n+\t\tref->deletion = is_null_sha1(ref->new_sha1);\n \t\tif (ref->deletion && !allow_deleting_refs) {\n \t\t\tref->status = REF_STATUS_REJECT_NODELETE;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!ref->deletion &&\n-\t\t    !hashcmp(ref->old_sha1, new_sha1)) {\n+\t\t    !hashcmp(ref->old_sha1, ref->new_sha1)) {\n \t\t\tref->status = REF_STATUS_UPTODATE;\n \t\t\tcontinue;\n \t\t}\n@@ -474,14 +474,13 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\t    !ref->deletion &&\n \t\t    !is_null_sha1(ref->old_sha1) &&\n \t\t    (!has_sha1_file(ref->old_sha1)\n-\t\t      || !ref_newer(new_sha1, ref->old_sha1));\n+\t\t      || !ref_newer(ref->new_sha1, ref->old_sha1));\n \n \t\tif (ref->nonfastforward && !ref->force && !args.force_update) {\n \t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\n \t\t\tcontinue;\n \t\t}\n \n-\t\thashcpy(ref->new_sha1, new_sha1);\n \t\tif (!ref->deletion)\n \t\t\tnew_refs++;\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f0030ad..d211f93 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -437,6 +437,24 @@ test_expect_success 'push updates local refs' '\n \n '\n \n+test_expect_success 'push updates local refs (2)' '\n+\n+\trm -rf parent child &&\n+\tmkdir parent &&\n+\t(cd parent && git init &&\n+\t\techo one >foo && git add foo && git commit -m one) &&\n+\tgit clone parent child1 &&\n+\tgit clone parent child2 &&\n+\t(cd child1 &&\n+\t\techo two >foo && git commit -a -m two &&\n+\t\tgit push) &&\n+\t(cd child2 &&\n+\t\tgit pull ../child1 master &&\n+\t\tgit push &&\n+\ttest $(git rev-parse master) = $(git rev-parse remotes/origin/master))\n+\n+'\n+\n test_expect_success 'push does not update local refs on failure' '\n \n \trm -rf parent child &&\n-- \n1.6.0.3\n"},{"id":"94847","messageId":"20081104042643.GA31276@coredump.intra.peff.net","threadId":"16158","inReplyTo":"20081104000745.GA28480@localhost","subject":"Re: [PATCH] push: fix local refs update if already up-to-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-11-04T04:26:44Z","receivedAt":"2008-11-04T04:26:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 04, 2008 at 01:07:45AM +0100, Clemens Buchacher wrote:\n\n> git push normally updates local refs only after a successful push. If\n> the remote already has the updates -- pushed indirectly through\n> another repository, for example -- we forget to update local tracking\n> refs.\n\nI think this goal is a good enhancement.\n\n> The hashcpy for new_ref is now executed more often than absolutely\n> necessary. But this is not a critical path, right? So I decided to keep\n> things simple.\n\nNo, I don't think the loop is tight enough to care about an extra\nhashcpy. The minimally invasive change would be to just set\nref->new_sha1 in the UPTODATE code path. IOW, just:\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 298bd71..b8788f2 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -454,6 +454,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\tif (!ref->deletion &&\n \t\t    !hashcmp(ref->old_sha1, new_sha1)) {\n \t\t\tref->status = REF_STATUS_UPTODATE;\n+\t\t\thashcpy(ref->new_sha1, new_sha1);\n \t\t\tcontinue;\n \t\t}\n \n\nYour patch makes ref->new_sha1 \"valid\" for every status case. Ordinarily\nI would be in favor of that, since it reduces coupling with other parts\nof the code (which have to know _which_ status flags provide a useful\nvalue in ->new_sha1). But in this case, I think the value we would be\nsticking in is not necessarily useful for every status flag we end up\nsetting; so any consumers of the ref structure still need to know which\nflags set it. So even though it has a defined value, it is not really\n\"valid\" in all cases.\n\n> @@ -224,7 +224,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)\n>  {\n>  \tstruct refspec rs;\n>  \n> -\tif (ref->status != REF_STATUS_OK)\n> +\tif (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)\n>  \t\treturn;\n>  \n>  \trs.src = ref->name;\n\nHmm. I was hoping to see more in update_tracking_ref. With your patch,\nwe end up calling update_ref for _every_ uptodate ref, which results in\nwriting a new unpacked ref file for each one. And that _is_ a\nperformance problem for people with large numbers of refs.\n\nSo I think we need a check to make sure we aren't just updating with the\nsame value. Something like:\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 4c17f48..0e66e8f 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -237,8 +237,17 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)\n \trs.dst = NULL;\n \n \tif (!remote_find_tracking(remote, &rs)) {\n+\t\tunsigned char old_tracking_sha1[20];\n+\n \t\tif (args.verbose)\n \t\t\tfprintf(stderr, \"updating local tracking ref '%s'\\n\", rs.dst);\n+\n+\t\tif (!resolve_ref(rs.dst, old_tracking_sha1, 0, NULL) ||\n+\t\t    !hashcmp(old_tracking_sha1, ref->new_sha1)) {\n+\t\t\tfree(rs.dst);\n+\t\t\treturn;\n+\t\t}\n+\n \t\tif (ref->deletion) {\n \t\t\tdelete_ref(rs.dst, NULL, 0);\n \t\t} else\n\nThough I am not happy that we have to look up the tracking ref for every\nuptodate ref. I think it shouldn't be a big performance problem with\npacked refs, though, since they are cached (i.e., we pay only to compare\nthe hashes, not touch the filesystem for each ref).\n\n> +test_expect_success 'push updates local refs (2)' '\n\nNit: Just reading the test, it is hard to see what is interesting about\nit (though obviously I can blame it back to your commit :) ). Maybe a\nmore descriptive title like 'push updates uptodate local refs' would\nmake sense.\n\n-Peff\n"},{"id":"94872","messageId":"7v7i7jsx6a.fsf@gitster.siamese.dyndns.org","threadId":"16158","inReplyTo":"20081104042643.GA31276@coredump.intra.peff.net","subject":"Re: [PATCH] push: fix local refs update if already up-to-date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-04T08:38:37Z","receivedAt":"2008-11-04T08:38:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Though I am not happy that we have to look up the tracking ref for every\n> uptodate ref. I think it shouldn't be a big performance problem with\n> packed refs, though, since they are cached (i.e., we pay only to compare\n> the hashes, not touch the filesystem for each ref).\n\nIt is either (1) the user pays the cost of finding what remote tracking\nbranch we are mirroring when you push for all up-to-date refs, like you\ndid in your \"here is an improvement\" patch; or (2) the user pays the cost\nof fetching from there, immediately after pushing.  I'd imagine that the\ncost to do (1) would be smaller than (2).  The question is if seeing stale\ntracking branches is such a big deal, as next \"git fetch\" from there will\nupdate them anyway.  If it is a big deal, (1) would be a price worth\npaying.\n\nIn short, I agree with everything you said in your analysis.  Thanks for\nbeing a very good reviewer.\n\nClemens, care to reroll the patch?\n"},{"id":"94876","messageId":"20081104085630.GA22530@localhost","threadId":"16158","inReplyTo":"20081104042643.GA31276@coredump.intra.peff.net","subject":"Re: [PATCH] push: fix local refs update if already up-to-date","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-11-04T08:56:30Z","receivedAt":"2008-11-04T08:56:30Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Nov 03, 2008 at 11:26:44PM -0500, Jeff King wrote:\n> > The hashcpy for new_ref is now executed more often than absolutely\n> > necessary. But this is not a critical path, right? So I decided to keep\n> > things simple.\n> \n[...]\n> Your patch makes ref->new_sha1 \"valid\" for every status case. Ordinarily\n> I would be in favor of that, since it reduces coupling with other parts\n> of the code (which have to know _which_ status flags provide a useful\n> value in ->new_sha1). But in this case, I think the value we would be\n> sticking in is not necessarily useful for every status flag we end up\n> setting; so any consumers of the ref structure still need to know which\n> flags set it. So even though it has a defined value, it is not really\n> \"valid\" in all cases.\n\nThe other status flags are REF_STATUS_REJECT_NODELETE and\nREF_STATUS_REJECT_NONFASTFORWARD. So in these cases the \"new sha1\" is going\nto be the \"old sha1\". The default for new_sha1 is the null sha1. So while\nthe sha1 we're trying to push may not be more valid than the null sha1, it's\nnot less valid either, is it? And it even makes sense if you interpret\nnew_sha1 as the sha1 the client attempts to push.\n\n> Hmm. I was hoping to see more in update_tracking_ref. With your patch,\n> we end up calling update_ref for _every_ uptodate ref, which results in\n> writing a new unpacked ref file for each one. And that _is_ a\n> performance problem for people with large numbers of refs.\n> \n> So I think we need a check to make sure we aren't just updating with the\n> same value. Something like:\n\nI think update_ref already takes care of that. See this check in\nwrite_ref_sha1:\n\n        if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n                unlock_ref(lock);\n                return 0;\n        }\n\n> Though I am not happy that we have to look up the tracking ref for every\n> uptodate ref. I think it shouldn't be a big performance problem with\n> packed refs, though, since they are cached (i.e., we pay only to compare\n> the hashes, not touch the filesystem for each ref).\n\nI don't think we can avoid that, though.\n\nI agree with your other comments.\n"},{"id":"94877","messageId":"20081104090519.GB22530@localhost","threadId":"16158","inReplyTo":"7v7i7jsx6a.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] push: fix local refs update if already up-to-date","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-11-04T09:05:19Z","receivedAt":"2008-11-04T09:05:19Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Tue, Nov 04, 2008 at 12:38:37AM -0800, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > Though I am not happy that we have to look up the tracking ref for every\n> > uptodate ref. I think it shouldn't be a big performance problem with\n> > packed refs, though, since they are cached (i.e., we pay only to compare\n> > the hashes, not touch the filesystem for each ref).\n> \n> It is either (1) the user pays the cost of finding what remote tracking\n> branch we are mirroring when you push for all up-to-date refs, like you\n> did in your \"here is an improvement\" patch; or (2) the user pays the cost\n> of fetching from there, immediately after pushing.  I'd imagine that the\n> cost to do (1) would be smaller than (2).  The question is if seeing stale\n> tracking branches is such a big deal, as next \"git fetch\" from there will\n> update them anyway.  If it is a big deal, (1) would be a price worth\n> paying.\n\nRight. I think it is a big deal. I found out about this bug, because a user\non #git was confused by the fact that push reported \"Everything up-to-date\",\neven though there were changes. A fetch fixed that, of course. But it is\nconfusing and inconsistent with normal push behavior. So I really think it's\nworth a small performance hit.\n\n> Clemens, care to reroll the patch?\n\nI will do so later today.\n"},{"id":"94919","messageId":"20081104205743.GA26788@localhost","threadId":"16158","inReplyTo":"20081104042643.GA31276@coredump.intra.peff.net","subject":"[PATCH v2] push: fix local refs update if already up-to-date","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-11-04T20:57:43Z","receivedAt":"2008-11-04T20:57:43Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"git push normally updates local refs only after a successful push. If the\nremote already has the updates -- pushed indirectly through another repository,\nfor example -- we forget to update local tracking refs.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nOn Mon, Nov 03, 2008 at 11:26:44PM -0500, Jeff King wrote:\n> Nit: Just reading the test, it is hard to see what is interesting about\n> it (though obviously I can blame it back to your commit :) ). Maybe a\n> more descriptive title like 'push updates uptodate local refs' would\n> make sense.\n\nThat is all I changed in this update. Pending an Ack/Nack from Jeff I feel\nthat I'm done.\n\n builtin-send-pack.c   |   11 +++++------\n t/t5516-fetch-push.sh |   18 ++++++++++++++++++\n 2 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex d68ce2d..c91c12f 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -230,7 +230,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)\n {\n \tstruct refspec rs;\n \n-\tif (ref->status != REF_STATUS_OK)\n+\tif (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)\n \t\treturn;\n \n \trs.src = ref->name;\n@@ -444,15 +444,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\t}\n \t\telse\n \t\t\tnew_sha1 = ref->peer_ref->new_sha1;\n+\t\thashcpy(ref->new_sha1, new_sha1);\n \n-\n-\t\tref->deletion = is_null_sha1(new_sha1);\n+\t\tref->deletion = is_null_sha1(ref->new_sha1);\n \t\tif (ref->deletion && !allow_deleting_refs) {\n \t\t\tref->status = REF_STATUS_REJECT_NODELETE;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!ref->deletion &&\n-\t\t    !hashcmp(ref->old_sha1, new_sha1)) {\n+\t\t    !hashcmp(ref->old_sha1, ref->new_sha1)) {\n \t\t\tref->status = REF_STATUS_UPTODATE;\n \t\t\tcontinue;\n \t\t}\n@@ -480,14 +480,13 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\t    !ref->deletion &&\n \t\t    !is_null_sha1(ref->old_sha1) &&\n \t\t    (!has_sha1_file(ref->old_sha1)\n-\t\t      || !ref_newer(new_sha1, ref->old_sha1));\n+\t\t      || !ref_newer(ref->new_sha1, ref->old_sha1));\n \n \t\tif (ref->nonfastforward && !ref->force && !args.force_update) {\n \t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\n \t\t\tcontinue;\n \t\t}\n \n-\t\thashcpy(ref->new_sha1, new_sha1);\n \t\tif (!ref->deletion)\n \t\t\tnew_refs++;\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f0030ad..a82ce5a 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -437,6 +437,24 @@ test_expect_success 'push updates local refs' '\n \n '\n \n+test_expect_success 'push updates up-to-date local refs' '\n+\n+\trm -rf parent child &&\n+\tmkdir parent &&\n+\t(cd parent && git init &&\n+\t\techo one >foo && git add foo && git commit -m one) &&\n+\tgit clone parent child1 &&\n+\tgit clone parent child2 &&\n+\t(cd child1 &&\n+\t\techo two >foo && git commit -a -m two &&\n+\t\tgit push) &&\n+\t(cd child2 &&\n+\t\tgit pull ../child1 master &&\n+\t\tgit push &&\n+\ttest $(git rev-parse master) = $(git rev-parse remotes/origin/master))\n+\n+'\n+\n test_expect_success 'push does not update local refs on failure' '\n \n \trm -rf parent child &&\n-- \n1.6.0.3.617.ge4eb0\n"},{"id":"94949","messageId":"20081105024932.GA20907@coredump.intra.peff.net","threadId":"16158","inReplyTo":"20081104085630.GA22530@localhost","subject":"Re: [PATCH] push: fix local refs update if already up-to-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-11-05T02:49:32Z","receivedAt":"2008-11-05T02:49:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 04, 2008 at 09:56:30AM +0100, Clemens Buchacher wrote:\n\n> The other status flags are REF_STATUS_REJECT_NODELETE and\n> REF_STATUS_REJECT_NONFASTFORWARD. So in these cases the \"new sha1\" is going\n> to be the \"old sha1\". The default for new_sha1 is the null sha1. So while\n> the sha1 we're trying to push may not be more valid than the null sha1, it's\n> not less valid either, is it? And it even makes sense if you interpret\n> new_sha1 as the sha1 the client attempts to push.\n\nI have to admit I did not exhaustively look at all of the status cases\nwhen I reviewed earlier, and there are fewer than I realized. So I think\nyour change is reasonable.\n\nHowever, I would like to make one additional request.  Since you are\nkilling off all usage of new_sha1 initial assignment, I think it makes\nsense to just get rid of the variable entirely, so it cannot create\nconfusion later. Like this (on top of your patch):\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 4c17f48..d139eba 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -435,16 +435,13 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t */\n \tnew_refs = 0;\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n-\t\tconst unsigned char *new_sha1;\n-\n \t\tif (!ref->peer_ref) {\n \t\t\tif (!args.send_mirror)\n \t\t\t\tcontinue;\n-\t\t\tnew_sha1 = null_sha1;\n+\t\t\thashcpy(ref->new_sha1, null_sha1);\n \t\t}\n \t\telse\n-\t\t\tnew_sha1 = ref->peer_ref->new_sha1;\n-\t\thashcpy(ref->new_sha1, new_sha1);\n+\t\t\thashcpy(ref->new_sha1, ref->peer_ref->new_sha1);\n \n \t\tref->deletion = is_null_sha1(ref->new_sha1);\n \t\tif (ref->deletion && !allow_deleting_refs) {\n\n> > Hmm. I was hoping to see more in update_tracking_ref. With your patch,\n> > we end up calling update_ref for _every_ uptodate ref, which results in\n> > writing a new unpacked ref file for each one. And that _is_ a\n> > performance problem for people with large numbers of refs.\n> [...]\n> I think update_ref already takes care of that. See this check in\n> write_ref_sha1:\n> \n>         if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n>                 unlock_ref(lock);\n>                 return 0;\n>         }\n\nNope. That check is a concurrency safeguard. It checks that when we are\nmoving the ref from \"A\" to \"B\", that the ref still _is_ \"A\" when we lock\nit.\n\nBut more importantly, it is easy to demonstrate the problem with your\npatch:\n\n  mkdir parent &&\n    (cd parent &&\n       git init && touch file && git add file && git commit -m one) &&\n  git clone parent child &&\n    (cd child &&\n       echo BEFORE: && ls -l .git/refs/remotes/origin &&\n       git push &&\n       echo AFTER:  && ls -l .git/refs/remotes/origin)\n\nI get:\n\n  BEFORE:\n  -rw-r--r-- 1 peff peff 32 2008-11-04 21:43 HEAD\n  Everything up-to-date\n  AFTER:\n  -rw-r--r-- 1 peff peff 32 2008-11-04 21:43 HEAD\n  -rw-r--r-- 1 peff peff 41 2008-11-04 21:43 master\n\nOops. With the patch snippet I posted in my previous message, the\n'master' ref is not created by the uptodate push.\n\n> > Though I am not happy that we have to look up the tracking ref for every\n> > uptodate ref. I think it shouldn't be a big performance problem with\n> > packed refs, though, since they are cached (i.e., we pay only to compare\n> > the hashes, not touch the filesystem for each ref).\n> \n> I don't think we can avoid that, though.\n\nNo, you can't avoid it (without totally giving up on your patch's goal,\nwhich I think is a good one). So I think it is worth it, and I was just\nbeing paranoid about hurting performance. Even with packed refs, I think\nwe do still end up stat()ing for each ref, but we will have to live with\nit. I was thinking we might be able to do something clever with values\nwe had already read for the push, but it is impossible: we have read the\nrefs we are going to _push_, but we have not looked at the remote\ntracking branches, which are what contain the interesting information.\n\n-Peff\n"},{"id":"94950","messageId":"20081105025116.GB20907@coredump.intra.peff.net","threadId":"16158","inReplyTo":"20081104205743.GA26788@localhost","subject":"Re: [PATCH v2] push: fix local refs update if already up-to-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-11-05T02:51:16Z","receivedAt":"2008-11-05T02:51:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 04, 2008 at 09:57:43PM +0100, Clemens Buchacher wrote:\n\n> On Mon, Nov 03, 2008 at 11:26:44PM -0500, Jeff King wrote:\n> > Nit: Just reading the test, it is hard to see what is interesting about\n> > it (though obviously I can blame it back to your commit :) ). Maybe a\n> > more descriptive title like 'push updates uptodate local refs' would\n> > make sense.\n> \n> That is all I changed in this update. Pending an Ack/Nack from Jeff I feel\n> that I'm done.\n\nI have to NAK, because the extra written ref is still a problem (see my\nother mail). But with that fix (and I hope you both will agree with the\nstyle fixup on removing new_sha1, too), I think it should be good.\n\n-Peff\n"},{"id":"94994","messageId":"20081105202849.GA9484@localhost","threadId":"16158","inReplyTo":"20081105024932.GA20907@coredump.intra.peff.net","subject":"Re: [PATCH] push: fix local refs update if already up-to-date","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-11-05T20:28:49Z","receivedAt":"2008-11-05T20:28:49Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Tue, Nov 04, 2008 at 09:49:32PM -0500, Jeff King wrote:\n[...]\n> However, I would like to make one additional request.  Since you are\n> killing off all usage of new_sha1 initial assignment, I think it makes\n> sense to just get rid of the variable entirely, so it cannot create\n> confusion later.\n\nOk, I can live with that.\n\n> > > Hmm. I was hoping to see more in update_tracking_ref. With your patch,\n> > > we end up calling update_ref for _every_ uptodate ref, which results in\n> > > writing a new unpacked ref file for each one. And that _is_ a\n> > > performance problem for people with large numbers of refs.\n> > [...]\n> > I think update_ref already takes care of that. See this check in\n> > write_ref_sha1:\n> > \n> >         if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {\n> >                 unlock_ref(lock);\n> >                 return 0;\n> >         }\n> \n> Nope. That check is a concurrency safeguard. It checks that when we are\n> moving the ref from \"A\" to \"B\", that the ref still _is_ \"A\" when we lock\n> it.\n\nI think you are confusing this with verify_lock(). The code in\nwrite_ref_sha1() really does compare with the new sha1.\n\n> But more importantly, it is easy to demonstrate the problem with your\n> patch:\n> \n>   mkdir parent &&\n>     (cd parent &&\n>        git init && touch file && git add file && git commit -m one) &&\n>   git clone parent child &&\n>     (cd child &&\n>        echo BEFORE: && ls -l .git/refs/remotes/origin &&\n>        git push &&\n>        echo AFTER:  && ls -l .git/refs/remotes/origin)\n> \n> I get:\n> \n>   BEFORE:\n>   -rw-r--r-- 1 peff peff 32 2008-11-04 21:43 HEAD\n>   Everything up-to-date\n>   AFTER:\n>   -rw-r--r-- 1 peff peff 32 2008-11-04 21:43 HEAD\n>   -rw-r--r-- 1 peff peff 41 2008-11-04 21:43 master\n> \n> Oops. With the patch snippet I posted in my previous message, the\n> 'master' ref is not created by the uptodate push.\n\nThe reason it doesn't work is a bug in lock_ref_sha1_basic(). Dating back to\npre-\"pack-refs\" times, this code forces a write if the ref file does not\nexist. I will resubmit the patch including your above testcase and a bugfix\nfor lock_ref_sha1_basic().\n\nClemens\n"},{"id":"94996","messageId":"1225918554-29722-1-git-send-email-drizzd@aon.at","threadId":"16158","inReplyTo":"20081105202849.GA9484@localhost","subject":"[PATCH 1/2] do not force write of packed refs","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-11-05T20:55:53Z","receivedAt":"2008-11-05T20:55:53Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"We force writing a ref if it does not exist. Originally, we only had to look\nfor the ref file to check if it existed. Now we have to look for a packed ref\nas well. Luckily, resolve_ref already does all the work for us.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n refs.c               |    7 ++++---\n t/t3210-pack-refs.sh |    7 +++++++\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0a126fa..fc8105d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -794,10 +794,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \tchar *ref_file;\n \tconst char *orig_ref = ref;\n \tstruct ref_lock *lock;\n-\tstruct stat st;\n \tint last_errno = 0;\n \tint type, lflags;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n+\tint inexistent = 0;\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n@@ -825,12 +825,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \t\t\torig_ref, strerror(errno));\n \t\tgoto error_return;\n \t}\n+\tinexistent = is_null_sha1(lock->old_sha1);\n \t/* When the ref did not exist and we are creating it,\n \t * make sure there is no existing ref that is packed\n \t * whose name begins with our refname, nor a ref whose\n \t * name is a proper prefix of our refname.\n \t */\n-\tif (is_null_sha1(lock->old_sha1) &&\n+\tif (inexistent &&\n             !is_refname_available(ref, NULL, get_packed_refs(), 0))\n \t\tgoto error_return;\n \n@@ -844,7 +845,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \tlock->ref_name = xstrdup(ref);\n \tlock->orig_ref_name = xstrdup(orig_ref);\n \tref_file = git_path(\"%s\", ref);\n-\tif (lstat(ref_file, &st) && errno == ENOENT)\n+\tif (inexistent)\n \t\tlock->force_write = 1;\n \tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n \t\tlock->force_write = 1;\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 087ef75..413019a 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -96,6 +96,13 @@ test_expect_success \\\n      git branch -d n/o/p &&\n      git branch n'\n \n+test_expect_success \\\n+\t'see if up-to-date packed refs are preserved' \\\n+\t'git branch q &&\n+\t git pack-refs --all --prune &&\n+\t git update-ref refs/heads/q refs/heads/q &&\n+\t ! test -f .git/refs/heads/q'\n+\n test_expect_success 'pack, prune and repack' '\n \tgit tag foo &&\n \tgit pack-refs --all --prune &&\n-- \n1.6.0.3.617.ge4eb0\n"},{"id":"94997","messageId":"1225918554-29722-2-git-send-email-drizzd@aon.at","threadId":"16158","inReplyTo":"1225918554-29722-1-git-send-email-drizzd@aon.at","subject":"[PATCH 2/2] push: fix local refs update if already up-to-date","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-11-05T20:55:54Z","receivedAt":"2008-11-05T20:55:54Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"git push normally updates local refs only after a successful push. If the\nremote already has the updates -- pushed indirectly through another repository,\nfor example -- we forget to update local tracking refs.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n builtin-send-pack.c   |   11 +++++------\n t/t5516-fetch-push.sh |   31 +++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex d68ce2d..c91c12f 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -230,7 +230,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)\n {\n \tstruct refspec rs;\n \n-\tif (ref->status != REF_STATUS_OK)\n+\tif (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)\n \t\treturn;\n \n \trs.src = ref->name;\n@@ -444,15 +444,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\t}\n \t\telse\n \t\t\tnew_sha1 = ref->peer_ref->new_sha1;\n+\t\thashcpy(ref->new_sha1, new_sha1);\n \n-\n-\t\tref->deletion = is_null_sha1(new_sha1);\n+\t\tref->deletion = is_null_sha1(ref->new_sha1);\n \t\tif (ref->deletion && !allow_deleting_refs) {\n \t\t\tref->status = REF_STATUS_REJECT_NODELETE;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!ref->deletion &&\n-\t\t    !hashcmp(ref->old_sha1, new_sha1)) {\n+\t\t    !hashcmp(ref->old_sha1, ref->new_sha1)) {\n \t\t\tref->status = REF_STATUS_UPTODATE;\n \t\t\tcontinue;\n \t\t}\n@@ -480,14 +480,13 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\t    !ref->deletion &&\n \t\t    !is_null_sha1(ref->old_sha1) &&\n \t\t    (!has_sha1_file(ref->old_sha1)\n-\t\t      || !ref_newer(new_sha1, ref->old_sha1));\n+\t\t      || !ref_newer(ref->new_sha1, ref->old_sha1));\n \n \t\tif (ref->nonfastforward && !ref->force && !args.force_update) {\n \t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\n \t\t\tcontinue;\n \t\t}\n \n-\t\thashcpy(ref->new_sha1, new_sha1);\n \t\tif (!ref->deletion)\n \t\t\tnew_refs++;\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex f0030ad..598664c 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -437,6 +437,37 @@ test_expect_success 'push updates local refs' '\n \n '\n \n+test_expect_success 'push updates up-to-date local refs' '\n+\n+\trm -rf parent child &&\n+\tmkdir parent &&\n+\t(cd parent && git init &&\n+\t\techo one >foo && git add foo && git commit -m one) &&\n+\tgit clone parent child1 &&\n+\tgit clone parent child2 &&\n+\t(cd child1 &&\n+\t\techo two >foo && git commit -a -m two &&\n+\t\tgit push) &&\n+\t(cd child2 &&\n+\t\tgit pull ../child1 master &&\n+\t\tgit push &&\n+\ttest $(git rev-parse master) = $(git rev-parse remotes/origin/master))\n+\n+'\n+\n+test_expect_success 'push preserves up-to-date packed refs' '\n+\n+\trm -rf parent child &&\n+\tmkdir parent &&\n+\t(cd parent && git init &&\n+\t\techo one >foo && git add foo && git commit -m one) &&\n+\tgit clone parent child &&\n+\t(cd child &&\n+\t\tgit push &&\n+\t! test -f .git/refs/remotes/origin/master)\n+\n+'\n+\n test_expect_success 'push does not update local refs on failure' '\n \n \trm -rf parent child &&\n-- \n1.6.0.3.617.ge4eb0\n"},{"id":"95001","messageId":"20081105215706.GA32063@localhost","threadId":"16158","inReplyTo":"20081105202849.GA9484@localhost","subject":"Re: [PATCH] push: fix local refs update if already up-to-date","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-11-05T21:57:06Z","receivedAt":"2008-11-05T21:57:06Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Nov 05, 2008 at 09:28:49PM +0100, Clemens Buchacher wrote:\n> On Tue, Nov 04, 2008 at 09:49:32PM -0500, Jeff King wrote:\n> [...]\n> > However, I would like to make one additional request.  Since you are\n> > killing off all usage of new_sha1 initial assignment, I think it makes\n> > sense to just get rid of the variable entirely, so it cannot create\n> > confusion later.\n\nConsidering that the ref is initialized to the null_sha1, do you think it\nwould be Ok to do the following instead? The call to hashcpy would not be\nneeded twice and we get rid of the temporary new_sha1.\n\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -435,24 +435,18 @@ static int do_send_pack(int in, int out, struct remote *re\n         */\n        new_refs = 0;\n        for (ref = remote_refs; ref; ref = ref->next) {\n-               const unsigned char *new_sha1;\n-\n-               if (!ref->peer_ref) {\n-                       if (!args.send_mirror)\n-\t\t\t\tcontinue;\n-                       new_sha1 = null_sha1;\n-               }\n-               else\n-                       new_sha1 = ref->peer_ref->new_sha1;\n-\n+               if (ref->peer_ref)\n+                       hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);\n+               else if (!args.send_mirror)\n+                       continue;\n"},{"id":"95006","messageId":"7v63n1lsmr.fsf@gitster.siamese.dyndns.org","threadId":"16158","inReplyTo":"20081105215706.GA32063@localhost","subject":"Re: [PATCH] push: fix local refs update if already up-to-date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-05T22:23:08Z","receivedAt":"2008-11-05T22:23:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> On Wed, Nov 05, 2008 at 09:28:49PM +0100, Clemens Buchacher wrote:\n>> On Tue, Nov 04, 2008 at 09:49:32PM -0500, Jeff King wrote:\n>> [...]\n>> > However, I would like to make one additional request.  Since you are\n>> > killing off all usage of new_sha1 initial assignment, I think it makes\n>> > sense to just get rid of the variable entirely, so it cannot create\n>> > confusion later.\n>\n> Considering that the ref is initialized to the null_sha1, do you think it\n> would be Ok to do the following instead?\n\nYeah, that's pretty much what I munged and committed.\n"},{"id":"95011","messageId":"20081105224450.GA20440@coredump.intra.peff.net","threadId":"16158","inReplyTo":"20081105202849.GA9484@localhost","subject":"Re: [PATCH] push: fix local refs update if already up-to-date","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-11-05T22:44:51Z","receivedAt":"2008-11-05T22:44:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 05, 2008 at 09:28:49PM +0100, Clemens Buchacher wrote:\n\n> The reason it doesn't work is a bug in lock_ref_sha1_basic(). Dating back to\n> pre-\"pack-refs\" times, this code forces a write if the ref file does not\n> exist. I will resubmit the patch including your above testcase and a bugfix\n> for lock_ref_sha1_basic().\n\nOK, thanks for looking into it further. Both patches look sane to me.\n\n-Peff\n"}]}