Bug report: Git 1.8 on Ubuntu 13.10 refs not valid

Hi,
I'm running:

Ubuntu 13.10 64 bit

and git version
git:amd64/saucy 1:1.8.3.2-1 uptodate

my remote repository is on a Chiliprojekt server (a fork of Redmine).

cloning the repo over http results in following error:

sneher@sneher-XPS:~/Dokumente/test$ git clone http://[email protected]/xrd-csd.git
Klone nach 'xrd-csd'...
Password for 'http://[email protected]':
fatal: http://[email protected]/xrd-csd.git/info/refs not valid: is this a git repository?

the content of ../info/refs looks like this

e49ae34096fd6fff3d1e7b8e7b6e78ae29bad913 refs/heads/0.2.2
3d375b2f7eeeb7bc12b24cc8181aa085f471ba10 refs/heads/master
f7a69735c1e2cb8363be849afa9e9bfdf2db61c6 refs/heads/new_lab
879ccace941ea6dc83876b1dcfcc099e5c7e5b42 refs/heads/testing
2f9504da3febcdafb9cb92806e7e178144fec0c9 refs/remotes/origin/HEAD
2f9504da3febcdafb9cb92806e7e178144fec0c9 refs/remotes/origin/master
f7a69735c1e2cb8363be849afa9e9bfdf2db61c6 refs/remotes/origin/new_lab
58fe57f5a2a0c8e8096c62f8ab8be2077c592e53 refs/remotes/origin/testing
4b64a990dc1534abcccfb7f8c22f0cc5388e9db8 refs/tags/0.1.0
a90ce817ca3bde41ce6c88cf22a9993bd7560f55 refs/tags/0.1.1
9a25635e866979b044b83f914cfd993a7031a9ca refs/tags/0.1.2
5a94e698b1042b34a25c87ced98f5f42d40e2578 refs/tags/0.2.0
7cb00e325c1fb9a4112700744237f873bd5bae16 refs/tags/0.2.1

I use to have the same problem on a different Ubuntu version (12.10). There the problem occurede with the git 1.8 update. I just switcht back to the older version.

Problem is, there is no older version for saucy.

Thanks for your help! and, in case this do to my inability, sorry for bugging you!

Siggi

Hmm. The only way to trigger that message is if the dumb info/refs output from the server is not valid. In particular, it is looking for the tab character between the sha1 and the refs, and making sure that the sha1 is exactly 40 characters.

I noticed other people having the problem, too:

https://github.com/kubitron/redmine_git_hosting/issues/106

so I think it is related to the output that the redmine plugin is producing. But the interesting thing is that the plugin is supposed to enable git's smart-http protocol. But the error message you are seeing indicates that the client thinks it is doing a "dumb" http fetch.

The parsing code did not change in the v1.8.x series, but the logic to determine whether we are using smart/dumb http did change. For example, we now actually check the content-type returned by the server (which should be "application/x-git-upload-pack-advertisement").

Can you try running your clone with GIT_CURL_VERBOSE=1 in the environment? That should show the headers (including content-type). Do be careful when sharing the output; I believe it will contain "Authorization" lines that have your base64-encoded password on them.

Also, I would be curious to see the output of:

curl http://[email protected]/xrd-csd.git/info/refs | cat -A

My suspicion is that it is really smart-http output, but the client is parsing it as dumb-http output (and probably because of the content-type mentioned above).

-Peff

The interesting bit is at the end...

This content-type is the problem. There should not be a charset parameter (it is meaningless, and it throws off git's content-type check). So your web server configuration (or the redmine git plugin) should be fixed, but you'll have to talk to redmine folks to figure that part out.

That being said, git _could_ be more liberal in accepting a content-type with parameters (even though it does not know about any parameters, and charset here is completely meaningless). I have mixed feelings on that.

-Peff

It may be just a matter of replacing strbuf_cmp() with "the initial part must be this string" followed by "it could have an optional whitespaces and semicolon after that", but I share the mixed feelings.

I am not sure if it is a right thing to follow "be liberal to accept" dictum in this case. It may be liberal in accepting blindly, but if the other end is asking a behaviour that is not standard (but perhaps in future versions of Git such an enhanced service may be implemented by the client), by ignoring the parameter we do not even know about how to handle, we would be giving surprises to the overall protocol exchange.

Yeah, I think something like this is probably enough:

index 52c2d96..be21fb6 100644
@@ -221,6 +221,13 @@ static int show_http_message(struct strbuf *type, struct strbuf *msg)
 	return 0;
 }
 
+static int match_content_type(struct strbuf *have, struct strbuf *want)
+{
+	if (!starts_with(have->buf, want->buf))
+		return 0;
+	return have->len == want->len || have->buf[want->len] == ';';
+}
+
 static struct discovery* discover_refs(const char *service, int for_push)
 {
 	struct strbuf exp = STRBUF_INIT;

@@ -277,7 +284,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	strbuf_addf(&exp, "application/x-%s-advertisement", service);
 	if (maybe_smart &&
 	    (5 <= last->len && last->buf[4] == '#') &&
-	    !strbuf_cmp(&exp, &type)) {
+	    match_content_type(type.buf, exp.buf)) {
 		char *line;
 
 		/*

though perhaps there are exotic whitespace rules that we would also need to allow. Either way, I do not think it is the implementation that is the problem, but rather the semantics.

I actually think ignoring them could provide room for future expansion in git (i.e., we could add new c-t parameters with the knowledge that existing git would ignore them). So there is a potential upside. But current git does _not_ ignore them. Git v1.10 (or 2.0, or whatever) could, but we would have to wait for old versions to die out before making that assumption anyway.

It's also possible that we would want to intentionally break compatibility (to say "if you do not understand foo=bar, then do not even process this"). But I don't think that is a big deal, as we could already switch the content-type itself ("x-upload-pack-advertisement2" or something). And if we really wanted to dump extra optional data into the http conversation, we can already do so with http headers.

So really, I do not see us ever realistically expanding into content-type parameters. This would _just_ be about handling odd implementations. And there I can see it as us being nice (I do not think "charset" is doing anything here), or us being careful about broken implementations (why would one add a charset parameter at all? If the implementation is blindly re-encoding to utf8 or something, it could very well be corrupting the data in rare cases).

Given that there is only one implementation known to this in the first place, I'd be inclined to say "fix that implementation". If it becomes a widespread problem, then we can think about loosening the check after reviewing exactly what each implementation is doing.

I think all of that is to say I'm violently agreeing with you. But having tried to think it through, I felt it was worth posting the more detailed thought process.

-Peff