Bug report: Git 1.8 on Ubuntu 13.10 refs not valid

Bug report: Git 1.8 on Ubuntu 13.10 refs not valid

[Siggi]

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://sneher@git.projects.gwdg.de/xrd-csd.git
Klone nach 'xrd-csd'...
Password for 'http://sneher@git.projects.gwdg.de':
fatal: http://sneher@git.projects.gwdg.de/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

Re: Bug report: Git 1.8 on Ubuntu 13.10 refs not valid

[Jeff King]

On Thu, Mar 27, 2014 at 03:45:34PM +0100, Siggi wrote:

> 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://sneher@git.projects.gwdg.de/xrd-csd.git
> Klone nach 'xrd-csd'...
> Password for 'http://sneher@git.projects.gwdg.de':
> fatal: http://sneher@git.projects.gwdg.de/xrd-csd.git/info/refs not valid:
> is this a git repository?

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://sneher@git.projects.gwdg.de/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

Re: Bug report: Git 1.8 on Ubuntu 13.10 refs not valid

[Jeff King]

On Mon, Mar 31, 2014 at 07:19:09PM +0200, Siggi wrote:

> here are the two outputs you wanted to see.

The interesting bit is at the end...

> < HTTP/1.1 200 OK
> < Date: Mon, 31 Mar 2014 17:04:57 GMT
> * Server Apache/2.2.22 (Ubuntu) is not blacklisted
> < Server: Apache/2.2.22 (Ubuntu)
> < X-Powered-By: Phusion Passenger (mod_rails/mod_rack) 3.0.19
> < ETag: "2ab570112563de50022189f0a2ffcdd4"
> < Pragma: no-cache
> < Expires: Fri, 01 Jan 1980 00:00:00 GMT
> < X-Runtime: 72
> < Cache-Control: no-cache, max-age=0, must-revalidate
> < Content-Length: 1047
> < Status: 200
> < Content-Type: application/x-git-upload-pack-advertisement; charset=utf-8

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

Re: Bug report: Git 1.8 on Ubuntu 13.10 refs not valid

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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.

Re: Bug report: Git 1.8 on Ubuntu 13.10 refs not valid

[Jeff King]

On Mon, Mar 31, 2014 at 11:27:53AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> 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.

Yeah, I think something like this is probably enough:

diff --git a/remote-curl.c b/remote-curl.c
index 52c2d96..be21fb6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -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 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.

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