Re: "fatal error in commit_refs" from pushing to github
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
Duy Nguyen