{"thread":{"id":"37876","subject":"[PATCH 0/2] fetch, reflogs, and bare repositories","startedAt":"2014-11-04T13:10:07Z","lastAt":"2014-11-04T19:40:53Z","messageCount":4,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"251372","messageId":"20141104131006.GA1077@peff.net","threadId":"37876","inReplyTo":null,"subject":"[PATCH 0/2] fetch, reflogs, and bare repositories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-04T13:10:07Z","receivedAt":"2014-11-04T13:10:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I ran into a rather confusing case of fetching into a bare repository\nwith reflogs turned on:\n\n    # make a repo with a two-component branch name\n    git init -q &&\n    git commit -q --allow-empty -m one &&\n    git branch foo/bar &&\n    \n    # now fetch it all into a bare repo with reflogs\n    git init -q --bare parent.git &&\n    cd parent.git &&\n    git config core.logallrefupdates true &&\n    git fetch --prune .. +refs/*:refs/* &&\n    \n    # now replace the branch with one that has a d/f conflict\n    cd .. &&\n    git branch -d foo/bar &&\n    git branch foo &&\n    \n    # and fetch again\n    cd parent.git &&\n    git fetch --prune .. +refs/*:refs/*\n\nThe final fetch fails and produces this output:\n\n    From ..\n     x [deleted]         (none)     -> foo/bar\n    error: Unable to append to ./logs/refs/heads/foo: Is a directory\n     ! [new branch]      foo        -> foo  (unable to update local ref)\n\nThis turns out to be caused by two subtle bugs: one that makes \"git\nfetch\" use reflogs inconsistently, and the other that causes some ref\nupdates to fail when reflogs are turned on and off. Details are in the\nfixes themselves.\n\n  [1/2]: fetch: load all default config at startup\n  [2/2]: ignore stale directories when checking reflog existence\n\n-Peff\n"},{"id":"251373","messageId":"20141104131118.GA23608@peff.net","threadId":"37876","inReplyTo":"20141104131006.GA1077@peff.net","subject":"[PATCH 1/2] fetch: load all default config at startup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-04T13:11:19Z","receivedAt":"2014-11-04T13:11:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we start the git-fetch program, we call git_config to\nload all config, but our callback only processes the\nfetch.prune option; we do not chain to git_default_config at\nall.\n\nThis means that we may not load some core configuration\nwhich will have an effect. For instance, we do not load\ncore.logAllRefUpdates, which impacts whether or not we\ncreate reflogs in a bare repository.\n\nNote that I said \"may\" above. It gets even more exciting. If\nwe have to transfer actual objects as part of the fetch,\nthen we call fetch_pack as part of the same process. That\nfunction loads its own config, which does chain to\ngit_default_config, impacting global variables which are\nused by the rest of fetch. But if the fetch is a pure ref\nupdate (e.g., a new ref which is a copy of an old one), we\nskip fetch_pack entirely. So we get inconsistent results\ndepending on whether or not we have actual objects to\ntransfer or not!\n\nLet's just load the core config at the start of fetch, so we\nknow we have it (we may also load it again as part of\nfetch_pack, but that's OK; it's designed to be idempotent).\n\nOur tests check both cases (with and without a pack). We\nalso check similar behavior for push for good measure, but\nit already works as expected.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fetch.c       |  2 +-\n t/t5516-fetch-push.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 6ffd023..7b84d35 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -68,7 +68,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t\tfetch_prune_config = git_config_bool(k, v);\n \t\treturn 0;\n \t}\n-\treturn 0;\n+\treturn git_default_config(k, v, cb);\n }\n \n static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 7c8a769..f4da20a 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -11,6 +11,7 @@ This test checks the following functionality:\n * hooks\n * --porcelain output format\n * hiderefs\n+* reflogs\n '\n \n . ./test-lib.sh\n@@ -1290,4 +1291,43 @@ test_expect_success 'pushing a tag pushes the tagged object' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'push into bare respects core.logallrefupdates' '\n+\trm -rf dst.git &&\n+\tgit init --bare dst.git &&\n+\tgit -C dst.git config core.logallrefupdates true &&\n+\n+\t# double push to test both with and without\n+\t# the actual pack transfer\n+\tgit push dst.git master:one &&\n+\techo \"one@{0} push\" >expect &&\n+\tgit -C dst.git log -g --format=\"%gd %gs\" one >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit push dst.git master:two &&\n+\techo \"two@{0} push\" >expect &&\n+\tgit -C dst.git log -g --format=\"%gd %gs\" two >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'fetch into bare respects core.logallrefupdates' '\n+\trm -rf dst.git &&\n+\tgit init --bare dst.git &&\n+\t(\n+\t\tcd dst.git &&\n+\t\tgit config core.logallrefupdates true &&\n+\n+\t\t# as above, we double-fetch to test both\n+\t\t# with and without pack transfer\n+\t\tgit fetch .. master:one &&\n+\t\techo \"one@{0} fetch .. master:one: storing head\" >expect &&\n+\t\tgit log -g --format=\"%gd %gs\" one >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit fetch .. master:two &&\n+\t\techo \"two@{0} fetch .. master:two: storing head\" >expect &&\n+\t\tgit log -g --format=\"%gd %gs\" two >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"251374","messageId":"20141104132452.GB23608@peff.net","threadId":"37876","inReplyTo":"20141104131006.GA1077@peff.net","subject":"[PATCH 2/2] ignore stale directories when checking reflog existence","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-04T13:24:53Z","receivedAt":"2014-11-04T13:24:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we update a ref, we have two rules for whether or not\nwe actually update the reflog:\n\n  1. If the reflog already exists, we will always append to\n     it.\n\n  2. If log_all_ref_updates is set, we will create a new\n     reflog file if necessary.\n\nWe do the existence check by trying to open the reflog file,\neither with or without O_CREAT (depending on log_all_ref_updates).\nIf it fails, then we check errno to see what happened.\n\nIf we were not using O_CREAT and we got ENOENT, the file\ndoesn't exist, and we return success (there isn't a reflog\nalready, and we were not told to make a new one).\n\nIf we get EISDIR, then there is likely a stale directory\nthat needs to be removed (e.g., there used to be \"foo/bar\",\nit was deleted, and the directory \"foo\" was left. Now we\nwant to create the ref \"foo\"). If O_CREAT is set, then we\ncatch this case, try to remove the directory, and retry our\nopen. So far so good.\n\nBut if we get EISDIR and O_CREAT is not set, then we treat\nthis as any other error, which is not right. Like ENOENT,\nEISDIR is an indication that we do not have a reflog, and we\nshould silently return success (we were not told to create\nit). Instead, the current code reports this as an error, and\nwe fail to update the ref at all.\n\nNote that this is relatively unlikely to happen, as you\nwould have to have had reflogs turned on, and then later\nturned them off (it could also happen due to a bug in fetch,\nbut that was fixed in the previous commit). However, it's\nquite easy to fix: we just need to treat EISDIR like ENOENT\nfor the non-O_CREAT case, and silently return (note that\nthis early return means we can also simplify the O_CREAT\ncase).\n\nOur new tests cover both cases (O_CREAT and non-O_CREAT).\nThe first one already worked, of course.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNote that rs/ref-transaction-reflog tweaks this area a bit; it\nintroduces a separate reflog_exists() check that runs before we even get\nto the first open(). Which would make the tests I introduce here run\ncorrectly even without this patch. However, the code I am changing does\nstill exist in that series, and I think would handle cases where\nlogallrefupdates was turned on, but the ref was outside of refs/heads/.\nSo even with that series, we do still need this fix (and of course there\nis the obvious benefit that we can probably merge this fix sooner than\nthat series).\n\n refs.c            |  4 ++--\n t/t1410-reflog.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0368ed4..5ff457e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2962,10 +2962,10 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \n \tlogfd = open(logfile, oflags, 0666);\n \tif (logfd < 0) {\n-\t\tif (!(oflags & O_CREAT) && errno == ENOENT)\n+\t\tif (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))\n \t\t\treturn 0;\n \n-\t\tif ((oflags & O_CREAT) && errno == EISDIR) {\n+\t\tif (errno == EISDIR) {\n \t\t\tif (remove_empty_directories(logfile)) {\n \t\t\t\tint save_errno = errno;\n \t\t\t\terror(\"There are still logs under '%s'\",\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 8cab06f..485f1a7 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -253,4 +253,38 @@ test_expect_success 'checkout should not delete log for packed ref' '\n \ttest $(git reflog master | wc -l) = 4\n '\n \n+test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' '\n+\ttest_when_finished \"git branch -d a || git branch -d a/b\" &&\n+\n+\tgit branch a/b &&\n+\techo \"a/b@{0} branch: Created from foo\" >expect &&\n+\tgit log -g --format=\"%gd %gs\" a/b >actual &&\n+\ttest_cmp expect actual &&\n+\tgit branch -d a/b &&\n+\n+\t# now logs/refs/heads/a is a stale directory, but\n+\t# we should move it out of the way to create \"a\" reflog\n+\tgit branch a &&\n+\techo \"a@{0} branch: Created from foo\" >expect &&\n+\tgit log -g --format=\"%gd %gs\" a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'stale dirs do not cause d/f conflicts (reflogs off)' '\n+\ttest_when_finished \"git branch -d a || git branch -d a/b\" &&\n+\n+\tgit branch a/b &&\n+\techo \"a/b@{0} branch: Created from foo\" >expect &&\n+\tgit log -g --format=\"%gd %gs\" a/b >actual &&\n+\ttest_cmp expect actual &&\n+\tgit branch -d a/b &&\n+\n+\t# same as before, but we only create a reflog for \"a\" if\n+\t# it already exists, which it does not\n+\tgit -c core.logallrefupdates=false branch a &&\n+\t: >expect &&\n+\tgit log -g --format=\"%gd %gs\" a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"251378","messageId":"xmqqy4rqpxdm.fsf@gitster.dls.corp.google.com","threadId":"37876","inReplyTo":"20141104131006.GA1077@peff.net","subject":"Re: [PATCH 0/2] fetch, reflogs, and bare repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-04T19:40:53Z","receivedAt":"2014-11-04T19:40:53Z","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> I ran into a rather confusing case of fetching into a bare repository\n> with reflogs turned on:\n> ...\n> This turns out to be caused by two subtle bugs: one that makes \"git\n> fetch\" use reflogs inconsistently, and the other that causes some ref\n> updates to fail when reflogs are turned on and off. Details are in the\n> fixes themselves.\n>\n>   [1/2]: fetch: load all default config at startup\n>   [2/2]: ignore stale directories when checking reflog existence\n\nMust have been painful and fun at the same time to diagnose these\n;-)  Both patches looked very sensible.\n\nThanks.\n"}]}