From 72549dfd5d33424fbf557b0078503687780a3a53 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 4 Nov 2014 08:11:19 -0500 Subject: [PATCH 1/2] fetch: load all default config at startup When we start the git-fetch program, we call git_config to load all config, but our callback only processes the fetch.prune option; we do not chain to git_default_config at all. This means that we may not load some core configuration which will have an effect. For instance, we do not load core.logAllRefUpdates, which impacts whether or not we create reflogs in a bare repository. Note that I said "may" above. It gets even more exciting. If we have to transfer actual objects as part of the fetch, then we call fetch_pack as part of the same process. That function loads its own config, which does chain to git_default_config, impacting global variables which are used by the rest of fetch. But if the fetch is a pure ref update (e.g., a new ref which is a copy of an old one), we skip fetch_pack entirely. So we get inconsistent results depending on whether or not we have actual objects to transfer or not! Let's just load the core config at the start of fetch, so we know we have it (we may also load it again as part of fetch_pack, but that's OK; it's designed to be idempotent). Our tests check both cases (with and without a pack). We also check similar behavior for push for good measure, but it already works as expected. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fetch.c | 2 +- t/t5516-fetch-push.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 55f457c04f..36f386dbb3 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -66,7 +66,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb) fetch_prune_config = git_config_bool(k, v); return 0; } - return 0; + return git_default_config(k, v, cb); } static struct option builtin_fetch_options[] = { diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 67e0ab3462..2b69afaf37 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -11,6 +11,7 @@ This test checks the following functionality: * hooks * --porcelain output format * hiderefs +* reflogs ' . ./test-lib.sh @@ -1277,4 +1278,43 @@ EOF git push --no-thin --receive-pack="$rcvpck" no-thin/.git refs/heads/master:refs/heads/foo ' +test_expect_success 'push into bare respects core.logallrefupdates' ' + rm -rf dst.git && + git init --bare dst.git && + git -C dst.git config core.logallrefupdates true && + + # double push to test both with and without + # the actual pack transfer + git push dst.git master:one && + echo "one@{0} push" >expect && + git -C dst.git log -g --format="%gd %gs" one >actual && + test_cmp expect actual && + + git push dst.git master:two && + echo "two@{0} push" >expect && + git -C dst.git log -g --format="%gd %gs" two >actual && + test_cmp expect actual +' + +test_expect_success 'fetch into bare respects core.logallrefupdates' ' + rm -rf dst.git && + git init --bare dst.git && + ( + cd dst.git && + git config core.logallrefupdates true && + + # as above, we double-fetch to test both + # with and without pack transfer + git fetch .. master:one && + echo "one@{0} fetch .. master:one: storing head" >expect && + git log -g --format="%gd %gs" one >actual && + test_cmp expect actual && + + git fetch .. master:two && + echo "two@{0} fetch .. master:two: storing head" >expect && + git log -g --format="%gd %gs" two >actual && + test_cmp expect actual + ) +' + test_done From 9233887cce8eaebd8469315622b84bd26910351f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 4 Nov 2014 08:24:53 -0500 Subject: [PATCH 2/2] ignore stale directories when checking reflog existence When we update a ref, we have two rules for whether or not we actually update the reflog: 1. If the reflog already exists, we will always append to it. 2. If log_all_ref_updates is set, we will create a new reflog file if necessary. We do the existence check by trying to open the reflog file, either with or without O_CREAT (depending on log_all_ref_updates). If it fails, then we check errno to see what happened. If we were not using O_CREAT and we got ENOENT, the file doesn't exist, and we return success (there isn't a reflog already, and we were not told to make a new one). If we get EISDIR, then there is likely a stale directory that needs to be removed (e.g., there used to be "foo/bar", it was deleted, and the directory "foo" was left. Now we want to create the ref "foo"). If O_CREAT is set, then we catch this case, try to remove the directory, and retry our open. So far so good. But if we get EISDIR and O_CREAT is not set, then we treat this as any other error, which is not right. Like ENOENT, EISDIR is an indication that we do not have a reflog, and we should silently return success (we were not told to create it). Instead, the current code reports this as an error, and we fail to update the ref at all. Note that this is relatively unlikely to happen, as you would have to have had reflogs turned on, and then later turned them off (it could also happen due to a bug in fetch, but that was fixed in the previous commit). However, it's quite easy to fix: we just need to treat EISDIR like ENOENT for the non-O_CREAT case, and silently return (note that this early return means we can also simplify the O_CREAT case). Our new tests cover both cases (O_CREAT and non-O_CREAT). The first one already worked, of course. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 4 ++-- t/t1410-reflog.sh | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index f0bd7ac0e9..e9eda881d0 100644 --- a/refs.c +++ b/refs.c @@ -2751,10 +2751,10 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize) logfd = open(logfile, oflags, 0666); if (logfd < 0) { - if (!(oflags & O_CREAT) && errno == ENOENT) + if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR)) return 0; - if ((oflags & O_CREAT) && errno == EISDIR) { + if (errno == EISDIR) { if (remove_empty_directories(logfile)) { return error("There are still logs under '%s'", logfile); diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh index 236b13a3ab..e4409e38df 100755 --- a/t/t1410-reflog.sh +++ b/t/t1410-reflog.sh @@ -245,4 +245,38 @@ test_expect_success 'gc.reflogexpire=false' ' ' +test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' ' + test_when_finished "git branch -d a || git branch -d a/b" && + + git branch a/b master && + echo "a/b@{0} branch: Created from master" >expect && + git log -g --format="%gd %gs" a/b >actual && + test_cmp expect actual && + git branch -d a/b && + + # now logs/refs/heads/a is a stale directory, but + # we should move it out of the way to create "a" reflog + git branch a master && + echo "a@{0} branch: Created from master" >expect && + git log -g --format="%gd %gs" a >actual && + test_cmp expect actual +' + +test_expect_success 'stale dirs do not cause d/f conflicts (reflogs off)' ' + test_when_finished "git branch -d a || git branch -d a/b" && + + git branch a/b master && + echo "a/b@{0} branch: Created from master" >expect && + git log -g --format="%gd %gs" a/b >actual && + test_cmp expect actual && + git branch -d a/b && + + # same as before, but we only create a reflog for "a" if + # it already exists, which it does not + git -c core.logallrefupdates=false branch a master && + : >expect && + git log -g --format="%gd %gs" a >actual && + test_cmp expect actual +' + test_done