Re: "fatal error in commit_refs" from pushing to github

From: Jeff King <peff@peff.net>
Date: Sep 8, 2016, 01:25 UTC
Message-ID: <20160908012553.q2aubze4qggfzjxz@sigill.intra.peff.net>
In-Reply-To: <CACsJy8BF_mnSUcEeH=RBMyfh8RrQGJ4dwm_svsC4TciJtECn_w@mail.gmail.com>

On Thu, Sep 08, 2016 at 07:49:12AM +0700, Duy Nguyen wrote:

Show 8 quoted lines

> I got the message in the subject when pushing to github today. Yes I
> know it's github, not git. But according to stackoveflow [1] it's a
> local problem. Which makes me think, if we know exactly what this is
> (or at least roughly the problem area), maybe we could improve git to
> catch it locally in the first place (and because other git servers may
> not have the same protection as github).  Jeff maybe you can reveal
> something about this "fatal error in commit_refs"? I'm sure it's not
> in git code. But I would understand if the answer is "no".

The short answer is that it's nothing to do with Git or the client; it's GitHub-specific code running on the server that is outside of Git entirely.

The long answer is that pushes to GitHub don't hit Git directly these days. They hit a proxy layer that speaks just enough of the Git protocol to relay to N separate receives spread across N replica servers[1]. Those receive-packs take in the pack and verify it, but don't actually update any refs[2]. Then the proxy layer runs its own set of policy hooks, and speaks a commit-protocol to each of the replicas so that they all agree on the new ref state. That last step is called "commit_refs" internally.

So this is really an internal failure at the ref-update stage. There _should_ be a reasonable error message, but I think "fatal error in commit_refs" is the generic last-ditch fallback. I'll pass this along to people in charge of that code, as we should be generating a more useful error message.

-Peff

[1] I glossed over a lot of details there. If you're interested:

    http://githubengineering.com/introducing-dgit/

    http://githubengineering.com/building-resilience-in-spokes/

[2] Initially the proxy just fed a set of temporary refs to
    receive-pack, and it was completely stock. These days we do have
    a trivial patch that skips the ref write. I haven't sent it upstream
    because it's useless by itself (but it's below for reference).

    I'm happy to polish it if somebody actually has a use for it.

---
 Documentation/config.txt        |  8 ++++++++
 builtin/receive-pack.c          | 13 +++++++++++--
 t/t9944-receive-pack-nowrite.sh | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)
 create mode 100755 t/t9944-receive-pack-nowrite.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f8e6484..38cc1ac 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2406,6 +2406,14 @@ receive.refUpdateNameLimit::
 	is a hard limit of 65520 bytes due to git's protocol, so this
 	value must be smaller than that.
 
+receive.writeRefs::
+	If set to `false`, `receive-pack` will perform all of the usual
+	ref consistency checks (checking for non-ff, etc), but _not_
+	actually write any ref changes to disk (nor even check that such
+	writes would succeed, as doing so atomically would require
+	taking individual ref locks to be of any value). The default is
+	`true`.
+
 remote.pushDefault::
 	The remote to push to by default.  Overrides
 	`branch.<name>.remote` for all branches, and is overridden by
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 94704e7..4a87365 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -87,6 +87,8 @@ static long nonce_stamp_slop;
 static unsigned long nonce_stamp_slop_limit;
 static struct ref_transaction *transaction;
 
+static int write_refs = 1;
+
 static enum deny_action parse_deny_action(const char *var, const char *value)
 {
 	if (value) {
@@ -244,6 +246,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.writerefs") == 0) {
+		write_refs = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 
@@ -1060,7 +1067,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 				cmd->did_not_exist = 1;
 			}
 		}
-		if (ref_transaction_delete(transaction,
+		if (write_refs &&
+		    ref_transaction_delete(transaction,
 					   namespaced_name,
 					   old_sha1,
 					   flags, "push", &err)) {
@@ -1077,7 +1085,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		    update_shallow_ref(cmd, si))
 			return "shallow error";
 
-		if (ref_transaction_update(transaction,
+		if (write_refs &&
+		    ref_transaction_update(transaction,
 					   namespaced_name,
 					   new_sha1, old_sha1,
 					   flags, "push",
diff --git a/t/t9944-receive-pack-nowrite.sh b/t/t9944-receive-pack-nowrite.sh
new file mode 100755
index 0000000..7b27bc1
--- /dev/null
+++ b/t/t9944-receive-pack-nowrite.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='test no-write tweak to receive-pack'
+. ./test-lib.sh
+
+test_expect_success 'create a few commits' '
+	test_commit one &&
+	git update-ref refs/heads/a HEAD &&
+	test_commit two &&
+	git update-ref refs/heads/b HEAD &&
+	test_commit three &&
+	git update-ref refs/heads/c HEAD
+'
+
+# push just two; hold back "c" so we can push a creation later
+test_expect_success 'create destination repo' '
+	git init --bare dst.git &&
+	git for-each-ref refs/heads/a refs/heads/b >expect &&
+	git push dst.git a b &&
+	git -C dst.git for-each-ref >actual &&
+	test_cmp expect actual
+'
+
+# push an update, a deletion, and a creation
+test_expect_success 'push with no-write config set' '
+	git push --receive-pack="git -c \
+			receive.writeRefs=false \
+			receive-pack" \
+		dst.git b:a :b c:c
+'
+
+test_expect_success 'push did not touch real refs' '
+	git -C dst.git for-each-ref >actual &&
+	test_cmp expect actual
+'
+
+test_done

-- 
2.10.0.rc2.154.gb4a4b8b