[PATCH] transport: do not allow to push over git:// protocol

[PATCH] transport: do not allow to push over git:// protocol

[Nguyễn Thái Ngọc Duy]

This protocol has never been designed for pushing. Attempts to push
over git:// usually result in

  fatal: The remote end hung up unexpectedly

That message does not really point out the reason. With this patch, we get

  error: this protocol does not support pushing
  error: failed to push some refs to 'git://some-host.com/my/repo'

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I wanted to advise using remote.*.pushurl too, more friendly. But then I
 had to detect if url comes from command line or config, and I gave up.

 transport.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/transport.c b/transport.c
index fa279d5..b109145 100644
--- a/transport.c
+++ b/transport.c
@@ -933,7 +933,8 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->set_option = NULL;
 		ret->get_refs_list = get_refs_via_connect;
 		ret->fetch = fetch_refs_via_pack;
-		ret->push_refs = git_transport_push;
+		if (prefixcmp(url, "git://"))
+			ret->push_refs = git_transport_push;
 		ret->connect = connect_git;
 		ret->disconnect = disconnect_git;
 		ret->smart_options = &(data->options);
@@ -1075,6 +1076,8 @@ int transport_push(struct transport *transport,
 
 		return ret;
 	}
+	else
+		return error("this protocol does not support pushing");
 	return 1;
 }
 
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH] transport: do not allow to push over git:// protocol

[Ilari Liusvaara]

On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyễn Thái Ngọc Duy wrote:
> This protocol has never been designed for pushing. Attempts to push
> over git:// usually result in
> 
>   fatal: The remote end hung up unexpectedly
> 
> That message does not really point out the reason. With this patch, we get
> 
>   error: this protocol does not support pushing
>   error: failed to push some refs to 'git://some-host.com/my/repo'

What about sticking code to return an error to git daemon instead of this?

Here's what happens if I try to push to one of repos on this computer
over git://:

$ git push git://localhost/foobar
fatal: remote error: W access for foobar DENIED to anonymous

So send-pack can deal with ERR packet (and yes, that error message
is really from Gitolite).

Aside: git archive seemingly can't deal with ERR packets. And worse
yet, it doesn't even print what it received, resulting this:

$ git archive --remote=git://localhost/foobar HEAD
fatal: git archive: protocol error


-Ilari 

Re: [PATCH] transport: do not allow to push over git:// protocol

[Nguyen Thai Ngoc Duy]

2011/10/1 Ilari Liusvaara <ilari.liusvaara@elisanet.fi>:
> What about sticking code to return an error to git daemon instead of this?
>
> Here's what happens if I try to push to one of repos on this computer
> over git://:
>
> $ git push git://localhost/foobar
> fatal: remote error: W access for foobar DENIED to anonymous
>
> So send-pack can deal with ERR packet (and yes, that error message
> is really from Gitolite).

I'm dealing with git.gnome.org and not sure what's the server behind.
I had a look at git-daemon and it does allow push, but disabled by
default. So yes, maybe updating git-daemon is better.

> Aside: git archive seemingly can't deal with ERR packets. And worse
> yet, it doesn't even print what it received, resulting this:
>
> $ git archive --remote=git://localhost/foobar HEAD
> fatal: git archive: protocol error

Yes, builtin/archive.c seems only recognize either ACK or NACK.
pack-protocol.txt does not mention about ERR either, which seems to be
introduced in a807328 (connect.c: add a way for git-daemon to pass an
error back to client).
-- 
Duy

Re: [PATCH] transport: do not allow to push over git:// protocol

[Jonathan Nieder]

Ilari Liusvaara wrote:

> What about sticking code to return an error to git daemon instead of this?

The code has even been written:
http://thread.gmane.org/gmane.comp.version-control.git/145456/focus=145573

Testing and other improvements would be very welcome.

Re: [PATCH] transport: do not allow to push over git:// protocol

[Nguyen Thai Ngoc Duy]

2011/10/1 Jonathan Nieder <jrnieder@gmail.com>:
> Ilari Liusvaara wrote:
>
>> What about sticking code to return an error to git daemon instead of this?
>
> The code has even been written:
> http://thread.gmane.org/gmane.comp.version-control.git/145456/focus=145573
>
> Testing and other improvements would be very welcome.

Tests aside, are there any problems with the patch? I don't see any
followup discussions. Personally I don't see much value in adding the
description though.
-- 
Duy

Re: [PATCH] transport: do not allow to push over git:// protocol

[Nguyen Thai Ngoc Duy]

2011/10/3 Alexey Shumkin <alex.crezoff@gmail.com>:
>> This protocol has never been designed for pushing. Attempts to push
>> over git:// usually result in
>>
>>   fatal: The remote end hung up unexpectedly
>
> hmmm.. So, how does my project work? ))
>
> $ git remote -v
> origin git://drcis/d/Data/GitRepos/projects/billing+beeline.git (fetch)
> origin git://drcis/d/Data/GitRepos/projects/billing+beeline.git (push)
>
>
> $git daemon --help
> ...
> SERVICES
> ..
>       receive-pack
>           This serves git send-pack clients, allowing anonymous push.
>           It is disabled by default
> ...
>
> It does not correspond your words...
>
> What do I miss?

.. what I said in a later mail, my patch is wrong :)
-- 
Duy

Re: [PATCH] transport: do not allow to push over git:// protocol

[Jeff King]

On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyen Thai Ngoc Duy wrote:

> This protocol has never been designed for pushing. Attempts to push
> over git:// usually result in
> 
>   fatal: The remote end hung up unexpectedly
> 
> That message does not really point out the reason. With this patch, we get
> 
>   error: this protocol does not support pushing
>   error: failed to push some refs to 'git://some-host.com/my/repo'

I thought pushing over git:// _is_ supported. It's just that most
servers don't have it turned on, for the obvious lack-of-authentication
reasons.

See 4b3b1e1 (git-push through git protocol, 2007-01-21), and the
discussion here:

  http://thread.gmane.org/gmane.comp.version-control.git/37325

Your patch shuts it off at the client level, so even with it turned on
for the server, the client can never get to it.

I still think push-over-git:// is a bit insane, and especially now with
smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
seeing it deprecated. But just shutting it off without a deprecation
period seems unnecessarily harsh.

The real problem here seems to be that instead of communicating "no, we
don't support that", git-daemon just hangs up. It would be a much nicer
fix if we could change that. I'm not sure it's possible, though. There's
not much room in the beginning of the room to make that communication in
a way that's backwards compatible.

-Peff

Re: [PATCH] transport: do not allow to push over git:// protocol

[Johannes Sixt]

Am 10/3/2011 9:42, schrieb Jeff King:
> I still think push-over-git:// is a bit insane, and especially now with
> smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
> seeing it deprecated.

You must be kidding ;) It is so much easier to type

  git daemon --export-all --enable=receive-pack

for a one-shot, temporary git connection compared to setting up a
smart-http, ssh, or even a rsh server.

-- Hannes

Re: [PATCH] transport: do not allow to push over git:// protocol

[Jeff King]

On Mon, Oct 03, 2011 at 10:44:23AM +0200, Johannes Sixt wrote:

> Am 10/3/2011 9:42, schrieb Jeff King:
> > I still think push-over-git:// is a bit insane, and especially now with
> > smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
> > seeing it deprecated.
> 
> You must be kidding ;) It is so much easier to type
> 
>   git daemon --export-all --enable=receive-pack
> 
> for a one-shot, temporary git connection compared to setting up a
> smart-http, ssh, or even a rsh server.

Ah, yeah, I didn't think about one-shot invocations like that (I think
the original motivation was somebody actually running it all the time).

So yeah, that makes it even worse for the client to start refusing this
without even contacting the server. I forgot that we added the "ERR"
response way back in a807328 (connect.c: add a way for git-daemon to
pass an error back to client, 2008-11-01).

GitHub uses it to make nice messages:

  $ git push origin
  fatal: remote error:
    You can't push to git://github.com/gitster/git.git
    Use git@github.com:gitster/git.git

We should maybe do something like the patch below:

diff --git a/daemon.c b/daemon.c
index 4c8346d..c1fa55f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -255,6 +255,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	loginfo("Request %s for '%s'", service->name, dir);
 
 	if (!enabled && !service->overridable) {
+		packet_write(1, "ERR %s: service not enabled", service->name);
 		logerror("'%s': service not enabled.", service->name);
 		errno = EACCES;
 		return -1;
@@ -288,6 +289,8 @@ static int run_service(char *dir, struct daemon_service *service)
 			enabled = service_enabled;
 	}
 	if (!enabled) {
+		packet_write(1, "ERR %s: service not enabled for '%s'",
+		       service->name, path);
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
 		errno = EACCES;

but:

  1. There is some information leakage there. In particular, one can
     tell the difference now between "repo does not exist" and
     "receive-pack is not turned on". Personally, I think the tradeoff
     to have actual error messages is worth it. HTTP has had real error
     codes for decades, and I don't think anybody is too up-in-arms that
     I can probe which pages are 404, and which are 401.

  2. It probably makes sense to have a more human-friendly error
     message.

  3. It may be worth adding error messages for lots of other conditions
     (e.g., no such repo). Assuming we accept the information leakage
     for (1).

-Peff

Re: [PATCH] transport: do not allow to push over git:// protocol

[Nguyen Thai Ngoc Duy]

2011/10/3 Jeff King <peff@peff.net>:
> So yeah, that makes it even worse for the client to start refusing this
> without even contacting the server. I forgot that we added the "ERR"
> response way back in a807328 (connect.c: add a way for git-daemon to
> pass an error back to client, 2008-11-01).
>
> GitHub uses it to make nice messages:
>
>  $ git push origin
>  fatal: remote error:
>    You can't push to git://github.com/gitster/git.git
>    Use git@github.com:gitster/git.git
>
> We should maybe do something like the patch below:

Jonathan also mentions another patch

http://article.gmane.org/gmane.comp.version-control.git/182536

> but:
>
>  1. There is some information leakage there. In particular, one can
>     tell the difference now between "repo does not exist" and
>     "receive-pack is not turned on". Personally, I think the tradeoff
>     to have actual error messages is worth it. HTTP has had real error
>     codes for decades, and I don't think anybody is too up-in-arms that
>     I can probe which pages are 404, and which are 401.

To me, just "<service>: access denied" is enough. Not particularly
friendly but should be a good enough clue.
-- 
Duy

Re: [PATCH] transport: do not allow to push over git:// protocol

[Jeff King]

On Mon, Oct 03, 2011 at 08:44:22PM +1100, Nguyen Thai Ngoc Duy wrote:

> > GitHub uses it to make nice messages:
> >
> >  $ git push origin
> >  fatal: remote error:
> >    You can't push to git://github.com/gitster/git.git
> >    Use git@github.com:gitster/git.git
> >
> > We should maybe do something like the patch below:
> 
> Jonathan also mentions another patch
> 
> http://article.gmane.org/gmane.comp.version-control.git/182536

Yeah, I was just reading that. Sorry, I should have read the rest of the
thread more carefully. :)

> >  1. There is some information leakage there. In particular, one can
> >     tell the difference now between "repo does not exist" and
> >     "receive-pack is not turned on". Personally, I think the tradeoff
> >     to have actual error messages is worth it. HTTP has had real error
> >     codes for decades, and I don't think anybody is too up-in-arms that
> >     I can probe which pages are 404, and which are 401.
> 
> To me, just "<service>: access denied" is enough. Not particularly
> friendly but should be a good enough clue.

Yeah, maybe. Certainly it's better than "the remote end hung up
unexpectedly".

However, the leakage is still there. You would get "the remote hung up"
for no-such-repo, and "access denied" for this. Or were you just
proposing that _all_ errors give "access denied". Certainly it's better
than just hanging up, too, and there is no leakage there.

It might be nice to default to that, and let sites easily enable
friendlier messages, though.

-Peff

Re: [PATCH] transport: do not allow to push over git:// protocol

[Nguyen Thai Ngoc Duy]

On Mon, Oct 3, 2011 at 8:47 PM, Jeff King <peff@peff.net> wrote:
>> To me, just "<service>: access denied" is enough. Not particularly
>> friendly but should be a good enough clue.
>
> Yeah, maybe. Certainly it's better than "the remote end hung up
> unexpectedly".
>
> However, the leakage is still there. You would get "the remote hung up"
> for no-such-repo, and "access denied" for this. Or were you just
> proposing that _all_ errors give "access denied". Certainly it's better
> than just hanging up, too, and there is no leakage there.

All of them. At least it's good to know my request has reached (and
rejected by) the server, not dropped on the floor by some random
firewall along the line.

> It might be nice to default to that, and let sites easily enable
> friendlier messages, though.

I'm thinking of passing "verbose" option back to server to get more
helpful messages, the option would be turned off by default. It's up
to admin to decide (would be actually helpful during deployment test,
for example). Or is it possible already?
-- 
Duy

Re: [PATCH] transport: do not allow to push over git:// protocol

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> To me, just "<service>: access denied" is enough. Not particularly
> friendly but should be a good enough clue.

Yes, I think you're right.  It also has the benefit of being easily
parsable, so some day the client might learn to give a friendly
message in the operator's chosen language.

[PATCH] daemon: print "access denied" if a service does not work

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Note that if a service fails, then "access denied" is printed too.
 Not sure if it's a good thing, the service in question may have
 responded to user already. On the other hand, this catches faults
 from start_command() in run_service_command().

 daemon.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/daemon.c b/daemon.c
index 4c8346d..6552ca7 100644
--- a/daemon.c
+++ b/daemon.c
@@ -562,7 +562,10 @@ static int execute(void)
 			 * Note: The directory here is probably context sensitive,
 			 * and might depend on the actual service being performed.
 			 */
-			return run_service(line + namelen + 5, s);
+			if (!run_service(line + namelen + 5, s))
+				return 0;
+			packet_write(1, "ERR %s: access denied", line + namelen + 5);
+			return -1;
 		}
 	}
 
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH] daemon: print "access denied" if a service does not work

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> --- a/daemon.c
> +++ b/daemon.c
> @@ -562,7 +562,10 @@ static int execute(void)
>  			 * Note: The directory here is probably context sensitive,
>  			 * and might depend on the actual service being performed.
>  			 */
> -			return run_service(line + namelen + 5, s);
> +			if (!run_service(line + namelen + 5, s))
> +				return 0;
> +			packet_write(1, "ERR %s: access denied", line + namelen + 5);
> +			return -1;
>  		}

At first I liked the simplification relative to the patch I sent.
This means the error message is shown when

 1. the service is not enabled at all
 2. path not allowed (for example because it doesn't exist, because
    of permission problems, or because it is blacklisted)
 3. the repository is not exported
 4. the service is not enabled for $path
 5. the service command exited with nonzero status

Unfortunately I think that last case (#5) would be confusing and would
break protocol, especially when the command dies at an inconvenient
moment.  Better for the service command to send an appropriate error
indicator and to just hang up when it fails to do so.

Re: [PATCH] daemon: print "access denied" if a service does not work

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Note that if a service fails, then "access denied" is printed too.

After run_service() returns with a failure, it is simply irresponsible to
append an "ERR" packet to the output channel without knowing what the
state of that channel is (e.g. it might be that the service wrote only
half a pkt_line it wanted to write, and you may be appending to it).

I think the earlier patch from Peff makes the division of responsibility
clearer. If the daemon's dispatch code notices it does not want to run it,
it is the daemon's job to report it. Otherwise the service can and should
report what it does, and after it starts running, the channel belongs to
the service.

[PATCH] daemon: return "access denied" if a service is not allowed

[Nguyễn Thái Ngọc Duy]

The message is chosen to avoid leaking information, yet let users know
that they are deliberately not allowed to use the service, not a fault
in service configuration or the service itself.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 OK let's try again. I don't send ERR when faults happen in
 service->fn() (eventually run_service_command) because

  - if it's start_command(), it's likely due to service configuration
    fault (wrong --exec-path..)

  - if it's finish_command(), the service may have run and sent
    something back to users. We may break the protocol by sending ERR

 daemon.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index 4c8346d..f0cae24 100644
--- a/daemon.c
+++ b/daemon.c
@@ -257,11 +257,11 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
 		errno = EACCES;
-		return -1;
+		goto failed;
 	}
 
 	if (!(path = path_ok(dir)))
-		return -1;
+		goto failed;
 
 	/*
 	 * Security on the cheap.
@@ -277,7 +277,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
 		logerror("'%s': repository not exported.", path);
 		errno = EACCES;
-		return -1;
+		goto failed;
 	}
 
 	if (service->overridable) {
@@ -291,7 +291,7 @@ static int run_service(char *dir, struct daemon_service *service)
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
 		errno = EACCES;
-		return -1;
+		goto failed;
 	}
 
 	/*
@@ -301,6 +301,10 @@ static int run_service(char *dir, struct daemon_service *service)
 	signal(SIGTERM, SIG_IGN);
 
 	return service->fn();
+
+failed:
+	packet_write(1, "ERR %s: access denied", dir);
+	return -1;
 }
 
 static void copy_to_log(int fd)
-- 
1.7.3.1.256.g2539c.dirty

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> The message is chosen to avoid leaking information, yet let users know
> that they are deliberately not allowed to use the service, not a fault
> in service configuration or the service itself.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  OK let's try again. I don't send ERR when faults happen in
>  service->fn() (eventually run_service_command) because
>
>   - if it's start_command(), it's likely due to service configuration
>     fault (wrong --exec-path..)
>
>   - if it's finish_command(), the service may have run and sent
>     something back to users. We may break the protocol by sending ERR
>
>  daemon.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 4c8346d..f0cae24 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -257,11 +257,11 @@ static int run_service(char *dir, struct daemon_service *service)
>  	if (!enabled && !service->overridable) {
>  		logerror("'%s': service not enabled.", service->name);
>  		errno = EACCES;
> -		return -1;
> +		goto failed;
>  	}
>  
>  	if (!(path = path_ok(dir)))
> -		return -1;
> +		goto failed;
>  
>  	/*
>  	 * Security on the cheap.
> @@ -277,7 +277,7 @@ static int run_service(char *dir, struct daemon_service *service)
>  	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
>  		logerror("'%s': repository not exported.", path);
>  		errno = EACCES;
> -		return -1;
> +		goto failed;
>  	}
>  
>  	if (service->overridable) {
> @@ -291,7 +291,7 @@ static int run_service(char *dir, struct daemon_service *service)
>  		logerror("'%s': service not enabled for '%s'",
>  			 service->name, path);
>  		errno = EACCES;
> -		return -1;
> +		goto failed;
>  	}
>  
>  	/*
> @@ -301,6 +301,10 @@ static int run_service(char *dir, struct daemon_service *service)
>  	signal(SIGTERM, SIG_IGN);
>  
>  	return service->fn();
> +
> +failed:
> +	packet_write(1, "ERR %s: access denied", dir);
> +	return -1;
>  }

This looks better.

I think telling "dir" back to the user is probably safe (it is not
affected by what path_ok() does).

I briefly wondered if we also want to say which service failed, but "dir"
is much more likely to be typoed and deserves to be parroted back to help
the user realize mistakes, while the service name is not something the
user usually types, so the balance the patch strikes is probably the
optimal.

Thanks.

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jeff King]

On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:

> The message is chosen to avoid leaking information, yet let users know
> that they are deliberately not allowed to use the service, not a fault
> in service configuration or the service itself.

I do think this is an improvement, but I wonder if the verbosity should
be configurable. Then open sites like kernel.org could be friendlier to
their users. Something like this instead:

---
 daemon.c |   21 +++++++++++++++++----
 1 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index 4c8346d..ec88fd0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -20,6 +20,7 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
+static int informative_errors;
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"
@@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+static int daemon_error(const char *dir, const char *msg)
+{
+	if (!informative_errors)
+		msg = "access denied";
+	packet_write(1, "ERR %s: %s", dir, msg);
+	return -1;
+}
+
 static int run_service(char *dir, struct daemon_service *service)
 {
 	const char *path;
@@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "service not enabled");
 	}
 
 	if (!(path = path_ok(dir)))
-		return -1;
+		return daemon_error(dir, "no such repository");
 
 	/*
 	 * Security on the cheap.
@@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
 		logerror("'%s': repository not exported.", path);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "repository not exported");
 	}
 
 	if (service->overridable) {
@@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "service not enabled");
 	}
 
 	/*
@@ -1167,6 +1176,10 @@ int main(int argc, char **argv)
 			make_service_overridable(arg + 18, 0);
 			continue;
 		}
+		if (!prefixcmp(arg, "--informative-errors")) {
+			informative_errors = 1;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {
 			ok_paths = &argv[i+1];
 			break;
-- 
1.7.7.rc2.21.gb9948

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jonathan Nieder]

(+cc: Andreas[*])
Jeff King wrote:
> On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:

>> The message is chosen to avoid leaking information, yet let users know
>> that they are deliberately not allowed to use the service, not a fault
>> in service configuration or the service itself.
>
> I do think this is an improvement, but I wonder if the verbosity should
> be configurable. Then open sites like kernel.org could be friendlier to
> their users. Something like this instead:

FWIW the more verbose version you suggest also sounds fine to me.  A
person trying to find the names of local users by checking for
repositories with names like "/home/user" would always receive the
error "no such repository", whether that user exists or not and
whether the actual error encountered was ENOENT, EACCES, lack of git
metadata, or the path running afoul of a whitelist or blacklist.

Either Duy's patch or this patch sounds very good to me.  Thanks to
both of you for working on it.

[*] context:
http://thread.gmane.org/gmane.comp.version-control.git/182529/focus=183409

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Nguyen Thai Ngoc Duy]

On Wed, Oct 12, 2011 at 04:09:16PM -0400, Jeff King wrote:
> On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:
> 
> > The message is chosen to avoid leaking information, yet let users know
> > that they are deliberately not allowed to use the service, not a fault
> > in service configuration or the service itself.
> 
> I do think this is an improvement, but I wonder if the verbosity should
> be configurable. Then open sites like kernel.org could be friendlier to
> their users. Something like this instead:

How about allow users to select which messages they want to print? We
can even go further, allowing users to specify the messages themselves..

I don't know. I'm not a real server admin so maybe I'm just too
paranoid. Any admins care to speak up?

On the other hand, grouping all messages at one place may be easier to
audit, even if we don't allow customization.

Anyway, two cents on top of your patch..

-- 8< --
diff --git a/daemon.c b/daemon.c
index ec88fd0..a846ef1 100644
--- a/daemon.c
+++ b/daemon.c
@@ -17,10 +17,25 @@
 #define initgroups(x, y) (0) /* nothing */
 #endif
 
+/* Must match messages[] order below */
+#define MSG_SERVICE_NOT_ENABLED     0
+#define MSG_NO_SUCH_REPOSITORY      1
+#define MSG_REPOSITORY_NOT_EXPORTED 2
+
+static struct daemon_message
+{
+	const char *message;
+	const char *config;
+	int enabled;
+} messages[] = {
+	{ "service not enabled", "message.serviceNotEnabled" },
+	{ "no such repository", "message.noSuchRepository" },
+	{ "repository not exported", "message.repositoryNotExported" },
+};
+
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
-static int informative_errors;
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"
@@ -238,20 +253,31 @@ static int service_enabled;
 
 static int git_daemon_config(const char *var, const char *value, void *cb)
 {
+	int i;
+
 	if (!prefixcmp(var, "daemon.") &&
 	    !strcmp(var + 7, service_looking_at->config_name)) {
 		service_enabled = git_config_bool(var, value);
 		return 0;
 	}
 
+	for (i = 0; i < ARRAY_SIZE(messages); i++)
+		if (!strcmp(var, messages[i].config)) {
+			messages[i].enabled = git_config_bool(var, value);
+			return 0;
+		}
+
 	/* we are not interested in parsing any other configuration here */
 	return 0;
 }
 
-static int daemon_error(const char *dir, const char *msg)
+static int daemon_error(const char *dir, int msg_id)
 {
-	if (!informative_errors)
+	const char *msg;
+	if (!messages[msg_id].enabled)
 		msg = "access denied";
+	else
+		msg = messages[msg_id].message;
 	packet_write(1, "ERR %s: %s", dir, msg);
 	return -1;
 }
@@ -266,11 +292,11 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
 		errno = EACCES;
-		return daemon_error(dir, "service not enabled");
+		return daemon_error(dir, MSG_SERVICE_NOT_ENABLED);
 	}
 
 	if (!(path = path_ok(dir)))
-		return daemon_error(dir, "no such repository");
+		return daemon_error(dir, MSG_NO_SUCH_REPOSITORY);
 
 	/*
 	 * Security on the cheap.
@@ -286,7 +312,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
 		logerror("'%s': repository not exported.", path);
 		errno = EACCES;
-		return daemon_error(dir, "repository not exported");
+		return daemon_error(dir, MSG_REPOSITORY_NOT_EXPORTED);
 	}
 
 	if (service->overridable) {
@@ -300,7 +326,7 @@ static int run_service(char *dir, struct daemon_service *service)
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
 		errno = EACCES;
-		return daemon_error(dir, "service not enabled");
+		return daemon_error(dir, MSG_SERVICE_NOT_ENABLED);
 	}
 
 	/*
@@ -1177,7 +1203,9 @@ int main(int argc, char **argv)
 			continue;
 		}
 		if (!prefixcmp(arg, "--informative-errors")) {
-			informative_errors = 1;
+			int i;
+			for (i = 0; i < ARRAY_SIZE(messages); i++)
+				messages[i].enabled = 1;
 			continue;
 		}
 		if (!strcmp(arg, "--")) {
-- 8< --

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> How about allow users to select which messages they want to print? We
> can even go further, allowing users to specify the messages themselves..
[...]
> +	{ "service not enabled", "message.serviceNotEnabled" },
> +	{ "no such repository", "message.noSuchRepository" },
> +	{ "repository not exported", "message.repositoryNotExported" },

I administer a private server that is only accessible as "localhost".
:)  This much customization would leave me confused about what the
right choices are and what the choices mean (even if I were to make
the server public and start having security worries).

What is the intended use --- translation?  The idealist in me thinks
that should be taken care of on the client side, if at all.  (This
way, we would not be preventing especially friendly clients from
offering pertinent detailed advice for each error condition.
Alternatively, maybe some day the protocol will want to provide a way
for clients to indicate a preferred language and message verbosity.)

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Nguyen Thai Ngoc Duy]

On Thu, Oct 13, 2011 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Nguyen Thai Ngoc Duy wrote:
>
>> How about allow users to select which messages they want to print? We
>> can even go further, allowing users to specify the messages themselves..
> [...]
>> +     { "service not enabled", "message.serviceNotEnabled" },
>> +     { "no such repository", "message.noSuchRepository" },
>> +     { "repository not exported", "message.repositoryNotExported" },
>
> I administer a private server that is only accessible as "localhost".
> :)  This much customization would leave me confused about what the
> right choices are and what the choices mean (even if I were to make
> the server public and start having security worries).

--informative-errors is your friend. All errors are enabled.

> What is the intended use --- translation?  The idealist in me thinks
> that should be taken care of on the client side, if at all.  (This
> way, we would not be preventing especially friendly clients from
> offering pertinent detailed advice for each error condition.
> Alternatively, maybe some day the protocol will want to provide a way
> for clients to indicate a preferred language and message verbosity.)

Translation could be fun to do, but it's more about how much admins
want to reveal. For example, I may only want to show "service not
enabled" and "no such repository", not the last one, which simply
becomes "access denied".

Again I'm not real admin and this may be just bogus.
-- 
Duy

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Nguyen Thai Ngoc Duy]

On Thu, Oct 13, 2011 at 5:56 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> Translation could be fun to do

By translation, I don't mean inter-language translation. More like
personification. Instead of "service not enabled" you may want
"service is off, you want to attack me or what?"
-- 
Duy

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jeff King]

On Thu, Oct 13, 2011 at 03:45:44PM +1100, Nguyen Thai Ngoc Duy wrote:

> On Wed, Oct 12, 2011 at 04:09:16PM -0400, Jeff King wrote:
> > On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:
> > 
> > > The message is chosen to avoid leaking information, yet let users know
> > > that they are deliberately not allowed to use the service, not a fault
> > > in service configuration or the service itself.
> > 
> > I do think this is an improvement, but I wonder if the verbosity should
> > be configurable. Then open sites like kernel.org could be friendlier to
> > their users. Something like this instead:
> 
> How about allow users to select which messages they want to print? We
> can even go further, allowing users to specify the messages themselves..

I thought about that, but it just seemed like it was making things way
more complex than it needed to be. GitHub does do this kind of
customization, but we also have a custom layer that intercepts git://
connections, anyway, so we added the relevant code there.

I don't know if medium-sized sites (i.e., ones that aren't so big they
are running custom proxies on the frontend) would care about adding
custom messages here or not.

> I don't know. I'm not a real server admin so maybe I'm just too
> paranoid. Any admins care to speak up?

I doubt anybody would care that much about turning individual messages
on and off. I think the real value is in being able to say "don't push
by git://. The right way to push to this site is...".

But your patch kind of falls short of what people would want to do for
two reasons:

  1. The message isn't dynamic at all. So I can't say:

        You tried to push to git://host.tld/foo.git. The right way to do
        that is:

          git push https://host.tld/foo.git

     That's what the GitHub message does if you try to push over git://;
     it gives you a new remote name that will actually work, customized
     to the repo you wanted to push to.

  2. Tweaking just the message for anything but "service not enabled"
     isn't all that useful. What do you say about "no such repository"
     in a simple message, even with placeholders?

     If you _really_ want to get fancy, a server could do a fuzzy
     search on the available repos and say "did you mean...?".
     But now we are talking about hooking arbitrary code into the
     message.

So if we want to do anything, I would think it would be a hook. Except
that we may or may not have a repo, so it would not be a hook in
$GIT_DIR/hooks, but rather some script to be run passed on the command
line, like:

  git daemon --informative-errors=/path/to/hook

-Peff

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So if we want to do anything, I would think it would be a hook. Except
> that we may or may not have a repo, so it would not be a hook in
> $GIT_DIR/hooks, but rather some script to be run passed on the command
> line, like:
>
>   git daemon --informative-errors=/path/to/hook

I don't think it is necessarily good to have such a variation across
hosting sites. Your "something like this" patch looked like it was giving
a reasonable level of detail, IMO.

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jeff King]

On Thu, Oct 13, 2011 at 10:01:01PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So if we want to do anything, I would think it would be a hook. Except
> > that we may or may not have a repo, so it would not be a hook in
> > $GIT_DIR/hooks, but rather some script to be run passed on the command
> > line, like:
> >
> >   git daemon --informative-errors=/path/to/hook
> 
> I don't think it is necessarily good to have such a variation across
> hosting sites. Your "something like this" patch looked like it was giving
> a reasonable level of detail, IMO.

Yeah. With arbitrary messages, the client has no way of programatically
deciphering which message is which, so localization becomes impossible.
HTTP solved this by having a response code _and_ still allowing content
for custom pages.  That kind of works, though most of the time I find
things like custom 404 pages to just be junk.

Let's start with my original patch (which I'll clean and repost). And if
somebody really wants to push towards customized messages, that is easy
enough to do on top later.

-Peff

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jeff King]

On Fri, Oct 14, 2011 at 09:10:41AM -0400, Jeff King wrote:

> Let's start with my original patch (which I'll clean and repost). And if
> somebody really wants to push towards customized messages, that is easy
> enough to do on top later.

Here it is, with a few minor tweaks:

  1. git-daemon respects --no-informative-errors, too

  2. the new flag is documented

  3. I switched the format from:

       fatal: remote: /path/in/your/git/url: access denied

     to:

       fatal: remote: access denied: /path/in/your/git/url

     I find the latter easier to read, and it would be easier for a
     client to recognize.

  4. there's now a commit message

-- >8 --
Subject: [PATCH] daemon: give friendlier error messages to clients

When the git-daemon is asked about an inaccessible
repository, it simply hangs up the connection without saying
anything further. This makes it hard to distinguish between
a repository we cannot access (e.g., due to typo), and a
service or network outage.

Instead, let's print an "ERR" line, which git clients
understand since v1.6.1 (2008-12-24).

Because there is a risk of leaking information about
non-exported repositories, by default all errors simply say
"access denied". Open sites can pass a flag to turn on more
specific messages.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-daemon.txt |    8 ++++++++
 daemon.c                     |   25 +++++++++++++++++++++----
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 69a1e4a..ac57c6d 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -161,6 +161,14 @@ the facility of inet daemon to achieve the same before spawning
 	repository configuration.  By default, all the services
 	are overridable.
 
+--informative-errors::
+	Return more verbose errors to the client, differentiating
+	conditions like "no such repository" from "repository not
+	exported". This is more convenient for clients, but may leak
+	information about the existence of unexported repositories.
+	Without this option, all errors report "access denied" to the
+	client.
+
 <directory>::
 	A directory to add to the whitelist of allowed directories. Unless
 	--strict-paths is specified this will also include subdirectories
diff --git a/daemon.c b/daemon.c
index 4c8346d..e5869ec 100644
--- a/daemon.c
+++ b/daemon.c
@@ -20,6 +20,7 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
+static int informative_errors;
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"
@@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+static int daemon_error(const char *dir, const char *msg)
+{
+	if (!informative_errors)
+		msg = "access denied";
+	packet_write(1, "ERR %s: %s", msg, dir);
+	return -1;
+}
+
 static int run_service(char *dir, struct daemon_service *service)
 {
 	const char *path;
@@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "service not enabled");
 	}
 
 	if (!(path = path_ok(dir)))
-		return -1;
+		return daemon_error(dir, "no such repository");
 
 	/*
 	 * Security on the cheap.
@@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
 		logerror("'%s': repository not exported.", path);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "repository not exported");
 	}
 
 	if (service->overridable) {
@@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "service not enabled");
 	}
 
 	/*
@@ -1167,6 +1176,14 @@ int main(int argc, char **argv)
 			make_service_overridable(arg + 18, 0);
 			continue;
 		}
+		if (!prefixcmp(arg, "--informative-errors")) {
+			informative_errors = 1;
+			continue;
+		}
+		else if (!prefixcmp(arg, "--no-informative-errors")) {
+			informative_errors = 0;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {
 			ok_paths = &argv[i+1];
 			break;
-- 
1.7.6.4.37.g43b58b

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jeff King]

On Fri, Oct 14, 2011 at 03:23:26PM -0400, Jeff King wrote:

> Subject: [PATCH] daemon: give friendlier error messages to clients
> 
> When the git-daemon is asked about an inaccessible
> repository, it simply hangs up the connection without saying
> anything further. This makes it hard to distinguish between
> a repository we cannot access (e.g., due to typo), and a
> service or network outage.
> 
> Instead, let's print an "ERR" line, which git clients
> understand since v1.6.1 (2008-12-24).
> 
> Because there is a risk of leaking information about
> non-exported repositories, by default all errors simply say
> "access denied". Open sites can pass a flag to turn on more
> specific messages.

I'm tempted to suggest this on top:

-- >8 --
Subject: [PATCH] daemon: turn on informative errors by default

These are only a problem if you have a bunch of inaccessible
repositories served from the same root as your regular
exported repositories, and you are sensitive about people
learning about the existence of those repositories.

Git is foremost an open system, and our defaults should
reflect that.

Signed-off-by: Jeff King <peff@peff.net>
---
But since it is a potential security issue, it does seem kind of mean to
closed sites to just flip the switch on them.

 Documentation/git-daemon.txt |    6 +++---
 daemon.c                     |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index ac57c6d..2b17175 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -161,12 +161,12 @@ the facility of inet daemon to achieve the same before spawning
 	repository configuration.  By default, all the services
 	are overridable.
 
---informative-errors::
-	Return more verbose errors to the client, differentiating
+--no-informative-errors::
+	By default, we return verbose errors to the client, differentiating
 	conditions like "no such repository" from "repository not
 	exported". This is more convenient for clients, but may leak
 	information about the existence of unexported repositories.
-	Without this option, all errors report "access denied" to the
+	With this option, all errors report "access denied" to the
 	client.
 
 <directory>::
diff --git a/daemon.c b/daemon.c
index e5869ec..ba41a40 100644
--- a/daemon.c
+++ b/daemon.c
@@ -20,7 +20,7 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
-static int informative_errors;
+static int informative_errors = 1;
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"
-- 
1.7.6.4.37.g43b58b

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Subject: [PATCH] daemon: turn on informative errors by default
>
> These are only a problem if you have a bunch of inaccessible
> repositories served from the same root as your regular
> exported repositories, and you are sensitive about people
> learning about the existence of those repositories.
>
> Git is foremost an open system, and our defaults should
> reflect that.
>
> Signed-off-by: Jeff King <peff@peff.net>

I think the logic in the last paragraph is flawed.

There is a difference between Git being an open system, and installations
and users of Git being primarily people who work on open projects.

Even though personally I wish there weren't.

> But since it is a potential security issue, it does seem kind of mean to
> closed sites to just flip the switch on them.

It would have been a better split to have the 1/2 patch to support both
informative and uninformative errors, with the default to say "access
denied", and 2/2 to flip the default to be more open.

Will queue as-is, though.

>  Documentation/git-daemon.txt |    6 +++---
>  daemon.c                     |    2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> index ac57c6d..2b17175 100644
> --- a/Documentation/git-daemon.txt
> +++ b/Documentation/git-daemon.txt
> @@ -161,12 +161,12 @@ the facility of inet daemon to achieve the same before spawning
>  	repository configuration.  By default, all the services
>  	are overridable.
>  
> ---informative-errors::
> -	Return more verbose errors to the client, differentiating
> +--no-informative-errors::
> +	By default, we return verbose errors to the client, differentiating
>  	conditions like "no such repository" from "repository not
>  	exported". This is more convenient for clients, but may leak
>  	information about the existence of unexported repositories.
> -	Without this option, all errors report "access denied" to the
> +	With this option, all errors report "access denied" to the
>  	client.

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jeff King]

On Fri, Oct 14, 2011 at 01:24:07PM -0700, Junio C Hamano wrote:

> > Git is foremost an open system, and our defaults should
> > reflect that.
> [...]
> 
> I think the logic in the last paragraph is flawed.
> 
> There is a difference between Git being an open system, and installations
> and users of Git being primarily people who work on open projects.
> 
> Even though personally I wish there weren't.

I think it is not the logic that is flawed, but the communication. What
I meant was that git was originally designed to support open projects
(like the kernel), and they are our primary target.

Ingo said something similar here:

  http://article.gmane.org/gmane.linux.kernel/1202320

Still, primary target and primary user are not necessarily the same
thing. And a minor convenience for one audience that introduces a
security problem for another audience may not be a good tradeoff, no
matter who the audiences are.

I didn't really expect you to take my second patch. We tend to be a bit
more conservative than that around here.

> > But since it is a potential security issue, it does seem kind of mean to
> > closed sites to just flip the switch on them.
> 
> It would have been a better split to have the 1/2 patch to support both
> informative and uninformative errors, with the default to say "access
> denied", and 2/2 to flip the default to be more open.

Isn't that what I did? It was what I meant to do, anyway...

Or did you mean the options would have been better worded as:

  --errors={terse,informative}

or something similar?

-Peff

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> It would have been a better split to have the 1/2 patch to support both
>> informative and uninformative errors, with the default to say "access
>> denied", and 2/2 to flip the default to be more open.
>
> Isn't that what I did? It was what I meant to do, anyway...
>
> Or did you mean the options would have been better worded as:
>
>   --errors={terse,informative}
>
> or something similar?

Nothing that elaborate.

Supporting --no-* variant even when the default is already no will allow
people to prepare their daemon invocation command line beforehand to ensure
that they won't be affected to a more lenient default that may or may not
come in the future.  That's all.

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jeff King]

On Fri, Oct 14, 2011 at 01:48:51PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> It would have been a better split to have the 1/2 patch to support both
> >> informative and uninformative errors, with the default to say "access
> >> denied", and 2/2 to flip the default to be more open.
> >
> > Isn't that what I did? It was what I meant to do, anyway...
> >
> > Or did you mean the options would have been better worded as:
> >
> >   --errors={terse,informative}
> >
> > or something similar?
> 
> Nothing that elaborate.
> 
> Supporting --no-* variant even when the default is already no will allow
> people to prepare their daemon invocation command line beforehand to ensure
> that they won't be affected to a more lenient default that may or may not
> come in the future.  That's all.

Oh. Then look again at 1/2. It supports both forms; I just didn't bother
advertising the --no form in the manpage, since it was the default.

-Peff

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jonathan Nieder]

Jeff King wrote:

> Oh. Then look again at 1/2. It supports both forms; I just didn't bother
> advertising the --no form in the manpage, since it was the default.

Yes, and that was the bug Junio mentioned.  If we are considering 2/2
then admins will need to know about the --no form before we roll out
the change in default. :)

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jonathan Nieder]

Jeff King wrote:

> I'm tempted to suggest this on top:
>
> -- >8 --
> Subject: [PATCH] daemon: turn on informative errors by default

Very good idea, as long as we are cautious about making sure admins
know about the change before it comes (for example using release
notes).

[...]
> Git is foremost an open system, and our defaults should
> reflect that.

I think this is a lousy justification. :)

Sure, certain prominent users of git (like kernel.org) would probably
not want to set --no-informative-errors.  And you and I might prefer
that _nobody_ set --no-informative-errors.  But this does not mean the
design of Git has anything to do with that, and I am afraid of the
direction it would take us in if we start pretending it does.

The git daemon is primarily a functional, secure, admin-friendly and
client-friendly program whose defaults should make admins happy where
possible.  Luckily all that is consistent with --informative-errors.
In most use cases (i.e., not weird military security kinds of things),
although --informative-errors without --base-path could have negative
security impact as Andreas has explained before, --informative-errors
with --base-path is harmless as far as I can tell.

I am tempted to propose making --base-path mandatory when there is not
at least one path_ok argument, so in the unusual case, people would
have to explicitly say they want to serve repositories rooted at /.

Just my two cents,
Jonathan

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jonathan Nieder]

Jeff King wrote:

> When the git-daemon is asked about an inaccessible
> repository, it simply hangs up the connection without saying
> anything further. This makes it hard to distinguish between
> a repository we cannot access (e.g., due to typo), and a
> service or network outage.

*nod*

> Instead, let's print an "ERR" line, which git clients
> understand since v1.6.1 (2008-12-24).

Just to be clear, "git archive --remote" does not understand ERR lines
in the 'master' branch (though 908aaceb makes it understand them in
'next').  But I consider even distinguishing

 a. fatal: git archive: protocol error
 b. fatal: git archive: expected ACK/NAK, got EOF

[(a) is how an ERR response is reported, and (b) a remote hangup] to
be progress, so it's not so important. :)

> Because there is a risk of leaking information about
> non-exported repositories, by default all errors simply say
> "access denied". Open sites can pass a flag to turn on more
> specific messages.

I'm not sure what an "open site" is. :)  But having this flag for
sites to declare whether they consider whether a repository exists to
be privileged information seems reasonable to me.

Note that this really would be privileged information in some
not-too-weird cases.  For example, if many users have a repository at
~/.git, ~/.config/.git, or ~/src/linux/.git, then someone might try to
access

	/home/alice/.git
	/home/alice/.config/.git
	/home/alice/src/linux/.git
	/home/bob/.git
	...

in turn to find a valid username, as reconnaisance for a later
attack not involving git.

Luckily, this can be avoided with

	git daemon --user-path=public_git --base-path=/pub/git

which only allows access to subdirectories of /pub/git and
public_git in home directories.  With

	git daemon --base-path=/pub/git

there is still no problem, since access to home directories is not
allowed at all in that case.  I suppose the documentation should
mention that --informative-errors is best not used unless --base-path
is also in use.  (Almost everyone is already using --base-path, so
this isn't a very serious problem.)

> --- a/daemon.c
> +++ b/daemon.c
> @@ -20,6 +20,7 @@
[...]
> @@ -1167,6 +1176,14 @@ int main(int argc, char **argv)
>  			make_service_overridable(arg + 18, 0);
>  			continue;
>  		}
> +		if (!prefixcmp(arg, "--informative-errors")) {
> +			informative_errors = 1;
> +			continue;
> +		}
> +		else if (!prefixcmp(arg, "--no-informative-errors")) {
> +			informative_errors = 0;
> +			continue;
> +		}
>  		if (!strcmp(arg, "--")) {

Micronit: uncuddled "else".  The style of the surrounding code is to
just not include the "else" at all and rely on "continue" to
short-circuit things.

Anyway, except for the documentation nits mentioned above (and Junio's
nit, too),

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks a lot for this.

Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Jeff King]

On Fri, Oct 14, 2011 at 04:02:51PM -0500, Jonathan Nieder wrote:

> > Instead, let's print an "ERR" line, which git clients
> > understand since v1.6.1 (2008-12-24).
> 
> Just to be clear, "git archive --remote" does not understand ERR lines
> in the 'master' branch (though 908aaceb makes it understand them in
> 'next').  But I consider even distinguishing
> 
>  a. fatal: git archive: protocol error
>  b. fatal: git archive: expected ACK/NAK, got EOF
> 
> [(a) is how an ERR response is reported, and (b) a remote hangup] to
> be progress, so it's not so important. :)

Thanks, I forgot to mention that. It's not as nice as the push/fetch
case, but I agree it's a step forward.

> > Because there is a risk of leaking information about
> > non-exported repositories, by default all errors simply say
> > "access denied". Open sites can pass a flag to turn on more
> > specific messages.
> 
> I'm not sure what an "open site" is. :)  But having this flag for
> sites to declare whether they consider whether a repository exists to
> be privileged information seems reasonable to me.

I meant sites which are just serving a bunch of public repos, like
kernel.org.

> Note that this really would be privileged information in some
> not-too-weird cases.  For example, if many users have a repository at
> ~/.git, ~/.config/.git, or ~/src/linux/.git, then someone might try to
> access
> 
> 	/home/alice/.git
> 	/home/alice/.config/.git
> 	/home/alice/src/linux/.git
> 	/home/bob/.git
> 	...
> 
> in turn to find a valid username, as reconnaisance for a later
> attack not involving git.

I sort of assume everybody serves a specific directory hierarchy, but
maybe that is not the case. I don't run git-daemon myself, so I am
probably guilty of generalizing how other people use it.

Anyway, I think the issue is sufficiently nuanced that we should keep
the default to the conservative "access denied" (i.e., throw away my
second patch for now).

> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -20,6 +20,7 @@
> [...]
> > @@ -1167,6 +1176,14 @@ int main(int argc, char **argv)
> >  			make_service_overridable(arg + 18, 0);
> >  			continue;
> >  		}
> > +		if (!prefixcmp(arg, "--informative-errors")) {
> > +			informative_errors = 1;
> > +			continue;
> > +		}
> > +		else if (!prefixcmp(arg, "--no-informative-errors")) {
> > +			informative_errors = 0;
> > +			continue;
> > +		}
> >  		if (!strcmp(arg, "--")) {
> 
> Micronit: uncuddled "else".  The style of the surrounding code is to
> just not include the "else" at all and rely on "continue" to
> short-circuit things.

Oops. Yes, it should just drop the else to match the surrounding code.

-Peff

[PATCHv3] daemon: give friendlier error messages to clients

[Jeff King]

When the git-daemon is asked about an inaccessible
repository, it simply hangs up the connection without saying
anything further. This makes it hard to distinguish between
a repository we cannot access (e.g., due to typo), and a
service or network outage.

Instead, let's print an "ERR" line, which git clients
understand since v1.6.1 (2008-12-24).

Because there is a risk of leaking information about
non-exported repositories, by default all errors simply say
"access denied". Sites which don't have hidden repositories,
or don't care, can pass a flag to turn on more specific
messages.

Signed-off-by: Jeff King <peff@peff.net>
---
Minor tweaks to the documentation and code style to make Jonathan happy.
:)

Note: I labeled this "v3", as it is the third one posted, but the prior
ones were not labeled with versions at all.

 Documentation/git-daemon.txt |   10 ++++++++++
 daemon.c                     |   25 +++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 69a1e4a..31b28fc 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -161,6 +161,16 @@ the facility of inet daemon to achieve the same before spawning
 	repository configuration.  By default, all the services
 	are overridable.
 
+--informative-errors::
+--no-informative-errors::
+	When informative errors are turned on, git-daemon will report
+	more verbose errors to the client, differentiating conditions
+	like "no such repository" from "repository not exported". This
+	is more convenient for clients, but may leak information about
+	the existence of unexported repositories.  When informative
+	errors are not enabled, all errors report "access denied" to the
+	client. The default is --no-informative-errors.
+
 <directory>::
 	A directory to add to the whitelist of allowed directories. Unless
 	--strict-paths is specified this will also include subdirectories
diff --git a/daemon.c b/daemon.c
index 4c8346d..6f111af 100644
--- a/daemon.c
+++ b/daemon.c
@@ -20,6 +20,7 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
+static int informative_errors;
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"
@@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+static int daemon_error(const char *dir, const char *msg)
+{
+	if (!informative_errors)
+		msg = "access denied";
+	packet_write(1, "ERR %s: %s", msg, dir);
+	return -1;
+}
+
 static int run_service(char *dir, struct daemon_service *service)
 {
 	const char *path;
@@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "service not enabled");
 	}
 
 	if (!(path = path_ok(dir)))
-		return -1;
+		return daemon_error(dir, "no such repository");
 
 	/*
 	 * Security on the cheap.
@@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
 		logerror("'%s': repository not exported.", path);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "repository not exported");
 	}
 
 	if (service->overridable) {
@@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "service not enabled");
 	}
 
 	/*
@@ -1167,6 +1176,14 @@ int main(int argc, char **argv)
 			make_service_overridable(arg + 18, 0);
 			continue;
 		}
+		if (!prefixcmp(arg, "--informative-errors")) {
+			informative_errors = 1;
+			continue;
+		}
+		if (!prefixcmp(arg, "--no-informative-errors")) {
+			informative_errors = 0;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {
 			ok_paths = &argv[i+1];
 			break;
-- 
1.7.6.4.37.g43b58b

Re: [PATCHv3] daemon: give friendlier error messages to clients

[Junio C Hamano]

Thanks, both. I'll drop the previous one, and rebuild the integration
branches by queuign this version instead.

Re: [PATCHv3] daemon: give friendlier error messages to clients

[Sitaram Chamarty]

On Sat, Oct 15, 2011 at 2:49 AM, Jeff King <peff@peff.net> wrote:
> When the git-daemon is asked about an inaccessible
> repository, it simply hangs up the connection without saying
> anything further. This makes it hard to distinguish between
> a repository we cannot access (e.g., due to typo), and a
> service or network outage.
>
> Instead, let's print an "ERR" line, which git clients
> understand since v1.6.1 (2008-12-24).
>
> Because there is a risk of leaking information about
> non-exported repositories, by default all errors simply say
> "access denied". Sites which don't have hidden repositories,

I suggest that even the "secure" version of the message say something
like "access denied or repository not exported".  You're still not
leaking anything, but it reduces confusion to the user, who otherwise
may not realise it *could be* the latter.

regards

sitaram

> or don't care, can pass a flag to turn on more specific
> messages.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Minor tweaks to the documentation and code style to make Jonathan happy.
> :)
>
> Note: I labeled this "v3", as it is the third one posted, but the prior
> ones were not labeled with versions at all.
>
>  Documentation/git-daemon.txt |   10 ++++++++++
>  daemon.c                     |   25 +++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> index 69a1e4a..31b28fc 100644
> --- a/Documentation/git-daemon.txt
> +++ b/Documentation/git-daemon.txt
> @@ -161,6 +161,16 @@ the facility of inet daemon to achieve the same before spawning
>        repository configuration.  By default, all the services
>        are overridable.
>
> +--informative-errors::
> +--no-informative-errors::
> +       When informative errors are turned on, git-daemon will report
> +       more verbose errors to the client, differentiating conditions
> +       like "no such repository" from "repository not exported". This
> +       is more convenient for clients, but may leak information about
> +       the existence of unexported repositories.  When informative
> +       errors are not enabled, all errors report "access denied" to the
> +       client. The default is --no-informative-errors.
> +
>  <directory>::
>        A directory to add to the whitelist of allowed directories. Unless
>        --strict-paths is specified this will also include subdirectories
> diff --git a/daemon.c b/daemon.c
> index 4c8346d..6f111af 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -20,6 +20,7 @@
>  static int log_syslog;
>  static int verbose;
>  static int reuseaddr;
> +static int informative_errors;
>
>  static const char daemon_usage[] =
>  "git daemon [--verbose] [--syslog] [--export-all]\n"
> @@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
>        return 0;
>  }
>
> +static int daemon_error(const char *dir, const char *msg)
> +{
> +       if (!informative_errors)
> +               msg = "access denied";
> +       packet_write(1, "ERR %s: %s", msg, dir);
> +       return -1;
> +}
> +
>  static int run_service(char *dir, struct daemon_service *service)
>  {
>        const char *path;
> @@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
>        if (!enabled && !service->overridable) {
>                logerror("'%s': service not enabled.", service->name);
>                errno = EACCES;
> -               return -1;
> +               return daemon_error(dir, "service not enabled");
>        }
>
>        if (!(path = path_ok(dir)))
> -               return -1;
> +               return daemon_error(dir, "no such repository");
>
>        /*
>         * Security on the cheap.
> @@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)
>        if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
>                logerror("'%s': repository not exported.", path);
>                errno = EACCES;
> -               return -1;
> +               return daemon_error(dir, "repository not exported");
>        }
>
>        if (service->overridable) {
> @@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)
>                logerror("'%s': service not enabled for '%s'",
>                         service->name, path);
>                errno = EACCES;
> -               return -1;
> +               return daemon_error(dir, "service not enabled");
>        }
>
>        /*
> @@ -1167,6 +1176,14 @@ int main(int argc, char **argv)
>                        make_service_overridable(arg + 18, 0);
>                        continue;
>                }
> +               if (!prefixcmp(arg, "--informative-errors")) {
> +                       informative_errors = 1;
> +                       continue;
> +               }
> +               if (!prefixcmp(arg, "--no-informative-errors")) {
> +                       informative_errors = 0;
> +                       continue;
> +               }
>                if (!strcmp(arg, "--")) {
>                        ok_paths = &argv[i+1];
>                        break;
> --
> 1.7.6.4.37.g43b58b
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Sitaram

Re: [PATCHv3] daemon: give friendlier error messages to clients

[Junio C Hamano]

Sitaram Chamarty <sitaramc@gmail.com> writes:

>> Because there is a risk of leaking information about
>> non-exported repositories, by default all errors simply say
>> "access denied". Sites which don't have hidden repositories,
>
> I suggest that even the "secure" version of the message say something
> like "access denied or repository not exported".  You're still not
> leaking anything, but it reduces confusion to the user, who otherwise
> may not realise it *could be* the latter.

I kind of like the suggestion, but I am afraid that "access denied,
repository nonexistent or not exported" can soon easily get long enough to
be unmanageable.

Re: [PATCHv3] daemon: give friendlier error messages to clients

[Sitaram Chamarty]

On Sat, Oct 15, 2011 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>>> Because there is a risk of leaking information about
>>> non-exported repositories, by default all errors simply say
>>> "access denied". Sites which don't have hidden repositories,
>>
>> I suggest that even the "secure" version of the message say something
>> like "access denied or repository not exported".  You're still not
>> leaking anything, but it reduces confusion to the user, who otherwise
>> may not realise it *could be* the latter.
>
> I kind of like the suggestion, but I am afraid that "access denied,
> repository nonexistent or not exported" can soon easily get long enough to
> be unmanageable.

When someone who *does* have access makes a typo, "access denied"
makes it harder to realise it, because it subtly implies the repo
*does* exist and it's an ACL issue.  I've seen lots of frustrating
back-and-forth between admin and user before someone eventually
noticed the typo.

"Access denied or no such repo" is much better.  (The "not exported"
nuance is not relevant in this context; you can safely ignore it.)

regards

-- 
Sitaram

Re: [PATCHv3] daemon: give friendlier error messages to clients

[Jakub Narebski]

Sitaram Chamarty <sitaramc@gmail.com> writes:
> On Sat, Oct 15, 2011 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Sitaram Chamarty <sitaramc@gmail.com> writes:
>>
>>>> Because there is a risk of leaking information about
>>>> non-exported repositories, by default all errors simply say
>>>> "access denied". Sites which don't have hidden repositories,
>>>
>>> I suggest that even the "secure" version of the message say something
>>> like "access denied or repository not exported".  You're still not
>>> leaking anything, but it reduces confusion to the user, who otherwise
>>> may not realise it *could be* the latter.
>>
>> I kind of like the suggestion, but I am afraid that "access denied,
>> repository nonexistent or not exported" can soon easily get long enough to
>> be unmanageable.
> 
> When someone who *does* have access makes a typo, "access denied"
> makes it harder to realise it, because it subtly implies the repo
> *does* exist and it's an ACL issue.  I've seen lots of frustrating
> back-and-forth between admin and user before someone eventually
> noticed the typo.
> 
> "Access denied or no such repo" is much better.  (The "not exported"
> nuance is not relevant in this context; you can safely ignore it.)

To join this bike-shedding:

  "Access denied or repository not available"

or just

  "Repository not available"

-- 
Jakub Narębski

Re: [PATCHv3] daemon: give friendlier error messages to clients

[Jonathan Nieder]

Jakub Narebski wrote:
> Sitaram Chamarty <sitaramc@gmail.com> writes:

>> "Access denied or no such repo" is much better.  (The "not exported"
>> nuance is not relevant in this context; you can safely ignore it.)
>
> To join this bike-shedding:
>
>   "Access denied or repository not available"
>
> or just
>
>   "Repository not available"

If such details about the message matter, then I have to say that
Sitaram's "access denied or no such repository" is as close to perfect
as I can imagine.  The admin who is eventually forwarded this message
will be reminded to check two things:

 - that access is not denied, neither globally, using a whitelist,
   using filesystem permissions, nor by leaving out
   git-daemon-export-ok, and

 - that such a repo exists at all, and there was not a typo in the
   address.

Re: [PATCHv3] daemon: give friendlier error messages to clients

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> ... The admin who is eventually forwarded this message
> will be reminded ...

The admin has access to logs that record the real cause anyway, no?
I thought this was all about how the error is given to the end user.

Re: [PATCHv3] daemon: give friendlier error messages to clients

[Jonathan Nieder]

Junio C Hamano wrote:

> The admin has access to logs that record the real cause anyway, no?

Yes, you're right.  If this is a good admin then she will look at the
logs, preventing the back-and-forth Sitaram described.

Though that doesn't really change anything fundamental.  It seems nice
to remind the end user to check for typos, too.

Re: [PATCHv3] daemon: give friendlier error messages to clients

[Sitaram Chamarty]

On Sun, Oct 16, 2011 at 3:47 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Junio C Hamano wrote:
>
>> The admin has access to logs that record the real cause anyway, no?
>
> Yes, you're right.  If this is a good admin then she will look at the
> logs, preventing the back-and-forth Sitaram described.

Actually, even if it's a good admin, you're adding to her load needlessly.

> Though that doesn't really change anything fundamental.  It seems nice
> to remind the end user to check for typos, too.

Yup.

DId I mention "been there, done that" in my earlier email?  I'm not
bike-shedding -- there *is* an impact on productivity in terms of how
people troubleshoot when they run across a problem, and I really *do*
feel strongly about this in principle (even though I don't use
git-daemon myself so it doesn't bother me how you decide in this
*specific* case)

I'll shut up now... :-)

-- 
Sitaram

Re: [PATCHv3] daemon: give friendlier error messages to clients

[Nguyen Thai Ngoc Duy]

On Sat, Oct 15, 2011 at 8:19 AM, Jeff King <peff@peff.net> wrote:
> @@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
>        if (!enabled && !service->overridable) {
>                logerror("'%s': service not enabled.", service->name);
>                errno = EACCES;
> -               return -1;
> +               return daemon_error(dir, "service not enabled");
>        }

Nit picking. In this case the service is disabled entirely regardless
dir and it uses the same message with the case where service is
disabled per repo later on. Maybe we could reword it a little bit to
differentiate the two cases? Say the first case "service disabled",
and the second one "service not enabled"?
-- 
Duy

[PATCH 1/2] daemon: add tests

[Clemens Buchacher]

The semantics of the git daemon tests are similar to the http
transport tests.  In fact, they are only a slightly modified copy
of t5550, plus the newly added remote error tests.

All daemon tests will be skipped unless the environment variable
GIT_TEST_DAEMON is set.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

This patch is based on jk/daemon-msgs.

 t/lib-daemon.sh       |   52 +++++++++++++++++
 t/t5570-git-daemon.sh |  148 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 200 insertions(+), 0 deletions(-)
 create mode 100644 t/lib-daemon.sh
 create mode 100755 t/t5570-git-daemon.sh

diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh
new file mode 100644
index 0000000..30a89ea
--- /dev/null
+++ b/t/lib-daemon.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+if test -z "$GIT_TEST_DAEMON"
+then
+	skip_all="Daemon testing disabled (define GIT_TEST_DAEMON to enable)"
+	test_done
+fi
+
+LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}
+
+DAEMON_PID=
+DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
+DAEMON_URL=git://127.0.0.1:$LIB_DAEMON_PORT
+
+start_daemon() {
+	if test -n "$DAEMON_PID"
+	then
+		error "start_daemon already called"
+	fi
+
+	mkdir -p "$DAEMON_DOCUMENT_ROOT_PATH"
+
+	trap 'code=$?; stop_daemon; (exit $code); die' EXIT
+
+	say >&3 "Starting git daemon ..."
+	git daemon --listen=127.0.0.1 --port="$LIB_DAEMON_PORT" \
+		--reuseaddr --verbose \
+		--base-path="$DAEMON_DOCUMENT_ROOT_PATH" \
+		"$@" "$DAEMON_DOCUMENT_ROOT_PATH" \
+		>&3 2>&4 &
+	DAEMON_PID=$!
+}
+
+stop_daemon() {
+	if test -z "$DAEMON_PID"
+	then
+		return
+	fi
+
+	trap 'die' EXIT
+
+	# kill git-daemon child of git
+	say >&3 "Stopping git daemon ..."
+	pkill -P "$DAEMON_PID"
+	wait "$DAEMON_PID"
+	ret=$?
+	if test $ret -ne 143
+	then
+		error "git daemon exited with status: $ret"
+	fi
+	DAEMON_PID=
+}
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
new file mode 100755
index 0000000..aa5771a
--- /dev/null
+++ b/t/t5570-git-daemon.sh
@@ -0,0 +1,148 @@
+#!/bin/sh
+
+test_description='test fetching over git protocol'
+. ./test-lib.sh
+
+. "$TEST_DIRECTORY"/lib-daemon.sh
+start_daemon
+
+test_expect_success 'setup repository' '
+	echo content >file &&
+	git add file &&
+	git commit -m one
+'
+
+test_expect_success 'create git-accessible bare repository' '
+	mkdir "$DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	(cd "$DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	 git --bare init &&
+	 : >git-daemon-export-ok
+	) &&
+	git remote add public "$DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push public master:master
+'
+
+test_expect_success 'clone git repository' '
+	git clone $DAEMON_URL/repo.git clone &&
+	test_cmp file clone/file
+'
+
+test_expect_success 'fetch changes via git protocol' '
+	echo content >>file &&
+	git commit -a -m two &&
+	git push public &&
+	(cd clone && git pull) &&
+	test_cmp file clone/file
+'
+
+test_expect_failure 'remote detects correct HEAD' '
+	git push public master:other &&
+	(cd clone &&
+	 git remote set-head -d origin &&
+	 git remote set-head -a origin &&
+	 git symbolic-ref refs/remotes/origin/HEAD > output &&
+	 echo refs/remotes/origin/master > expect &&
+	 test_cmp expect output
+	)
+'
+
+test_expect_success 'prepare pack objects' '
+	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	 git --bare repack &&
+	 git --bare prune-packed
+	)
+'
+
+test_expect_success 'fetch notices corrupt pack' '
+	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+	 p=`ls objects/pack/pack-*.pack` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad1.git &&
+	(cd repo_bad1.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch $DAEMON_URL/repo_bad1.git &&
+	 test 0 = `ls objects/pack/pack-*.pack | wc -l`
+	)
+'
+
+test_expect_success 'fetch notices corrupt idx' '
+	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	 p=`ls objects/pack/pack-*.idx` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad2.git &&
+	(cd repo_bad2.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch $DAEMON_URL/repo_bad2.git &&
+	 test 0 = `ls objects/pack | wc -l`
+	)
+'
+
+test_remote_error()
+{
+	do_export=YesPlease
+	while test $# -gt 0
+	do
+		case $1 in
+		-x)
+			shift
+			chmod -X "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+			;;
+		-n)
+			shift
+			do_export=
+			;;
+		*)
+			break
+		esac
+	done
+
+	if test $# -ne 3
+	then
+		error "invalid number of arguments"
+	fi
+
+	cmd=$1
+	repo=$2
+	msg=$3
+
+	if test -x "$DAEMON_DOCUMENT_ROOT_PATH/$repo"
+	then
+		if test -n "$do_export"
+		then
+			: >"$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+		else
+			rm -f "$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+		fi
+	fi
+
+	test_must_fail git "$cmd" "$DAEMON_URL/$repo" 2>output &&
+	echo "fatal: remote error: $msg: /$repo" >expect &&
+	test_cmp expect output
+	ret=$?
+	chmod +X "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+	(exit $ret)
+}
+
+msg="access denied or repository not exported"
+test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git '$msg'"
+test_expect_success 'push disabled'      "test_remote_error    push  repo.git    '$msg'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    '$msg'"
+test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    '$msg'"
+
+stop_daemon
+start_daemon --informative-errors
+
+test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
+test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
+test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
+
+stop_daemon
+test_done
-- 
1.7.7

[PATCH 2/2] daemon: report permission denied error to clients

[Clemens Buchacher]

If passed an inaccessible url, git daemon returns the
following error:

 $ git clone git://host/repo
 fatal: remote error: no such repository: /repo

In case of a permission denied error, return the following
instead:

 fatal: remote error: permission denied: /repo

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 daemon.c              |   32 +++++++++++++++++++++-----------
 path.c                |   31 +++++++++++++++++++++----------
 t/t5570-git-daemon.sh |    2 +-
 3 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/daemon.c b/daemon.c
index 72fb53a..1442b5b 100644
--- a/daemon.c
+++ b/daemon.c
@@ -109,7 +109,7 @@ static void NORETURN daemon_die(const char *err, va_list params)
 	exit(1);
 }
 
-static char *path_ok(char *directory)
+static int path_ok(char *directory, const char **return_path)
 {
 	static char rpath[PATH_MAX];
 	static char interp_path[PATH_MAX];
@@ -120,13 +120,13 @@ static char *path_ok(char *directory)
 
 	if (daemon_avoid_alias(dir)) {
 		logerror("'%s': aliased", dir);
-		return NULL;
+		return -1;
 	}
 
 	if (*dir == '~') {
 		if (!user_path) {
 			logerror("'%s': User-path not allowed", dir);
-			return NULL;
+			return EACCES;
 		}
 		if (*user_path) {
 			/* Got either "~alice" or "~alice/foo";
@@ -158,7 +158,7 @@ static char *path_ok(char *directory)
 		if (*dir != '/') {
 			/* Allow only absolute */
 			logerror("'%s': Non-absolute path denied (interpolated-path active)", dir);
-			return NULL;
+			return EACCES;
 		}
 
 		strbuf_expand(&expanded_path, interpolated_path,
@@ -173,7 +173,7 @@ static char *path_ok(char *directory)
 		if (*dir != '/') {
 			/* Allow only absolute */
 			logerror("'%s': Non-absolute path denied (base-path active)", dir);
-			return NULL;
+			return EACCES;
 		}
 		snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
 		dir = rpath;
@@ -190,10 +190,14 @@ static char *path_ok(char *directory)
 	}
 
 	if (!path) {
+		int ret = -1;
+		if (errno == EACCES)
+		       ret = EACCES;
 		logerror("'%s' does not appear to be a git repository", dir);
-		return NULL;
+		return ret;
 	}
 
+	*return_path = path;
 	if ( ok_paths && *ok_paths ) {
 		char **pp;
 		int pathlen = strlen(path);
@@ -211,17 +215,17 @@ static char *path_ok(char *directory)
 			    !memcmp(*pp, path, len) &&
 			    (path[len] == '\0' ||
 			     (!strict_paths && path[len] == '/')))
-				return path;
+				return 0;
 		}
 	}
 	else {
 		/* be backwards compatible */
 		if (!strict_paths)
-			return path;
+			return 0;
 	}
 
 	logerror("'%s': not in whitelist", path);
-	return NULL;		/* Fallthrough. Deny by default */
+	return EACCES;		/* Fallthrough. Deny by default */
 }
 
 typedef int (*daemon_service_fn)(void);
@@ -258,6 +262,7 @@ static int daemon_error(const char *dir, const char *msg)
 
 static int run_service(char *dir, struct daemon_service *service)
 {
+	int err;
 	const char *path;
 	int enabled = service->enabled;
 
@@ -269,8 +274,13 @@ static int run_service(char *dir, struct daemon_service *service)
 		return daemon_error(dir, "service not enabled");
 	}
 
-	if (!(path = path_ok(dir)))
-		return daemon_error(dir, "no such repository");
+	err = path_ok(dir, &path);
+	if (err) {
+		if (err == EACCES)
+			return daemon_error(dir, "permission denied");
+		else
+			return daemon_error(dir, "no such repository");
+	}
 
 	/*
 	 * Security on the cheap.
diff --git a/path.c b/path.c
index 6f3f5d5..227d8d7 100644
--- a/path.c
+++ b/path.c
@@ -288,6 +288,7 @@ char *enter_repo(char *path, int strict)
 	static char used_path[PATH_MAX];
 	static char validated_path[PATH_MAX];
 
+	errno = 0;
 	if (!path)
 		return NULL;
 
@@ -301,12 +302,15 @@ char *enter_repo(char *path, int strict)
 			path[len-1] = 0;
 			len--;
 		}
-		if (PATH_MAX <= len)
+		if (PATH_MAX <= len) {
+			errno = ENAMETOOLONG;
 			return NULL;
+		}
 		if (path[0] == '~') {
 			char *newpath = expand_user_path(path);
 			if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
 				free(newpath);
+				errno = 0;
 				return NULL;
 			}
 			/*
@@ -319,9 +323,10 @@ char *enter_repo(char *path, int strict)
 			strcpy(validated_path, path);
 			path = used_path;
 		}
-		else if (PATH_MAX - 10 < len)
+		else if (PATH_MAX - 10 < len) {
+			errno = ENAMETOOLONG;
 			return NULL;
-		else {
+		} else {
 			path = strcpy(used_path, path);
 			strcpy(validated_path, path);
 		}
@@ -331,23 +336,29 @@ char *enter_repo(char *path, int strict)
 			if (!access(path, F_OK)) {
 				strcat(validated_path, suffix[i]);
 				break;
+			} else if (errno == EACCES) {
+				return NULL;
 			}
 		}
-		if (!suffix[i] || chdir(path))
+		if (!suffix[i])
+			return NULL;
+		if (chdir(path))
 			return NULL;
 		path = validated_path;
 	}
 	else if (chdir(path))
 		return NULL;
 
-	if (access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
-	    validate_headref("HEAD") == 0) {
-		set_git_dir(".");
-		check_repository_format();
-		return path;
+	if (access("objects", X_OK) || access("refs", X_OK))
+		return NULL;
+	if (validate_headref("HEAD")) {
+		errno = 0;
+		return NULL;
 	}
 
-	return NULL;
+	set_git_dir(".");
+	check_repository_format();
+	return path;
 }
 
 int set_shared_perm(const char *path, int mode)
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index aa5771a..e6482eb 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -141,7 +141,7 @@ start_daemon --informative-errors
 
 test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
 test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
-test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'permission denied'"
 test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
 
 stop_daemon
-- 
1.7.7

Re: [PATCH 2/2] daemon: report permission denied error to clients

[Jeff King]

On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:

> If passed an inaccessible url, git daemon returns the
> following error:
> 
>  $ git clone git://host/repo
>  fatal: remote error: no such repository: /repo
> 
> In case of a permission denied error, return the following
> instead:
> 
>  fatal: remote error: permission denied: /repo
> 
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---

I like the intent. This actually does leak a little more information
than the existing --informative-errors, as before you couldn't tell the
difference between "not found" and "not exported". But I think the
spirit of --informative-errors is to let that information leak, and this
is a good change.

> -static char *path_ok(char *directory)
> +static int path_ok(char *directory, const char **return_path)
>  {
>  	static char rpath[PATH_MAX];
>  	static char interp_path[PATH_MAX];
> @@ -120,13 +120,13 @@ static char *path_ok(char *directory)
>  
>  	if (daemon_avoid_alias(dir)) {
>  		logerror("'%s': aliased", dir);
> -		return NULL;
> +		return -1;
>  	}
>  
>  	if (*dir == '~') {
>  		if (!user_path) {
>  			logerror("'%s': User-path not allowed", dir);
> -			return NULL;
> +			return EACCES;

The new calling conventions for this function seem a little weird.  I
would expect either "return negative, and set errno" for usual library
code, or possibly "return negative error value". But "return -1, or a
positive error code" seems unusual to me.

One of:

  errno = EACCESS;
  return -1;

or

  return -EACCESS;

would be more idiomatic, I think.

-Peff

Re: [PATCH 2/2] daemon: report permission denied error to clients

[Clemens Buchacher]

On Sun, Oct 16, 2011 at 10:09:12PM -0400, Jeff King wrote:
> On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:
> 
> > If passed an inaccessible url, git daemon returns the
> > following error:
> > 
> >  $ git clone git://host/repo
> >  fatal: remote error: no such repository: /repo
> > 
> > In case of a permission denied error, return the following
> > instead:
> > 
> >  fatal: remote error: permission denied: /repo
> > 
> > Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> > ---
> 
> I like the intent. This actually does leak a little more information
> than the existing --informative-errors, as before you couldn't tell the
> difference between "not found" and "not exported".

I think you mean that before, you couldn't tell the difference
between "not found" and "permission denied".

> > -static char *path_ok(char *directory)
> > +static int path_ok(char *directory, const char **return_path)
> >  {
> >  	static char rpath[PATH_MAX];
> >  	static char interp_path[PATH_MAX];
> > @@ -120,13 +120,13 @@ static char *path_ok(char *directory)
> >  
> >  	if (daemon_avoid_alias(dir)) {
> >  		logerror("'%s': aliased", dir);
> > -		return NULL;
> > +		return -1;
> >  	}
> >  
> >  	if (*dir == '~') {
> >  		if (!user_path) {
> >  			logerror("'%s': User-path not allowed", dir);
> > -			return NULL;
> > +			return EACCES;
> 
> The new calling conventions for this function seem a little weird.  I
> would expect either "return negative, and set errno" for usual library
> code, or possibly "return negative error value". But "return -1, or a
> positive error code" seems unusual to me.

Yes indeed, will fix.

Clemens

Re: [PATCH 2/2] daemon: report permission denied error to clients

[Jeff King]

On Mon, Oct 17, 2011 at 09:48:21PM +0200, Clemens Buchacher wrote:

> > I like the intent. This actually does leak a little more information
> > than the existing --informative-errors, as before you couldn't tell the
> > difference between "not found" and "not exported".
> 
> I think you mean that before, you couldn't tell the difference
> between "not found" and "permission denied".

Ah, right. Sorry, I was thinking path_ok handled the export-ok flag, but
I already handled it in my patch to run_service. So it is leaking a
little more, but even less than I indicated. And at any rate, I think it
is consistent with what --informative-errors is meant to do, so it's a
good change.

> > The new calling conventions for this function seem a little weird.  I
> > would expect either "return negative, and set errno" for usual library
> > code, or possibly "return negative error value". But "return -1, or a
> > positive error code" seems unusual to me.
> 
> Yes indeed, will fix.

Thanks.

-Peff

Re: [PATCH 2/2] daemon: report permission denied error to clients

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:
>
> I like the intent. This actually does leak a little more information
> than the existing --informative-errors, as before you couldn't tell the
> difference between "not found" and "not exported".

I personally think this is going a bit too far, even for "informative"
option, by allowing to fish for possible list of usernames. It would make
it a tougher sell to later default to "informative", I am afraid.

Suppose you are setting up your own repository (either on your own box or
on a box with a separate administrator), and youare wondering why your
attempted access failed. You know /pub/repo/sito.git exists (you created
it after all) and you get "no such repository: /repo/sito.git" when you
ran:

    $ git clone git://host/repo/sito.git/

If you have another repository in /pub/repo/ that does already work, and
if you know /pub/repo/sito.git/ is fine locally (e.g. you can see local
command like "git log" works fine there), then even if you see "not found"
you would know to compare what the difference between these two are.

If there is no other repositories in /pub/repo/ or you are setting up many
repositories on this same box for the first time, wouldn't it be plausible
that you _are_ the administrator of the box and have access to the daemon
log to diagnose the problem more easily anyway?

I can see how this is "leaking a little more information", but I am not
convinced that leak is helping legit users more than helping unwanted
snoopers.

> The new calling conventions for this function seem a little weird.  I
> would expect either "return negative, and set errno" for usual library
> code, or possibly "return negative error value". But "return -1, or a
> positive error code" seems unusual to me.
>
> One of:
>
>   errno = EACCESS;
>   return -1;
>
> or
>
>   return -EACCESS;
>
> would be more idiomatic, I think.

Yes, the former would probably be easier to handle.

Re: [PATCH 2/2] daemon: report permission denied error to clients

[Clemens Buchacher]

On Mon, Oct 17, 2011 at 02:03:03PM -0700, Junio C Hamano wrote:
> 
> I personally think this is going a bit too far, even for "informative"
> option, by allowing to fish for possible list of usernames. It would make
> it a tougher sell to later default to "informative", I am afraid.

I guess if permission is denied for access over git://, then nobody
can use the repository. So it's clearly a server side issue.

This change probably makes more sense for local access and over
ssh. I already have a similar patch brewing for that.

Clemens

Re: [PATCH 2/2] daemon: report permission denied error to clients

[Clemens Buchacher]

Clemens Buchacher <drizzd@aon.at> wrote:
>
> I guess if permission is denied for access over git://, then nobody
> can use the repository. So it's clearly a server side issue.
> 
> This change probably makes more sense for local access and over
> ssh. I already have a similar patch brewing for that.

As far as security is concerned, we have to treat ssh the same as git://, unless the user has permission to execute arbitrary commands and not just git-upload-pack. But I can think of no way to figure that out on the server side.

[PATCH v2 2/2] daemon: report permission denied error to clients

[Clemens Buchacher]

If passed an inaccessible url, git daemon returns the
following error:

 $ git clone git://host/repo
 fatal: remote error: no such repository: /repo

In case of a permission denied error, return the following
instead:

 fatal: remote error: permission denied: /repo

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

Compared to v1 of this patch, the calling convention of path_ok are
back to what they were previously. Now the only change is that it
sets errno.

 daemon.c              |   15 +++++++++++++--
 path.c                |   31 +++++++++++++++++++++----------
 t/t5570-git-daemon.sh |    2 +-
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/daemon.c b/daemon.c
index 72fb53a..2f7f84e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -120,12 +120,14 @@ static char *path_ok(char *directory)
 
 	if (daemon_avoid_alias(dir)) {
 		logerror("'%s': aliased", dir);
+		errno = 0;
 		return NULL;
 	}
 
 	if (*dir == '~') {
 		if (!user_path) {
 			logerror("'%s': User-path not allowed", dir);
+			errno = EACCES;
 			return NULL;
 		}
 		if (*user_path) {
@@ -158,6 +160,7 @@ static char *path_ok(char *directory)
 		if (*dir != '/') {
 			/* Allow only absolute */
 			logerror("'%s': Non-absolute path denied (interpolated-path active)", dir);
+			errno = EACCES;
 			return NULL;
 		}
 
@@ -173,6 +176,7 @@ static char *path_ok(char *directory)
 		if (*dir != '/') {
 			/* Allow only absolute */
 			logerror("'%s': Non-absolute path denied (base-path active)", dir);
+			errno = EACCES;
 			return NULL;
 		}
 		snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
@@ -190,7 +194,9 @@ static char *path_ok(char *directory)
 	}
 
 	if (!path) {
+		int err = errno;
 		logerror("'%s' does not appear to be a git repository", dir);
+		errno = err;
 		return NULL;
 	}
 
@@ -221,6 +227,7 @@ static char *path_ok(char *directory)
 	}
 
 	logerror("'%s': not in whitelist", path);
+	errno = EACCES;
 	return NULL;		/* Fallthrough. Deny by default */
 }
 
@@ -269,8 +276,12 @@ static int run_service(char *dir, struct daemon_service *service)
 		return daemon_error(dir, "service not enabled");
 	}
 
-	if (!(path = path_ok(dir)))
-		return daemon_error(dir, "no such repository");
+	if (!(path = path_ok(dir))) {
+		if (errno == EACCES)
+			return daemon_error(dir, "permission denied");
+		else
+			return daemon_error(dir, "no such repository");
+	}
 
 	/*
 	 * Security on the cheap.
diff --git a/path.c b/path.c
index 6f3f5d5..227d8d7 100644
--- a/path.c
+++ b/path.c
@@ -288,6 +288,7 @@ char *enter_repo(char *path, int strict)
 	static char used_path[PATH_MAX];
 	static char validated_path[PATH_MAX];
 
+	errno = 0;
 	if (!path)
 		return NULL;
 
@@ -301,12 +302,15 @@ char *enter_repo(char *path, int strict)
 			path[len-1] = 0;
 			len--;
 		}
-		if (PATH_MAX <= len)
+		if (PATH_MAX <= len) {
+			errno = ENAMETOOLONG;
 			return NULL;
+		}
 		if (path[0] == '~') {
 			char *newpath = expand_user_path(path);
 			if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
 				free(newpath);
+				errno = 0;
 				return NULL;
 			}
 			/*
@@ -319,9 +323,10 @@ char *enter_repo(char *path, int strict)
 			strcpy(validated_path, path);
 			path = used_path;
 		}
-		else if (PATH_MAX - 10 < len)
+		else if (PATH_MAX - 10 < len) {
+			errno = ENAMETOOLONG;
 			return NULL;
-		else {
+		} else {
 			path = strcpy(used_path, path);
 			strcpy(validated_path, path);
 		}
@@ -331,23 +336,29 @@ char *enter_repo(char *path, int strict)
 			if (!access(path, F_OK)) {
 				strcat(validated_path, suffix[i]);
 				break;
+			} else if (errno == EACCES) {
+				return NULL;
 			}
 		}
-		if (!suffix[i] || chdir(path))
+		if (!suffix[i])
+			return NULL;
+		if (chdir(path))
 			return NULL;
 		path = validated_path;
 	}
 	else if (chdir(path))
 		return NULL;
 
-	if (access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
-	    validate_headref("HEAD") == 0) {
-		set_git_dir(".");
-		check_repository_format();
-		return path;
+	if (access("objects", X_OK) || access("refs", X_OK))
+		return NULL;
+	if (validate_headref("HEAD")) {
+		errno = 0;
+		return NULL;
 	}
 
-	return NULL;
+	set_git_dir(".");
+	check_repository_format();
+	return path;
 }
 
 int set_shared_perm(const char *path, int mode)
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index aa5771a..e6482eb 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -141,7 +141,7 @@ start_daemon --informative-errors
 
 test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
 test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
-test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'permission denied'"
 test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
 
 stop_daemon
-- 
1.7.7

Re: [PATCH v2 2/2] daemon: report permission denied error to clients

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> diff --git a/daemon.c b/daemon.c
> index 72fb53a..2f7f84e 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -120,12 +120,14 @@ static char *path_ok(char *directory)
>  
>  	if (daemon_avoid_alias(dir)) {
>  		logerror("'%s': aliased", dir);
> +		errno = 0;
>  		return NULL;
>  	}
>  
>  	if (*dir == '~') {
>  		if (!user_path) {
>  			logerror("'%s': User-path not allowed", dir);
> +			errno = EACCES;
>  			return NULL;
>  		}

Isn't the first one inconsistent from all the others?

A request cames "../some/path" and it is not allowed by a daemon policy
and it gets errno==0 which is turned into "no such repo" later, while
another request to "~drizzed/another/path" is also rejected by a daemon
policy and gets errno==EACCESS which is turned into "permission denied".

Indeed everything else says EACCESS in this patch, except for the check
done by enter_repo() which can additionally say ENAMETOOLONG (which would
not be very useful in practice) or whatever error coming from failure to
go there with chdir(), which is not likely to be EACCESS because it has
already been checked with a separate access() that is done before the
actual chdir() call.

> +	if (!(path = path_ok(dir))) {
> +		if (errno == EACCES)
> +			return daemon_error(dir, "permission denied");
> +		else
> +			return daemon_error(dir, "no such repository");
> +	}

If errno is set to EACCESS in cases (1) we are not even going to tell you
if a repository exists there or not--you are not authorized to know and
(2) there is a repository but you do not have authorization to access it,
then this "leaking a bit more information" part is acceptable for site
with "--informative-errors", I would think. A repository that is invalid
from the daemon's point of view (e.g. validate_headref("HEAD") fails
because it points at an object that does not exist) but that the owner
intended to make it valid by correcting such mistakes would be reported as
"no such repository" with such a logic, so I am not sure if the distinction
between these two cases really matters in practice, though.

Re: [PATCH 1/2] daemon: add tests

[Jeff King]

On Mon, Oct 17, 2011 at 12:11:15AM +0200, Clemens Buchacher wrote:

> The semantics of the git daemon tests are similar to the http
> transport tests.  In fact, they are only a slightly modified copy
> of t5550, plus the newly added remote error tests.
> 
> All daemon tests will be skipped unless the environment variable
> GIT_TEST_DAEMON is set.

Thanks, it's nice to have some tests. Overall, some of the tests feel a
little silly, because the results should be exactly the same as fetching
or pushing a local repository (so the "set-head" thing, for example,
really has little to do with git-daemon). At the same time, maybe it's a
good thing to re-confirm that the results really are the same. :)

> diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh
> new file mode 100644
> index 0000000..30a89ea
> --- /dev/null
> +++ b/t/lib-daemon.sh
> @@ -0,0 +1,52 @@
> +#!/bin/sh
> +
> +if test -z "$GIT_TEST_DAEMON"
> +then
> +	skip_all="Daemon testing disabled (define GIT_TEST_DAEMON to enable)"
> +	test_done
> +fi
> +
> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}

I assume you picked this arbitrarily to be LIB_HTTPD_PORT+10. It's fine
to have a default, but note that each of the httpd tests actually
defaults the port to their test number, so they can be run in parallel.

So that would be:

> --- /dev/null
> +++ b/t/t5570-git-daemon.sh
> @@ -0,0 +1,148 @@
> +#!/bin/sh
> +
> +test_description='test fetching over git protocol'
> +. ./test-lib.sh
> +
> +. "$TEST_DIRECTORY"/lib-daemon.sh
> +start_daemon

LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}

here.

[PATCH] use test number as port number

[Clemens Buchacher]

Test 5550 was apparently using the default port number by mistake.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
> 
> LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}

Thanks, I missed that.

Clemens

 t/t5550-http-fetch.sh |    2 +-
 t/t5570-git-daemon.sh |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index a1883ca..8a77750 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -8,8 +8,8 @@ if test -n "$NO_CURL"; then
 	test_done
 fi
 
-. "$TEST_DIRECTORY"/lib-httpd.sh
 LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}
+. "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
 test_expect_success 'setup repository' '
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index e6482eb..a92d996 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -3,6 +3,7 @@
 test_description='test fetching over git protocol'
 . ./test-lib.sh
 
+LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
 . "$TEST_DIRECTORY"/lib-daemon.sh
 start_daemon
 
-- 
1.7.7

Re: [PATCH] use test number as port number

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> Test 5550 was apparently using the default port number by mistake.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
>
> On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
>> 
>> LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
>
> Thanks, I missed that.
>
> Clemens
>
>  t/t5550-http-fetch.sh |    2 +-
>  t/t5570-git-daemon.sh |    1 +
>  2 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
> index a1883ca..8a77750 100755
> --- a/t/t5550-http-fetch.sh
> +++ b/t/t5550-http-fetch.sh
> @@ -8,8 +8,8 @@ if test -n "$NO_CURL"; then
>  	test_done
>  fi
>  
> -. "$TEST_DIRECTORY"/lib-httpd.sh
>  LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}
> +. "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd

Good eyes. This is the only one in the 55xx series that gets the order
wrong.

I'll drop the patch to 5570 for now as that should be done in the change
that is still not in 'next' that adds 5570.

I've fixed and queued the previous one as aa0b028 (daemon: add tests,
2011-10-17); does that look good enough?

> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index e6482eb..a92d996 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -3,6 +3,7 @@
>  test_description='test fetching over git protocol'
>  . ./test-lib.sh
>  
> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
>  . "$TEST_DIRECTORY"/lib-daemon.sh
>  start_daemon

Re: [PATCH] use test number as port number

[Clemens Buchacher]

On Mon, Oct 17, 2011 at 01:57:00PM -0700, Junio C Hamano wrote:
> 
> I've fixed and queued the previous one as aa0b028 (daemon: add tests,
> 2011-10-17); does that look good enough?

Yep, perfect. Thanks.

Re: [PATCH 1/2] daemon: add tests

[Clemens Buchacher]

On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
> 
> Thanks, it's nice to have some tests. Overall, some of the tests feel a
> little silly, because the results should be exactly the same as fetching
> or pushing a local repository (so the "set-head" thing, for example,
> really has little to do with git-daemon).

Hmm, yes. Actually, I thought I had found a bug with the failure of
"set-head -a". But now I see that in t5505 this treated like a
feature.

Would it be difficult to support this over the git protocol? Maybe
I will have a look.

Clemens

Re: [PATCH 1/2] daemon: add tests

[Jeff King]

On Mon, Oct 17, 2011 at 10:05:28PM +0200, Clemens Buchacher wrote:

> On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
> > 
> > Thanks, it's nice to have some tests. Overall, some of the tests feel a
> > little silly, because the results should be exactly the same as fetching
> > or pushing a local repository (so the "set-head" thing, for example,
> > really has little to do with git-daemon).
> 
> Hmm, yes. Actually, I thought I had found a bug with the failure of
> "set-head -a". But now I see that in t5505 this treated like a
> feature.

It's not a feature, exactly. It's just documenting that we fail in the
face of ambiguous HEADs. Arguably, the test should be switched to use
text_expect_failure to document that we would prefer it the other way,
but it doesn't work now.

> Would it be difficult to support this over the git protocol? Maybe
> I will have a look.

It needs a protocol extension to communicate symbolic ref destinations.
The topic has come up a few times, and I think Junio even had patches at
one point.

-Peff

Re: [PATCH 1/2] daemon: add tests

[Jonathan Nieder]

(+cc: Erik, Ilari, Duy)
Hi,

Clemens Buchacher wrote:

> [Subject: daemon: add tests]

Can't believe I missed this.  That seems like a worthy cause ---
can someone remind me why this is dropped, or if there are any
tweaks I can help with to get it picked up again?

Patch left unsnipped for convenience of people cc-ed.

Jonathan

> The semantics of the git daemon tests are similar to the http
> transport tests.  In fact, they are only a slightly modified copy
> of t5550, plus the newly added remote error tests.
>
> All daemon tests will be skipped unless the environment variable
> GIT_TEST_DAEMON is set.
> 
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
> 
> This patch is based on jk/daemon-msgs.
> 
>  t/lib-daemon.sh       |   52 +++++++++++++++++
>  t/t5570-git-daemon.sh |  148 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 200 insertions(+), 0 deletions(-)
>  create mode 100644 t/lib-daemon.sh
>  create mode 100755 t/t5570-git-daemon.sh
> 
> diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh
> new file mode 100644
> index 0000000..30a89ea
> --- /dev/null
> +++ b/t/lib-daemon.sh
> @@ -0,0 +1,52 @@
> +#!/bin/sh
> +
> +if test -z "$GIT_TEST_DAEMON"
> +then
> +	skip_all="Daemon testing disabled (define GIT_TEST_DAEMON to enable)"
> +	test_done
> +fi
> +
> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}
> +
> +DAEMON_PID=
> +DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
> +DAEMON_URL=git://127.0.0.1:$LIB_DAEMON_PORT
> +
> +start_daemon() {
> +	if test -n "$DAEMON_PID"
> +	then
> +		error "start_daemon already called"
> +	fi
> +
> +	mkdir -p "$DAEMON_DOCUMENT_ROOT_PATH"
> +
> +	trap 'code=$?; stop_daemon; (exit $code); die' EXIT
> +
> +	say >&3 "Starting git daemon ..."
> +	git daemon --listen=127.0.0.1 --port="$LIB_DAEMON_PORT" \
> +		--reuseaddr --verbose \
> +		--base-path="$DAEMON_DOCUMENT_ROOT_PATH" \
> +		"$@" "$DAEMON_DOCUMENT_ROOT_PATH" \
> +		>&3 2>&4 &
> +	DAEMON_PID=$!
> +}
> +
> +stop_daemon() {
> +	if test -z "$DAEMON_PID"
> +	then
> +		return
> +	fi
> +
> +	trap 'die' EXIT
> +
> +	# kill git-daemon child of git
> +	say >&3 "Stopping git daemon ..."
> +	pkill -P "$DAEMON_PID"
> +	wait "$DAEMON_PID"
> +	ret=$?
> +	if test $ret -ne 143
> +	then
> +		error "git daemon exited with status: $ret"
> +	fi
> +	DAEMON_PID=
> +}
> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> new file mode 100755
> index 0000000..aa5771a
> --- /dev/null
> +++ b/t/t5570-git-daemon.sh
> @@ -0,0 +1,148 @@
> +#!/bin/sh
> +
> +test_description='test fetching over git protocol'
> +. ./test-lib.sh
> +
> +. "$TEST_DIRECTORY"/lib-daemon.sh
> +start_daemon
> +
> +test_expect_success 'setup repository' '
> +	echo content >file &&
> +	git add file &&
> +	git commit -m one
> +'
> +
> +test_expect_success 'create git-accessible bare repository' '
> +	mkdir "$DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
> +	(cd "$DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
> +	 git --bare init &&
> +	 : >git-daemon-export-ok
> +	) &&
> +	git remote add public "$DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
> +	git push public master:master
> +'
> +
> +test_expect_success 'clone git repository' '
> +	git clone $DAEMON_URL/repo.git clone &&
> +	test_cmp file clone/file
> +'
> +
> +test_expect_success 'fetch changes via git protocol' '
> +	echo content >>file &&
> +	git commit -a -m two &&
> +	git push public &&
> +	(cd clone && git pull) &&
> +	test_cmp file clone/file
> +'
> +
> +test_expect_failure 'remote detects correct HEAD' '
> +	git push public master:other &&
> +	(cd clone &&
> +	 git remote set-head -d origin &&
> +	 git remote set-head -a origin &&
> +	 git symbolic-ref refs/remotes/origin/HEAD > output &&
> +	 echo refs/remotes/origin/master > expect &&
> +	 test_cmp expect output
> +	)
> +'
> +
> +test_expect_success 'prepare pack objects' '
> +	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
> +	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
> +	 git --bare repack &&
> +	 git --bare prune-packed
> +	)
> +'
> +
> +test_expect_success 'fetch notices corrupt pack' '
> +	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
> +	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
> +	 p=`ls objects/pack/pack-*.pack` &&
> +	 chmod u+w $p &&
> +	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
> +	) &&
> +	mkdir repo_bad1.git &&
> +	(cd repo_bad1.git &&
> +	 git --bare init &&
> +	 test_must_fail git --bare fetch $DAEMON_URL/repo_bad1.git &&
> +	 test 0 = `ls objects/pack/pack-*.pack | wc -l`
> +	)
> +'
> +
> +test_expect_success 'fetch notices corrupt idx' '
> +	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
> +	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
> +	 p=`ls objects/pack/pack-*.idx` &&
> +	 chmod u+w $p &&
> +	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
> +	) &&
> +	mkdir repo_bad2.git &&
> +	(cd repo_bad2.git &&
> +	 git --bare init &&
> +	 test_must_fail git --bare fetch $DAEMON_URL/repo_bad2.git &&
> +	 test 0 = `ls objects/pack | wc -l`
> +	)
> +'
> +
> +test_remote_error()
> +{
> +	do_export=YesPlease
> +	while test $# -gt 0
> +	do
> +		case $1 in
> +		-x)
> +			shift
> +			chmod -X "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"
> +			;;
> +		-n)
> +			shift
> +			do_export=
> +			;;
> +		*)
> +			break
> +		esac
> +	done
> +
> +	if test $# -ne 3
> +	then
> +		error "invalid number of arguments"
> +	fi
> +
> +	cmd=$1
> +	repo=$2
> +	msg=$3
> +
> +	if test -x "$DAEMON_DOCUMENT_ROOT_PATH/$repo"
> +	then
> +		if test -n "$do_export"
> +		then
> +			: >"$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
> +		else
> +			rm -f "$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
> +		fi
> +	fi
> +
> +	test_must_fail git "$cmd" "$DAEMON_URL/$repo" 2>output &&
> +	echo "fatal: remote error: $msg: /$repo" >expect &&
> +	test_cmp expect output
> +	ret=$?
> +	chmod +X "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"
> +	(exit $ret)
> +}
> +
> +msg="access denied or repository not exported"
> +test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git '$msg'"
> +test_expect_success 'push disabled'      "test_remote_error    push  repo.git    '$msg'"
> +test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    '$msg'"
> +test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    '$msg'"
> +
> +stop_daemon
> +start_daemon --informative-errors
> +
> +test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
> +test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
> +test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
> +test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
> +
> +stop_daemon
> +test_done
> -- 
> 1.7.7
> 
> 

Re: [PATCH 1/2] daemon: add tests

[Clemens Buchacher]

On Mon, Jan 02, 2012 at 03:25:08AM -0600, Jonathan Nieder wrote:
> 
> > [Subject: daemon: add tests]
> 
> Can't believe I missed this.  That seems like a worthy cause ---
> can someone remind me why this is dropped, or if there are any
> tweaks I can help with to get it picked up again?

We were discussing some open issues with patch 2/2, which was based
on the tests. I later abandoned the idea for that patch. But the
tests should be ok by themselves.

Clemens

Re: [PATCH 1/2] daemon: add tests

[Jeff King]

On Mon, Jan 02, 2012 at 08:47:11PM +0100, Clemens Buchacher wrote:

> On Mon, Jan 02, 2012 at 03:25:08AM -0600, Jonathan Nieder wrote:
> > 
> > > [Subject: daemon: add tests]
> > 
> > Can't believe I missed this.  That seems like a worthy cause ---
> > can someone remind me why this is dropped, or if there are any
> > tweaks I can help with to get it picked up again?
> 
> We were discussing some open issues with patch 2/2, which was based
> on the tests. I later abandoned the idea for that patch. But the
> tests should be ok by themselves.

Yes, I'd like to see them included, even without the second patch. We
currently have zero tests for git-daemon, so even just verifying that
it starts and can let people fetch is an improvement.

-Peff

Re: [PATCH 1/2] daemon: add tests

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> (+cc: Erik, Ilari, Duy)
> Hi,
>
> Clemens Buchacher wrote:
>
>> [Subject: daemon: add tests]
>
> Can't believe I missed this.  That seems like a worthy cause ---
> can someone remind me why this is dropped, or if there are any
> tweaks I can help with to get it picked up again?

Thanks for your interest in this.

>> diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh
>> new file mode 100644
>> index 0000000..30a89ea
>> --- /dev/null
>> +++ b/t/lib-daemon.sh
>> @@ -0,0 +1,52 @@
>> +#!/bin/sh
>> +
>> +if test -z "$GIT_TEST_DAEMON"
>> +then
>> +	skip_all="Daemon testing disabled (define GIT_TEST_DAEMON to enable)"
>> +	test_done
>> +fi
>> +
>> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}

In lib-httpd.sh, LIB_HTTPD_PORT is defined in a similar way, but that is
always overridden by the users and the convention there is to use the test
numbers (cf. "git grep LIB_HTTPD_PORT t/"), which should be followed here
as well.

I am not very keen on the "lib-daemon.sh", GIT_TEST_DAEMON, etc. naming to
pretend as if "git daemon" will forever be the only daemon we will ever
ship, by the way.  We might one day want to add an inotify daemon, a
daemon for the git-pubsub protocol or somesuch.

>> +DAEMON_PID=
>> +DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
>> +DAEMON_URL=git://127.0.0.1:$LIB_DAEMON_PORT
>> +
>> +start_daemon() {
>> +	if test -n "$DAEMON_PID"
>> +	then
>> +		error "start_daemon already called"
>> +	fi
>> +
>> +	mkdir -p "$DAEMON_DOCUMENT_ROOT_PATH"
>> +
>> +	trap 'code=$?; stop_daemon; (exit $code); die' EXIT
>> +
>> +	say >&3 "Starting git daemon ..."
>> +	git daemon --listen=127.0.0.1 --port="$LIB_DAEMON_PORT" \
>> +		--reuseaddr --verbose \
>> +		--base-path="$DAEMON_DOCUMENT_ROOT_PATH" \
>> +		"$@" "$DAEMON_DOCUMENT_ROOT_PATH" \
>> +		>&3 2>&4 &
>> +	DAEMON_PID=$!
>> +}
>> +
>> +stop_daemon() {
>> +	if test -z "$DAEMON_PID"
>> +	then
>> +		return
>> +	fi
>> +
>> +	trap 'die' EXIT
>> +
>> +	# kill git-daemon child of git
>> +	say >&3 "Stopping git daemon ..."
>> +	pkill -P "$DAEMON_PID"

How portable is this one (I usually do not trust use of pkill anywhere)?

>> +	wait "$DAEMON_PID"
>> +	ret=$?
	# Please comment what 143 is on this line.
>> +	if test $ret -ne 143
>> +	then
>> +		error "git daemon exited with status: $ret"
>> +	fi
>> +	DAEMON_PID=
>> +}
>> ...
>> +test_expect_success 'prepare pack objects' '
>> +	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
>> +	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
>> +	 git --bare repack &&

As the later tests assume there will be only one pack, don't you want at
least "-a" and possibly "-a -d" here?

>> +	 git --bare prune-packed
>> +	)
>> +'
>> +
>> +test_expect_success 'fetch notices corrupt pack' '
>> +	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
>> +	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
>> +	 p=`ls objects/pack/pack-*.pack` &&
>> +	 chmod u+w $p &&
>> +	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
>> +	) &&
>> +	mkdir repo_bad1.git &&
>> +	(cd repo_bad1.git &&
>> +	 git --bare init &&
>> +	 test_must_fail git --bare fetch $DAEMON_URL/repo_bad1.git &&
>> +	 test 0 = `ls objects/pack/pack-*.pack | wc -l`
>> +	)
>> +'
>> +
>> +test_expect_success 'fetch notices corrupt idx' '
>> +	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
>> +	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
>> +	 p=`ls objects/pack/pack-*.idx` &&
>> +	 chmod u+w $p &&
>> +	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
>> +	) &&
>> +	mkdir repo_bad2.git &&
>> +	(cd repo_bad2.git &&
>> +	 git --bare init &&
>> +	 test_must_fail git --bare fetch $DAEMON_URL/repo_bad2.git &&
>> +	 test 0 = `ls objects/pack | wc -l`
>> +	)
>> +'
>> +
>> +test_remote_error()
>> +{
>> +	do_export=YesPlease
>> +	while test $# -gt 0
>> +	do
>> +		case $1 in
>> +		-x)
>> +			shift
>> +			chmod -X "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"

I find the use of cap X here dubious; it makes your intention unclear.

Are you interested in the current status of 'x' bits on that directory, or
are you more interested in dropping the executable/searchable bits from
the directory no matter what its current status is (rhetorical: I fully
expect that the answer is the latter)? The same comment applies to the use
of "chmod +X" at the end of this helper function.

Re: [PATCH 1/2] daemon: add tests

[Clemens Buchacher]

Thanks for your review. Please find fixes in reply to this email. In
order to better show individual changes I have not squashed them into
one commit. For upstream, you will probably want to squash patches 3-6
into patch 2. Patch 2 is the same as the one once queued as part of
cb/daemon-permission-errors.

[PATCH 1/6] t5550: repack everything into one file
[PATCH 2/6] daemon: add tests
[PATCH 3/6] avoid use of pkill
[PATCH 4/6] explain expected exit code
[PATCH 5/6] t5570: everything into one file
[PATCH 6/6] chmod: use lower-case x

On Tue, Jan 03, 2012 at 11:34:08AM -0800, Junio C Hamano wrote:
> >> +
> >> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}
> 
> In lib-httpd.sh, LIB_HTTPD_PORT is defined in a similar way, but that is
> always overridden by the users and the convention there is to use the test
> numbers (cf. "git grep LIB_HTTPD_PORT t/"), which should be followed here
> as well.

This you already fixed in the version previously queued and is contained
in [PATCH 2/6] daemon: add tests.

> I am not very keen on the "lib-daemon.sh", GIT_TEST_DAEMON, etc. naming to
> pretend as if "git daemon" will forever be the only daemon we will ever
> ship, by the way.  We might one day want to add an inotify daemon, a
> daemon for the git-pubsub protocol or somesuch.

Are you saying that the name "daemon" is too general, and it should
instead be "lib-git-daemon.sh" and GIT_TEST_GIT_DAEMON? Or do you
mean that it is not general enough and it should be called
lib-networking.sh and "GIT_TEST_NETWORKING"?

Either way, I have no preference here. Feel free to change any way you
like.

> >> +	# kill git-daemon child of git
> >> +	say >&3 "Stopping git daemon ..."
> >> +	pkill -P "$DAEMON_PID"
> 
> How portable is this one (I usually do not trust use of pkill anywhere)?

I read that it is supposed to be more portable than skill or killall.
But I have no way to research this. I have implemented a workaround
using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.

> >> +  wait "$DAEMON_PID"
> >> +  ret=$?
>       # Please comment what 143 is on this line.
> >> +  if test $ret -ne 143

Fixed in [PATCH 4/6] explain expected exit code.

> >> +	 git --bare repack &&
> 
> As the later tests assume there will be only one pack, don't you want at
> least "-a" and possibly "-a -d" here?

Fixed in

 [PATCH 1/6] t5550: repack everything into one file,
 [PATCH 5/6] t5570: repack everything into one file.

> I find the use of cap X here dubious; it makes your intention unclear.
> 
> Are you interested in the current status of 'x' bits on that directory, or
> are you more interested in dropping the executable/searchable bits from
> the directory no matter what its current status is (rhetorical: I fully
> expect that the answer is the latter)?

For directories, upper-case X does not have that meaning. The status is
always overwritten, irrespective of the current status. I wanted to
emphasize the fact that I am changing 'searchable' bits.  But since that
does not seem to have the desired effect, I changed it to lower-case in
[PATCH 6/6] chmod: use lower-case x.

Clemens

[PATCH 1/6] t5550: repack everything into one file

[Clemens Buchacher]

Subsequently we assume that there is only one pack. Currently this is
true only by accident. Pass '-a -d' to repack in order to guarantee that
assumption to hold true.

The prune-packed command is now redundant since repack -d already calls
it.
---
 t/t5550-http-fetch.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 311a33c..7926ab3 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -118,8 +118,7 @@ test_expect_success 'http remote detects correct HEAD' '
 test_expect_success 'fetch packed objects' '
 	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
 	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
-	 git --bare repack &&
-	 git --bare prune-packed
+	 git --bare repack -a -d
 	) &&
 	git clone $HTTPD_URL/dumb/repo_pack.git
 '
-- 
1.7.8

Re: [PATCH 1/6] t5550: repack everything into one file

[Junio C Hamano]

Thanks; I assume this is also signed off?

[PATCH 2/6] daemon: add tests

[Clemens Buchacher]

The semantics of the git daemon tests are similar to the http
transport tests.  In fact, they are only a slightly modified copy
of t5550, plus the newly added remote error tests.

All daemon tests will be skipped unless the environment variable
GIT_TEST_DAEMON is set.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-daemon.sh       |   52 +++++++++++++++++
 t/t5570-git-daemon.sh |  149 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 201 insertions(+), 0 deletions(-)
 create mode 100644 t/lib-daemon.sh
 create mode 100755 t/t5570-git-daemon.sh

diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh
new file mode 100644
index 0000000..30a89ea
--- /dev/null
+++ b/t/lib-daemon.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+if test -z "$GIT_TEST_DAEMON"
+then
+	skip_all="Daemon testing disabled (define GIT_TEST_DAEMON to enable)"
+	test_done
+fi
+
+LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}
+
+DAEMON_PID=
+DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
+DAEMON_URL=git://127.0.0.1:$LIB_DAEMON_PORT
+
+start_daemon() {
+	if test -n "$DAEMON_PID"
+	then
+		error "start_daemon already called"
+	fi
+
+	mkdir -p "$DAEMON_DOCUMENT_ROOT_PATH"
+
+	trap 'code=$?; stop_daemon; (exit $code); die' EXIT
+
+	say >&3 "Starting git daemon ..."
+	git daemon --listen=127.0.0.1 --port="$LIB_DAEMON_PORT" \
+		--reuseaddr --verbose \
+		--base-path="$DAEMON_DOCUMENT_ROOT_PATH" \
+		"$@" "$DAEMON_DOCUMENT_ROOT_PATH" \
+		>&3 2>&4 &
+	DAEMON_PID=$!
+}
+
+stop_daemon() {
+	if test -z "$DAEMON_PID"
+	then
+		return
+	fi
+
+	trap 'die' EXIT
+
+	# kill git-daemon child of git
+	say >&3 "Stopping git daemon ..."
+	pkill -P "$DAEMON_PID"
+	wait "$DAEMON_PID"
+	ret=$?
+	if test $ret -ne 143
+	then
+		error "git daemon exited with status: $ret"
+	fi
+	DAEMON_PID=
+}
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
new file mode 100755
index 0000000..a7d666c
--- /dev/null
+++ b/t/t5570-git-daemon.sh
@@ -0,0 +1,149 @@
+#!/bin/sh
+
+test_description='test fetching over git protocol'
+. ./test-lib.sh
+
+LIB_DAEMON_PORT=${LIB_DAEMON_PORT-5570}
+. "$TEST_DIRECTORY"/lib-daemon.sh
+start_daemon
+
+test_expect_success 'setup repository' '
+	echo content >file &&
+	git add file &&
+	git commit -m one
+'
+
+test_expect_success 'create git-accessible bare repository' '
+	mkdir "$DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	(cd "$DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	 git --bare init &&
+	 : >git-daemon-export-ok
+	) &&
+	git remote add public "$DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push public master:master
+'
+
+test_expect_success 'clone git repository' '
+	git clone $DAEMON_URL/repo.git clone &&
+	test_cmp file clone/file
+'
+
+test_expect_success 'fetch changes via git protocol' '
+	echo content >>file &&
+	git commit -a -m two &&
+	git push public &&
+	(cd clone && git pull) &&
+	test_cmp file clone/file
+'
+
+test_expect_failure 'remote detects correct HEAD' '
+	git push public master:other &&
+	(cd clone &&
+	 git remote set-head -d origin &&
+	 git remote set-head -a origin &&
+	 git symbolic-ref refs/remotes/origin/HEAD > output &&
+	 echo refs/remotes/origin/master > expect &&
+	 test_cmp expect output
+	)
+'
+
+test_expect_success 'prepare pack objects' '
+	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	 git --bare repack &&
+	 git --bare prune-packed
+	)
+'
+
+test_expect_success 'fetch notices corrupt pack' '
+	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+	 p=`ls objects/pack/pack-*.pack` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad1.git &&
+	(cd repo_bad1.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch $DAEMON_URL/repo_bad1.git &&
+	 test 0 = `ls objects/pack/pack-*.pack | wc -l`
+	)
+'
+
+test_expect_success 'fetch notices corrupt idx' '
+	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	 p=`ls objects/pack/pack-*.idx` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad2.git &&
+	(cd repo_bad2.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch $DAEMON_URL/repo_bad2.git &&
+	 test 0 = `ls objects/pack | wc -l`
+	)
+'
+
+test_remote_error()
+{
+	do_export=YesPlease
+	while test $# -gt 0
+	do
+		case $1 in
+		-x)
+			shift
+			chmod -X "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+			;;
+		-n)
+			shift
+			do_export=
+			;;
+		*)
+			break
+		esac
+	done
+
+	if test $# -ne 3
+	then
+		error "invalid number of arguments"
+	fi
+
+	cmd=$1
+	repo=$2
+	msg=$3
+
+	if test -x "$DAEMON_DOCUMENT_ROOT_PATH/$repo"
+	then
+		if test -n "$do_export"
+		then
+			: >"$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+		else
+			rm -f "$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+		fi
+	fi
+
+	test_must_fail git "$cmd" "$DAEMON_URL/$repo" 2>output &&
+	echo "fatal: remote error: $msg: /$repo" >expect &&
+	test_cmp expect output
+	ret=$?
+	chmod +X "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+	(exit $ret)
+}
+
+msg="access denied or repository not exported"
+test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git '$msg'"
+test_expect_success 'push disabled'      "test_remote_error    push  repo.git    '$msg'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    '$msg'"
+test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    '$msg'"
+
+stop_daemon
+start_daemon --informative-errors
+
+test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
+test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
+test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
+
+stop_daemon
+test_done
-- 
1.7.8

[PATCH 3/6] avoid use of pkill

[Clemens Buchacher]

---
 t/lib-daemon.sh |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh
index 30a89ea..5edced5 100644
--- a/t/lib-daemon.sh
+++ b/t/lib-daemon.sh
@@ -31,6 +31,24 @@ start_daemon() {
 	DAEMON_PID=$!
 }
 
+kill_children() {
+	parent=$1
+
+	ps -A -o ppid,pid |
+	(
+		# skip header
+		read
+		while read ppid pid
+		do
+			if test x"$ppid" = x"$parent"
+			then
+				echo "$pid"
+			fi
+		done
+	) |
+	xargs kill
+}
+
 stop_daemon() {
 	if test -z "$DAEMON_PID"
 	then
@@ -41,7 +59,7 @@ stop_daemon() {
 
 	# kill git-daemon child of git
 	say >&3 "Stopping git daemon ..."
-	pkill -P "$DAEMON_PID"
+	kill_children "$DAEMON_PID"
 	wait "$DAEMON_PID"
 	ret=$?
 	if test $ret -ne 143
-- 
1.7.8

[PATCH 4/6] explain expected exit code

[Clemens Buchacher]

---
 t/lib-daemon.sh |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh
index 5edced5..4701124 100644
--- a/t/lib-daemon.sh
+++ b/t/lib-daemon.sh
@@ -62,6 +62,10 @@ stop_daemon() {
 	kill_children "$DAEMON_PID"
 	wait "$DAEMON_PID"
 	ret=$?
+	#
+	# We signal TERM=15 to the child and expect the parent to
+	# exit with 143 = 128+15.
+	#
 	if test $ret -ne 143
 	then
 		error "git daemon exited with status: $ret"
-- 
1.7.8

[PATCH 5/6] t5570: repack everything into one file

[Clemens Buchacher]

Subsequently we assume that there is only one pack. Currently this is
true only by accident. Pass '-a -d' to repack in order to guarantee that
assumption to hold true.

The prune-packed command is now redundant since repack -d already calls
it.
---
 t/t5570-git-daemon.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index a7d666c..f2b374b 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -50,8 +50,7 @@ test_expect_failure 'remote detects correct HEAD' '
 test_expect_success 'prepare pack objects' '
 	cp -R "$DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
 	(cd "$DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
-	 git --bare repack &&
-	 git --bare prune-packed
+	 git --bare repack -a -d
 	)
 '
 
-- 
1.7.8

[PATCH 6/6] chmod: use lower-case x

[Clemens Buchacher]

---
 t/t5570-git-daemon.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index f2b374b..d9667f9 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -92,7 +92,7 @@ test_remote_error()
 		case $1 in
 		-x)
 			shift
-			chmod -X "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+			chmod -x "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"
 			;;
 		-n)
 			shift
@@ -126,7 +126,7 @@ test_remote_error()
 	echo "fatal: remote error: $msg: /$repo" >expect &&
 	test_cmp expect output
 	ret=$?
-	chmod +X "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+	chmod +x "$DAEMON_DOCUMENT_ROOT_PATH/repo.git"
 	(exit $ret)
 }
 
-- 
1.7.8

Re: [PATCH 1/2] daemon: add tests

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> Are you saying that the name "daemon" is too general, and it should
> instead be "lib-git-daemon.sh" and GIT_TEST_GIT_DAEMON? Or do you
> mean that it is not general enough and it should be called
> lib-networking.sh and "GIT_TEST_NETWORKING"?

The former. "daemon" is too general and letting "git daemon" squat on that
name makes it harder for other people to build daemons for new git
services and write tests for them.

> Either way, I have no preference here. Feel free to change any way you
> like.

No thanks.

>> >> +	# kill git-daemon child of git
>> >> +	say >&3 "Stopping git daemon ..."
>> >> +	pkill -P "$DAEMON_PID"
>> 
>> How portable is this one (I usually do not trust use of pkill anywhere)?
>
> I read that it is supposed to be more portable than skill or killall.
> But I have no way to research this. I have implemented a workaround
> using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.

Yuck, that patch looks even uglier X-<.

Do you really need to kill the children but not the daemon?

Re: [PATCH 1/2] daemon: add tests

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Clemens Buchacher <drizzd@aon.at> writes:
> ...
>>> >> +	# kill git-daemon child of git
>>> >> +	say >&3 "Stopping git daemon ..."
>>> >> +	pkill -P "$DAEMON_PID"
>>> 
>>> How portable is this one (I usually do not trust use of pkill anywhere)?
>>
>> I read that it is supposed to be more portable than skill or killall.
>> But I have no way to research this. I have implemented a workaround
>> using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.
>
> Yuck, that patch looks even uglier X-<.
>
> Do you really need to kill the children but not the daemon?

To reduce round-trip cost, here is what I'll queue for now.

-- >8 --
From: Clemens Buchacher <drizzd@aon.at>
Date: Wed, 4 Jan 2012 16:55:35 +0100
Subject: [PATCH] daemon: add tests

The semantics of the git daemon tests are similar to the http transport
tests.  In fact, they are only a slightly modified copy of t5550, plus the
newly added remote error tests.

All git-daemon tests will be skipped unless the environment variable
GIT_TEST_GIT_DAEMON is set.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-git-daemon.sh   |   56 ++++++++++++++++++
 t/t5570-git-daemon.sh |  148 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 204 insertions(+), 0 deletions(-)
 create mode 100644 t/lib-git-daemon.sh
 create mode 100755 t/t5570-git-daemon.sh

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
new file mode 100644
index 0000000..c0ff9e2
--- /dev/null
+++ b/t/lib-git-daemon.sh
@@ -0,0 +1,56 @@
+#!/bin/sh
+
+if test -z "$GIT_TEST_GIT_DAEMON"
+then
+	skip_all="git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)"
+	test_done
+fi
+
+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-'8121'}
+
+GIT_DAEMON_PID=
+GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
+GIT_DAEMON_URL=git://127.0.0.1:$LIB_GIT_DAEMON_PORT
+
+start_git_daemon() {
+	if test -n "$GIT_DAEMON_PID"
+	then
+		error "start_git_daemon already called"
+	fi
+
+	mkdir -p "$GIT_DAEMON_DOCUMENT_ROOT_PATH"
+
+	trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
+
+	say >&3 "Starting git daemon ..."
+	git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
+		--reuseaddr --verbose \
+		--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
+		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
+		>&3 2>&4 &
+	GIT_DAEMON_PID=$!
+}
+
+stop_git_daemon() {
+	if test -z "$GIT_DAEMON_PID"
+	then
+		return
+	fi
+
+	trap 'die' EXIT
+
+	# kill git-daemon child of git
+	say >&3 "Stopping git daemon ..."
+	pkill -P "$GIT_DAEMON_PID"
+	wait "$GIT_DAEMON_PID"
+	ret=$?
+	#
+	# We signal TERM=15 to the child and expect the parent to
+	# exit with 143 = 128+15.
+	#
+	if test $ret -ne 143
+	then
+		error "git daemon exited with status: $ret"
+	fi
+	GIT_DAEMON_PID=
+}
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
new file mode 100755
index 0000000..7cbc999
--- /dev/null
+++ b/t/t5570-git-daemon.sh
@@ -0,0 +1,148 @@
+#!/bin/sh
+
+test_description='test fetching over git protocol'
+. ./test-lib.sh
+
+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-5570}
+. "$TEST_DIRECTORY"/lib-git-daemon.sh
+start_git_daemon
+
+test_expect_success 'setup repository' '
+	echo content >file &&
+	git add file &&
+	git commit -m one
+'
+
+test_expect_success 'create git-accessible bare repository' '
+	mkdir "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	 git --bare init &&
+	 : >git-daemon-export-ok
+	) &&
+	git remote add public "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push public master:master
+'
+
+test_expect_success 'clone git repository' '
+	git clone "$GIT_DAEMON_URL/repo.git" clone &&
+	test_cmp file clone/file
+'
+
+test_expect_success 'fetch changes via git protocol' '
+	echo content >>file &&
+	git commit -a -m two &&
+	git push public &&
+	(cd clone && git pull) &&
+	test_cmp file clone/file
+'
+
+test_expect_failure 'remote detects correct HEAD' '
+	git push public master:other &&
+	(cd clone &&
+	 git remote set-head -d origin &&
+	 git remote set-head -a origin &&
+	 git symbolic-ref refs/remotes/origin/HEAD > output &&
+	 echo refs/remotes/origin/master > expect &&
+	 test_cmp expect output
+	)
+'
+
+test_expect_success 'prepare pack objects' '
+	cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	 git --bare repack -a -d
+	)
+'
+
+test_expect_success 'fetch notices corrupt pack' '
+	cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+	 p=`ls objects/pack/pack-*.pack` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad1.git &&
+	(cd repo_bad1.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch "$GIT_DAEMON_URL/repo_bad1.git" &&
+	 test 0 = `ls objects/pack/pack-*.pack | wc -l`
+	)
+'
+
+test_expect_success 'fetch notices corrupt idx' '
+	cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	 p=`ls objects/pack/pack-*.idx` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad2.git &&
+	(cd repo_bad2.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch "$GIT_DAEMON_URL/repo_bad2.git" &&
+	 test 0 = `ls objects/pack | wc -l`
+	)
+'
+
+test_remote_error()
+{
+	do_export=YesPlease
+	while test $# -gt 0
+	do
+		case $1 in
+		-x)
+			shift
+			chmod -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+			;;
+		-n)
+			shift
+			do_export=
+			;;
+		*)
+			break
+		esac
+	done
+
+	if test $# -ne 3
+	then
+		error "invalid number of arguments"
+	fi
+
+	cmd=$1
+	repo=$2
+	msg=$3
+
+	if test -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo"
+	then
+		if test -n "$do_export"
+		then
+			: >"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+		else
+			rm -f "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+		fi
+	fi
+
+	test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" 2>output &&
+	echo "fatal: remote error: $msg: /$repo" >expect &&
+	test_cmp expect output
+	ret=$?
+	chmod +x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+	(exit $ret)
+}
+
+msg="access denied or repository not exported"
+test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git '$msg'"
+test_expect_success 'push disabled'      "test_remote_error    push  repo.git    '$msg'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    '$msg'"
+test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    '$msg'"
+
+stop_git_daemon
+start_git_daemon --informative-errors
+
+test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
+test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
+test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
+
+stop_git_daemon
+test_done
-- 
1.7.8.2.340.gd18f0f

Re: [PATCH 1/2] daemon: add tests

[Clemens Buchacher]

On Wed, Jan 04, 2012 at 10:00:07AM -0800, Junio C Hamano wrote:
>
> >> >> +	# kill git-daemon child of git
> >> >> +	say >&3 "Stopping git daemon ..."
> >> >> +	pkill -P "$DAEMON_PID"
> >> 
> >> How portable is this one (I usually do not trust use of pkill anywhere)?
> >
> > I read that it is supposed to be more portable than skill or killall.
> > But I have no way to research this. I have implemented a workaround
> > using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.
> 
> Yuck, that patch looks even uglier X-<.
> 
> Do you really need to kill the children but not the daemon?

If I kill just the parent "git daemon" command, then the actual
git-daemon (started by run_command) will be left behind.

Re: [PATCH 1/2] daemon: add tests

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Wed, Jan 04, 2012 at 10:00:07AM -0800, Junio C Hamano wrote:
>>
>> >> >> +	# kill git-daemon child of git
>> >> >> +	say >&3 "Stopping git daemon ..."
>> >> >> +	pkill -P "$DAEMON_PID"
>> >> 
>> >> How portable is this one (I usually do not trust use of pkill anywhere)?
>> >
>> > I read that it is supposed to be more portable than skill or killall.
>> > But I have no way to research this. I have implemented a workaround
>> > using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.
>> 
>> Yuck, that patch looks even uglier X-<.
>> 
>> Do you really need to kill the children but not the daemon?
>
> If I kill just the parent "git daemon" command, then the actual
> git-daemon (started by run_command) will be left behind.

Sounds like we would be better off with a new "--foreground" option other
daemon-ish projects seem to have?

Re: [PATCH 1/2] daemon: add tests

[Jeff King]

On Wed, Jan 04, 2012 at 02:15:10PM -0800, Junio C Hamano wrote:

> >> Do you really need to kill the children but not the daemon?
> >
> > If I kill just the parent "git daemon" command, then the actual
> > git-daemon (started by run_command) will be left behind.
> 
> Sounds like we would be better off with a new "--foreground" option other
> daemon-ish projects seem to have?

Isn't that usually for "don't background yourself"? AFAIK, git-daemon
stays in the foreground and only forks to handle each connection. So
can't we just kill the main process, and any running cruft will
eventually die as connections are closed?

Or is the problem the git wrapper itself, which doesn't kill its
subprocess when it dies (which IMHO is a bug which we might want to
fix)? In that case, couldn't we just use --pid-file to save the actual
daemon pid, and then kill using that?

As a side note, it looks like we just start the daemon with "git daemon
&". Doesn't that create a race condition with the tests which
immediately try to access it (i.e., the first test may run before the
daemon actually opens the socket)?

-Peff

Re: [PATCH 1/2] daemon: add tests

[Clemens Buchacher]

On Wed, Jan 04, 2012 at 05:26:49PM -0500, Jeff King wrote:
> 
> Or is the problem the git wrapper itself, which doesn't kill its
> subprocess when it dies (which IMHO is a bug which we might want to
> fix)? In that case, couldn't we just use --pid-file to save the actual
> daemon pid, and then kill using that?

Or like this. Doesn't work with multiple children. I have yet to
check if we have those anywhere.

diff --git a/run-command.c b/run-command.c
index 1c51043..0c105e6 100644
--- a/run-command.c
+++ b/run-command.c
@@ -65,6 +65,22 @@ static int execv_shell_cmd(const char **argv)
 #ifndef WIN32
 static int child_err = 2;
 static int child_notifier = -1;
+static struct child_process *current_cmd;
+
+static void kill_current_cmd(int signo)
+{
+	signal(signo, SIG_DFL);
+
+	if (current_cmd) {
+		if (current_cmd->pid) {
+			/* forward signal to the child process */
+			kill(current_cmd->pid, signo);
+		} else {
+			/* trigger the default signal handler */
+			raise(signo);
+		}
+	}
+}
 
 static void notify_parent(void)
 {
@@ -201,6 +217,9 @@ fail_pipe:
 	if (pipe(notify_pipe))
 		notify_pipe[0] = notify_pipe[1] = -1;
 
+	current_cmd = cmd;
+	signal(SIGTERM, kill_current_cmd);
+
 	cmd->pid = fork();
 	if (!cmd->pid) {
 		/*
diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh
index b2ffd54..c1c41ee 100644
--- a/t/lib-daemon.sh
+++ b/t/lib-daemon.sh
@@ -41,8 +41,8 @@ stop_daemon() {
 
 	# kill git-daemon child of git
 	say >&3 "Stopping git daemon ..."
-	pkill -P "$DAEMON_PID"
-	wait "$DAEMON_PID"
+	kill "$DAEMON_PID"
+	wait "$DAEMON_PID" >&3 2>&4
 	ret=$?
 	#
 	# We signal TERM=15 to the child and expect the parent to

> As a side note, it looks like we just start the daemon with "git daemon
> &". Doesn't that create a race condition with the tests which
> immediately try to access it (i.e., the first test may run before the
> daemon actually opens the socket)?

That's correct. How would I fix that? Try connecting and sleep in a
loop until ready or timeout? Will look into that.

Re: [PATCH 1/2] daemon: add tests

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Wed, Jan 04, 2012 at 05:26:49PM -0500, Jeff King wrote:
>> 
>> Or is the problem the git wrapper itself, which doesn't kill its
>> subprocess when it dies (which IMHO is a bug which we might want to
>> fix)? In that case, couldn't we just use --pid-file to save the actual
>> daemon pid, and then kill using that?
>
> Or like this. Doesn't work with multiple children. I have yet to
> check if we have those anywhere.

Hmm, don't we have them in the same process group or something, though?
Can't we kill them as a whole?

Re: [PATCH 1/2] daemon: add tests

[Clemens Buchacher]

On Wed, Jan 04, 2012 at 04:24:23PM -0800, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > On Wed, Jan 04, 2012 at 05:26:49PM -0500, Jeff King wrote:
> >> 
> >> Or is the problem the git wrapper itself, which doesn't kill its
> >> subprocess when it dies (which IMHO is a bug which we might want to
> >> fix)? In that case, couldn't we just use --pid-file to save the actual
> >> daemon pid, and then kill using that?
> >
> > Or like this. Doesn't work with multiple children. I have yet to
> > check if we have those anywhere.
> 
> Hmm, don't we have them in the same process group or something, though?
> Can't we kill them as a whole?

I tried that, but it seems that the test script itself is in the
same process group.

Re: [PATCH 1/2] daemon: add tests

[Jeff King]

On Thu, Jan 05, 2012 at 01:07:13AM +0100, Clemens Buchacher wrote:

> On Wed, Jan 04, 2012 at 05:26:49PM -0500, Jeff King wrote:
> > 
> > Or is the problem the git wrapper itself, which doesn't kill its
> > subprocess when it dies (which IMHO is a bug which we might want to
> > fix)? In that case, couldn't we just use --pid-file to save the actual
> > daemon pid, and then kill using that?
> 
> Or like this. Doesn't work with multiple children. I have yet to
> check if we have those anywhere.

It so happens that I have just the patch you need. I've been meaning to
go over it again and submit it:

  run-command: optionally kill children on exit
  https://github.com/peff/git/commit/5523d7ebf2a0386c9c61d7bfbc21375041df4989

The original use case was to help with this:

  https://github.com/peff/git/commit/79bf3f232f89c3e2f5284a3b7b71a667be8825d1

> > As a side note, it looks like we just start the daemon with "git daemon
> > &". Doesn't that create a race condition with the tests which
> > immediately try to access it (i.e., the first test may run before the
> > daemon actually opens the socket)?
> 
> That's correct. How would I fix that? Try connecting and sleep in a
> loop until ready or timeout? Will look into that.

Your choices are basically busy-waiting, or convincing the daemon to
send a signal when it's ready to serve. I like the latter, but it does
mean adding a small amount of code to git-daemon.

-Peff

Re: [PATCH 1/2] daemon: add tests

[Clemens Buchacher]

On Wed, Jan 04, 2012 at 09:55:59PM -0500, Jeff King wrote:
> 
> It so happens that I have just the patch you need. I've been meaning to
> go over it again and submit it:
> 
>   run-command: optionally kill children on exit
>   https://github.com/peff/git/commit/5523d7ebf2a0386c9c61d7bfbc21375041df4989

Thanks, looks great. But if I add this on top (to enable this for
"git daemon"), then t0001 kills my entire X session. Not sure yet
what's going.

diff --git a/run-command.c b/run-command.c
index aeb9c6e..53218df 100644
--- a/run-command.c
+++ b/run-command.c
@@ -497,6 +497,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd,
        cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
        cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
        cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
+       cmd->clean_on_exit = 1;
 }
 
 int run_command_v_opt(const char **argv, int opt)

Re: [PATCH 1/2] daemon: add tests

[Jeff King]

On Thu, Jan 05, 2012 at 05:06:15PM +0100, Clemens Buchacher wrote:

> On Wed, Jan 04, 2012 at 09:55:59PM -0500, Jeff King wrote:
> > 
> > It so happens that I have just the patch you need. I've been meaning to
> > go over it again and submit it:
> > 
> >   run-command: optionally kill children on exit
> >   https://github.com/peff/git/commit/5523d7ebf2a0386c9c61d7bfbc21375041df4989
> 
> Thanks, looks great. But if I add this on top (to enable this for
> "git daemon"), then t0001 kills my entire X session. Not sure yet
> what's going.

Yikes. Thanks for noticing.

What happens is we have a failure case in start_command, set pid to -1,
and then fall through to the end of the function. So we end up marking
"-1" for cleanup, which attempts to kill all processes.

I never noticed it because it can only happen when fork() fails, or when
a child process signals an exec failure (which happens all the time with
aliases, but could not be triggered until your patch).

The fix is to move the recording of the PID up to a spot where we are
certain that it's a real PID. Fixup patch is below, and I'll push a new
version out to my github repo.

-Peff

diff --git a/run-command.c b/run-command.c
index aeb9c6e..614b722 100644
--- a/run-command.c
+++ b/run-command.c
@@ -353,6 +353,8 @@ fail_pipe:
 	if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
 			strerror(failed_errno = errno));
+	else if (cmd->clean_on_exit)
+		mark_child_for_cleanup(cmd->pid);
 
 	/*
 	 * Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -374,8 +376,6 @@ fail_pipe:
 	}
 	close(notify_pipe[0]);
 
-	if (cmd->clean_on_exit)
-		mark_child_for_cleanup(cmd->pid);
 }
 #else
 {

Re: [PATCH 1/2] daemon: add tests

[Clemens Buchacher]

On Fri, Jan 06, 2012 at 10:52:04AM -0500, Jeff King wrote:
> > > 
> > >   run-command: optionally kill children on exit
> > >   https://github.com/peff/git/commit/5523d7ebf2a0386c9c61d7bfbc21375041df4989
> > 
> > Thanks, looks great. But if I add this on top (to enable this for
> > "git daemon"), then t0001 kills my entire X session. Not sure yet
> > what's going.
> 
> The fix is to move the recording of the PID up to a spot where we are
> certain that it's a real PID. Fixup patch is below, and I'll push a new
> version out to my github repo.

I have rebased Junio's cb/git-daemon-tests onto your
jk/child-cleanup and replaced the call to pkill with a regular kill
command.

On top of that, I have added two commits to fix the discussed race
condition. I also verified that the race condition actually happens
by adding an artificial delay in the daemon (this change is
obviously not included).

I pushed the new cb/git-daemon-tests to
https://github.com/drizzd/git . If you have no objections I will
post the entire series including your run-command and send-pack
patches to the list.

Clemens

Re: [PATCH 1/2] daemon: add tests

[Jeff King]

On Fri, Jan 06, 2012 at 08:48:00PM +0100, Clemens Buchacher wrote:

> I have rebased Junio's cb/git-daemon-tests onto your
> jk/child-cleanup and replaced the call to pkill with a regular kill
> command.

Looks pretty good from my cursory examination. I think you should fill
out the rationale for "kill dashed externals on exit" a bit. My
reasoning is that whether a git command is an internal or external
process is purely an implementation detail, and killing the git wrapper
should behave identically in both cases.

> On top of that, I have added two commits to fix the discussed race
> condition. I also verified that the race condition actually happens
> by adding an artificial delay in the daemon (this change is
> obviously not included).

Looks reasonable to me.

> I pushed the new cb/git-daemon-tests to
> https://github.com/drizzd/git . If you have no objections I will
> post the entire series including your run-command and send-pack
> patches to the list.

No objections here. Thanks for moving this forward.

-Peff

[PATCH] credentials: unable to connect to cache daemon

[Clemens Buchacher]

Error out if we just spawned the daemon and yet we cannot connect.

And always release the string buffer.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

Hi Jeff,

I wrote this while debugging why t0301-credential-cache.sh failed after
I enabled cleanup_children by default. This error condition turned out
not to be the problem, and this patch would not have helped in debugging
this case. But I think it makes sense anyways.

 credential-cache.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/credential-cache.c b/credential-cache.c
index dc98372..8f25c06 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -71,11 +71,10 @@ static void do_cache(const char *socket, const char *action, int timeout,
 			die_errno("unable to relay credential");
 	}
 
-	if (!send_request(socket, &buf))
-		return;
-	if (flags & FLAG_SPAWN) {
+	if (send_request(socket, &buf) < 0 && flags & FLAG_SPAWN) {
 		spawn_daemon(socket);
-		send_request(socket, &buf);
+		if (send_request(socket, &buf) < 0)
+			die_errno("unable to connect to cache daemon");
 	}
 	strbuf_release(&buf);
 }
-- 
1.7.8

Re: [PATCH] credentials: unable to connect to cache daemon

[Jeff King]

On Sat, Jan 07, 2012 at 12:54:36PM +0100, Clemens Buchacher wrote:

> Error out if we just spawned the daemon and yet we cannot connect.

Actually it was intentional not to produce an error. The cache helper is
just a cache, so I consider it "best effort", and if it cannot cache a
password, it's not the end of the world. Git should continue, anyway.

That being said, it's probably nicer to be informative in this case than
not, since it is a configuration error the user probably would like to
fix. And since the rewrite of the credential helper API, it's OK for
helpers to return a failing exit code; git will just ignore it and keep
going.

So I think this is a reasonable thing to do.

Acked-by: Jeff King <peff@peff.net>

> And always release the string buffer.

Oops, thanks.

-Peff

Re: [PATCH 1/2] daemon: add tests

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> I have rebased Junio's cb/git-daemon-tests onto your
> jk/child-cleanup and replaced the call to pkill with a regular kill
> command.
>
> On top of that, I have added two commits to fix the discussed race
> condition. I also verified that the race condition actually happens
> by adding an artificial delay in the daemon (this change is
> obviously not included).
>
> I pushed the new cb/git-daemon-tests to
> https://github.com/drizzd/git . If you have no objections I will
> post the entire series including your run-command and send-pack
> patches to the list.

Looked fine except that some patches seem to lack enough justification
(justification in Peff's reply was good enough).

I actually was thinking that the previous round was good enough (perhaps
dropping the "pkill" bit altogether and replacing it with "kill" on the
daemon process itself, if OSX folks complain loudly), so it is in "next"
already, but it seems that the best course of action would be to drop it
and queue your re-roll afresh, aiming for the next cycle.

Thanks.

Re: [PATCH 1/2] daemon: add tests

[Clemens Buchacher]

On Fri, Jan 06, 2012 at 02:49:05PM -0800, Junio C Hamano wrote:
> 
> but it seems that the best course of action would be to drop it
> and queue your re-roll afresh, aiming for the next cycle.

Here's the re-rolled series, also available as cb/git-daemon-tests based
on current master at https://github.com/drizzd/git .

[PATCH 1/5] run-command: optionally kill children on exit
[PATCH 2/5] run-command: kill children on exit by default
[PATCH 3/5] git-daemon: add tests
[PATCH 4/5] git-daemon: produce output when ready
[PATCH 5/5] git-daemon tests: wait until daemon is ready

On Fri, Jan 06, 2012 at 05:32:15PM -0500, Jeff King wrote:
> On Fri, Jan 06, 2012 at 08:48:00PM +0100, Clemens Buchacher wrote:
> 
> > I have rebased Junio's cb/git-daemon-tests onto your
> > jk/child-cleanup and replaced the call to pkill with a regular kill
> > command.
> 
> Looks pretty good from my cursory examination. I think you should fill
> out the rationale for "kill dashed externals on exit" a bit. My
> reasoning is that whether a git command is an internal or external
> process is purely an implementation detail, and killing the git wrapper
> should behave identically in both cases.

The previous version of this patch only changed the behavior for users
of run_command_v_opt, but not for those who filled out the child_process
structure by themselves. I could have manually enabled all of those, but
that felt unnatural. Instead, I have now reversed the meaning of
clean_on_exit to stay_alive_on_exit in [PATCH 2/5] run-command: kill
children on exit by default.  Cleanup is on by default and callers of
run_command must disable it if children should stay alive.

[PATCH 1/5] run-command: optionally kill children on exit

[Clemens Buchacher]

From: Jeff King <peff@peff.net>

When we spawn a helper process, it should generally be done
and finish_command called before we exit. However, if we
exit abnormally due to an early return or a signal, the
helper may continue to run in our absence.

In the best case, this may simply be wasted CPU cycles or a
few stray messages on a terminal. But it could also mean a
process that the user thought was aborted continues to run
to completion (e.g., a push's pack-objects helper will
complete the push, even though you killed the push process).

This patch provides infrastructure for run-command to keep
track of PIDs to be killed, and clean them on signal
reception or input, just as we do with tempfiles. PIDs can
be added in two ways:

  1. If NO_PTHREADS is defined, async helper processes are
     automatically marked. By definition this code must be
     ready to die when the parent dies, since it may be
     implemented as a thread of the parent process.

  2. If the run-command caller specifies the "clean_on_exit"
     option. This is not the default, as there are cases
     where it is OK for the child to outlive us (e.g., when
     spawning a pager).

PIDs are cleared from the kill-list automatically during
wait_or_whine, which is called from finish_command and
finish_async.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

Not sure if I can sign off without your sign-off. Should I have
replaced this with Acked-by?

 run-command.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 run-command.h |    1 +
 2 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1c51043..0204aaf 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1,8 +1,66 @@
 #include "cache.h"
 #include "run-command.h"
 #include "exec_cmd.h"
+#include "sigchain.h"
 #include "argv-array.h"
 
+struct child_to_clean {
+	pid_t pid;
+	struct child_to_clean *next;
+};
+static struct child_to_clean *children_to_clean;
+static int installed_child_cleanup_handler;
+
+static void cleanup_children(int sig)
+{
+	while (children_to_clean) {
+		struct child_to_clean *p = children_to_clean;
+		children_to_clean = p->next;
+		kill(p->pid, sig);
+		free(p);
+	}
+}
+
+static void cleanup_children_on_signal(int sig)
+{
+	cleanup_children(sig);
+	sigchain_pop(sig);
+	raise(sig);
+}
+
+static void cleanup_children_on_exit(void)
+{
+	cleanup_children(SIGTERM);
+}
+
+static void mark_child_for_cleanup(pid_t pid)
+{
+	struct child_to_clean *p = xmalloc(sizeof(*p));
+	p->pid = pid;
+	p->next = children_to_clean;
+	children_to_clean = p;
+
+	if (!installed_child_cleanup_handler) {
+		atexit(cleanup_children_on_exit);
+		sigchain_push_common(cleanup_children_on_signal);
+		installed_child_cleanup_handler = 1;
+	}
+}
+
+static void clear_child_for_cleanup(pid_t pid)
+{
+	struct child_to_clean **last, *p;
+
+	last = &children_to_clean;
+	for (p = children_to_clean; p; p = p->next) {
+		if (p->pid == pid) {
+			*last = p->next;
+			free(p);
+			return;
+		}
+	}
+}
+
 static inline void close_pair(int fd[2])
 {
 	close(fd[0]);
@@ -130,6 +188,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 	} else {
 		error("waitpid is confused (%s)", argv0);
 	}
+
+	clear_child_for_cleanup(pid);
+
 	errno = failed_errno;
 	return code;
 }
@@ -292,6 +353,8 @@ fail_pipe:
 	if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
 			strerror(failed_errno = errno));
+	else if (cmd->clean_on_exit)
+		mark_child_for_cleanup(cmd->pid);
 
 	/*
 	 * Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -312,6 +375,7 @@ fail_pipe:
 		cmd->pid = -1;
 	}
 	close(notify_pipe[0]);
+
 }
 #else
 {
@@ -356,6 +420,8 @@ fail_pipe:
 	failed_errno = errno;
 	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
 		error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
+	if (cmd->clean_on_exit && cmd->pid >= 0)
+		mark_child_for_cleanup(cmd->pid);
 
 	if (cmd->env)
 		free_environ(env);
@@ -540,6 +606,8 @@ int start_async(struct async *async)
 		exit(!!async->proc(proc_in, proc_out, async->data));
 	}
 
+	mark_child_for_cleanup(async->pid);
+
 	if (need_in)
 		close(fdin[0]);
 	else if (async->in)
diff --git a/run-command.h b/run-command.h
index 56491b9..2a69466 100644
--- a/run-command.h
+++ b/run-command.h
@@ -38,6 +38,7 @@ struct child_process {
 	unsigned silent_exec_failure:1;
 	unsigned stdout_to_stderr:1;
 	unsigned use_shell:1;
+	unsigned clean_on_exit:1;
 	void (*preexec_cb)(void);
 };
 
-- 
1.7.8

Re: [PATCH 1/5] run-command: optionally kill children on exit

[Erik Faye-Lund]

On Sat, Jan 7, 2012 at 12:42 PM, Clemens Buchacher <drizzd@aon.at> wrote:
> +static void cleanup_children(int sig)
> +{
> +       while (children_to_clean) {
> +               struct child_to_clean *p = children_to_clean;
> +               children_to_clean = p->next;
> +               kill(p->pid, sig);
> +               free(p);
> +       }
> +}
> +
> +static void cleanup_children_on_signal(int sig)
> +{
> +       cleanup_children(sig);
> +       sigchain_pop(sig);
> +       raise(sig);
> +}
> +

Our Windows implementation of kill (mingw_kill in compat/mingw.c) only
supports SIGKILL, so propagating other signals to child-processes will
fail with EINVAL. That being said, Windows' support for signals is
severely limited, but I'm not entirely sure which ones can be
generated in this case.

> @@ -312,6 +375,7 @@ fail_pipe:
>                cmd->pid = -1;
>        }
>        close(notify_pipe[0]);
> +
>  }
>  #else
>  {

This hunk is probably unintentional...

Re: [PATCH 1/5] run-command: optionally kill children on exit

[Clemens Buchacher]

On Sat, Jan 07, 2012 at 01:45:03PM +0100, Erik Faye-Lund wrote:
> 
> Our Windows implementation of kill (mingw_kill in compat/mingw.c) only
> supports SIGKILL, so propagating other signals to child-processes will
> fail with EINVAL. That being said, Windows' support for signals is
> severely limited, but I'm not entirely sure which ones can be
> generated in this case.

On Linux at least, SIGKILL is not a viable alternative for SIGTERM,
since it does not give the child process to do any cleanup of its own
(such as signaling its own children, for example).

In any case, due this whole experience, and recently another one with
overzealous virus scanners, I have added a "get rid of dashed externals"
work item to my TODO list.

Re: [PATCH 1/5] run-command: optionally kill children on exit

[Jeff King]

On Sat, Jan 07, 2012 at 12:42:43PM +0100, Clemens Buchacher wrote:

> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
> 
> Not sure if I can sign off without your sign-off. Should I have
> replaced this with Acked-by?

Sorry, I usually sign-off when I sent to the list. But:

Signed-off-by: Jeff King <peff@peff.net>

for this and the other patch in this series.

As for whether you can sign-off, I think it is OK in this case. You are
basically signing off on the "Certificate of Origin" found in
SubmittingPatches. I think you are covered under (b), which is that to
the best of your knowledge it is based on open source work (i.e., even
though I didn't sign off explicitly, it is pretty obvious that this is
meant to be open source). But it's nicer to be explicit.

-Peff

[PATCH 2/5] run-command: kill children on exit by default

[Clemens Buchacher]

It feels natural for a user to view git commands as monolithic
commands with a single thread of execution. If the parent git
command dies, it should therefore clean up its child processes as
well. So enable the cleanup mechanism by default.

For dashed externals, this means that killing the git wrapper will
kill the command itself, just like what would happen in case of an
internal command. A notable exception is the credentials cache
daemon, which must stay alive after the store command has
completed.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

I considered squashing this into the previous commit. But it's a fairly
small change and may help with bisecting in case of problems.

 credential-cache.c |    1 +
 run-command.c      |    4 ++--
 run-command.h      |    2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/credential-cache.c b/credential-cache.c
index dc98372..15e7236 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -48,6 +48,7 @@ static void spawn_daemon(const char *socket)
 	daemon.argv = argv;
 	daemon.no_stdin = 1;
 	daemon.out = -1;
+	daemon.stay_alive_on_exit = 1;
 
 	if (start_command(&daemon))
 		die_errno("unable to start cache daemon");
diff --git a/run-command.c b/run-command.c
index 0204aaf..fe07b20 100644
--- a/run-command.c
+++ b/run-command.c
@@ -353,7 +353,7 @@ fail_pipe:
 	if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
 			strerror(failed_errno = errno));
-	else if (cmd->clean_on_exit)
+	else if (!cmd->stay_alive_on_exit)
 		mark_child_for_cleanup(cmd->pid);
 
 	/*
@@ -420,7 +420,7 @@ fail_pipe:
 	failed_errno = errno;
 	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
 		error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
-	if (cmd->clean_on_exit && cmd->pid >= 0)
+	if (!cmd->stay_alive_on_exit && cmd->pid >= 0)
 		mark_child_for_cleanup(cmd->pid);
 
 	if (cmd->env)
diff --git a/run-command.h b/run-command.h
index 2a69466..69dbea1 100644
--- a/run-command.h
+++ b/run-command.h
@@ -38,7 +38,7 @@ struct child_process {
 	unsigned silent_exec_failure:1;
 	unsigned stdout_to_stderr:1;
 	unsigned use_shell:1;
-	unsigned clean_on_exit:1;
+	unsigned stay_alive_on_exit:1;
 	void (*preexec_cb)(void);
 };
 
-- 
1.7.8

Re: [PATCH 2/5] run-command: kill children on exit by default

[Jeff King]

On Sat, Jan 07, 2012 at 12:42:44PM +0100, Clemens Buchacher wrote:

> It feels natural for a user to view git commands as monolithic
> commands with a single thread of execution. If the parent git
> command dies, it should therefore clean up its child processes as
> well. So enable the cleanup mechanism by default.

I'm not sure this is a good idea. run_command is used in ~70 places in
git, and I'm sure at least one of them is going to be unhappy (I see you
found one in credential-cache, but how many others are there). I'd
rather be conservative and leave the default the same, and then switch
over callsites that make sense.

-Peff

PS I thought this would certainly break the pager, since it should
   outlast us after we finish producing output. But I think at one point
   I switched the pager invocation so that the git wrapper lives and
   waits until the pager dies.

Re: [PATCH 2/5] run-command: kill children on exit by default

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Jan 07, 2012 at 12:42:44PM +0100, Clemens Buchacher wrote:
>
>> It feels natural for a user to view git commands as monolithic
>> commands with a single thread of execution. If the parent git
>> command dies, it should therefore clean up its child processes as
>> well. So enable the cleanup mechanism by default.
>
> I'm not sure this is a good idea. run_command is used in ~70 places in
> git, and I'm sure at least one of them is going to be unhappy (I see you
> found one in credential-cache, but how many others are there). I'd
> rather be conservative and leave the default the same, and then switch
> over callsites that make sense.

Yeah, I agree 100% with that reasoning. I seem to recall that was how this
commit was done in what I privately reviewed after Clemens announced his
github branch?

[PATCH 2/5 v2] dashed externals: kill children on exit

[Clemens Buchacher]

Several git commands are so-called dashed externals, that is commands
executed as a child process of the git wrapper command. If the git
wrapper is killed by a signal, the child process will continue to run.
This is different from internal commands, which always die with the git
wrapper command.

Enable the recently introduced cleanup mechanism for child processes in
order to make dashed externals act more in line with internal commands.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Sat, Jan 07, 2012 at 10:26:49PM -0800, Junio C Hamano wrote:
> 
> Yeah, I agree 100% with that reasoning. I seem to recall that was how this
> commit was done in what I privately reviewed after Clemens announced his
> github branch?

What I had previously enabled child cleanup for all callers of
run_command_v_opt. There is quite a few of those as well, most of them
not related to dashed externals at all.

So I reworked that patch a bit to enable cleanup only for dashed
externals. This is a replacement for "[PATCH 2/5] run-command: kill
children on exit by default".

I also re-added Jeff's "send-pack: kill pack-objects helper on signal or
exit" and I dropped the extraneous newline that Erik spotted in
"[PATCH 1/5] run-command: optionally kill children on exit".

I have pushed the reworked series to cb/git-daemon-tests on my github
repo.

 git.c         |    2 +-
 run-command.c |    1 +
 run-command.h |    1 +
 3 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/git.c b/git.c
index fb9029c..3805616 100644
--- a/git.c
+++ b/git.c
@@ -495,7 +495,7 @@ static void execv_dashed_external(const char **argv)
 	 * if we fail because the command is not found, it is
 	 * OK to return. Otherwise, we just pass along the status code.
 	 */
-	status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);
+	status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT);
 	if (status >= 0 || errno != ENOENT)
 		exit(status);
 
diff --git a/run-command.c b/run-command.c
index fff9073..90bfd8c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -496,6 +496,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd,
 	cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
 	cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
 	cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
+	cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
 }
 
 int run_command_v_opt(const char **argv, int opt)
diff --git a/run-command.h b/run-command.h
index 2a69466..44f7d2b 100644
--- a/run-command.h
+++ b/run-command.h
@@ -53,6 +53,7 @@ extern int run_hook(const char *index_file, const char *name, ...);
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
 #define RUN_SILENT_EXEC_FAILURE 8
 #define RUN_USING_SHELL 16
+#define RUN_CLEAN_ON_EXIT 32
 int run_command_v_opt(const char **argv, int opt);
 
 /*
-- 
1.7.8

Re: [PATCH 2/5 v2] dashed externals: kill children on exit

[Jeff King]

On Sun, Jan 08, 2012 at 09:41:09PM +0100, Clemens Buchacher wrote:

> What I had previously enabled child cleanup for all callers of
> run_command_v_opt. There is quite a few of those as well, most of them
> not related to dashed externals at all.
> 
> So I reworked that patch a bit to enable cleanup only for dashed
> externals. This is a replacement for "[PATCH 2/5] run-command: kill
> children on exit by default".

Thanks, this looks much closer to what I was expecting.

-Peff

[PATCH 3/5] git-daemon: add tests

[Clemens Buchacher]

The semantics of the git daemon tests are similar to the http transport
tests.  In fact, they are only a slightly modified copy of t5550, plus the
newly added remote error tests.

All git-daemon tests will be skipped unless the environment variable
GIT_TEST_GIT_DAEMON is set.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-git-daemon.sh   |   53 +++++++++++++++++
 t/t5570-git-daemon.sh |  148 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 201 insertions(+), 0 deletions(-)
 create mode 100644 t/lib-git-daemon.sh
 create mode 100755 t/t5570-git-daemon.sh

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
new file mode 100644
index 0000000..5e81a25
--- /dev/null
+++ b/t/lib-git-daemon.sh
@@ -0,0 +1,53 @@
+#!/bin/sh
+
+if test -z "$GIT_TEST_GIT_DAEMON"
+then
+	skip_all="git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)"
+	test_done
+fi
+
+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-'8121'}
+
+GIT_DAEMON_PID=
+GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
+GIT_DAEMON_URL=git://127.0.0.1:$LIB_GIT_DAEMON_PORT
+
+start_git_daemon() {
+	if test -n "$GIT_DAEMON_PID"
+	then
+		error "start_git_daemon already called"
+	fi
+
+	mkdir -p "$GIT_DAEMON_DOCUMENT_ROOT_PATH"
+
+	trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
+
+	say >&3 "Starting git daemon ..."
+	git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
+		--reuseaddr --verbose \
+		--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
+		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
+		>&3 2>&4 &
+	GIT_DAEMON_PID=$!
+}
+
+stop_git_daemon() {
+	if test -z "$GIT_DAEMON_PID"
+	then
+		return
+	fi
+
+	trap 'die' EXIT
+
+	# kill git-daemon child of git
+	say >&3 "Stopping git daemon ..."
+	kill "$GIT_DAEMON_PID"
+	wait "$GIT_DAEMON_PID" >&3 2>&4
+	ret=$?
+	# expect exit with status 143 = 128+15 for signal TERM=15
+	if test $ret -ne 143
+	then
+		error "git daemon exited with status: $ret"
+	fi
+	GIT_DAEMON_PID=
+}
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
new file mode 100755
index 0000000..7cbc999
--- /dev/null
+++ b/t/t5570-git-daemon.sh
@@ -0,0 +1,148 @@
+#!/bin/sh
+
+test_description='test fetching over git protocol'
+. ./test-lib.sh
+
+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-5570}
+. "$TEST_DIRECTORY"/lib-git-daemon.sh
+start_git_daemon
+
+test_expect_success 'setup repository' '
+	echo content >file &&
+	git add file &&
+	git commit -m one
+'
+
+test_expect_success 'create git-accessible bare repository' '
+	mkdir "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	 git --bare init &&
+	 : >git-daemon-export-ok
+	) &&
+	git remote add public "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push public master:master
+'
+
+test_expect_success 'clone git repository' '
+	git clone "$GIT_DAEMON_URL/repo.git" clone &&
+	test_cmp file clone/file
+'
+
+test_expect_success 'fetch changes via git protocol' '
+	echo content >>file &&
+	git commit -a -m two &&
+	git push public &&
+	(cd clone && git pull) &&
+	test_cmp file clone/file
+'
+
+test_expect_failure 'remote detects correct HEAD' '
+	git push public master:other &&
+	(cd clone &&
+	 git remote set-head -d origin &&
+	 git remote set-head -a origin &&
+	 git symbolic-ref refs/remotes/origin/HEAD > output &&
+	 echo refs/remotes/origin/master > expect &&
+	 test_cmp expect output
+	)
+'
+
+test_expect_success 'prepare pack objects' '
+	cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	 git --bare repack -a -d
+	)
+'
+
+test_expect_success 'fetch notices corrupt pack' '
+	cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
+	 p=`ls objects/pack/pack-*.pack` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad1.git &&
+	(cd repo_bad1.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch "$GIT_DAEMON_URL/repo_bad1.git" &&
+	 test 0 = `ls objects/pack/pack-*.pack | wc -l`
+	)
+'
+
+test_expect_success 'fetch notices corrupt idx' '
+	cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	 p=`ls objects/pack/pack-*.idx` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad2.git &&
+	(cd repo_bad2.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch "$GIT_DAEMON_URL/repo_bad2.git" &&
+	 test 0 = `ls objects/pack | wc -l`
+	)
+'
+
+test_remote_error()
+{
+	do_export=YesPlease
+	while test $# -gt 0
+	do
+		case $1 in
+		-x)
+			shift
+			chmod -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+			;;
+		-n)
+			shift
+			do_export=
+			;;
+		*)
+			break
+		esac
+	done
+
+	if test $# -ne 3
+	then
+		error "invalid number of arguments"
+	fi
+
+	cmd=$1
+	repo=$2
+	msg=$3
+
+	if test -x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo"
+	then
+		if test -n "$do_export"
+		then
+			: >"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+		else
+			rm -f "$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok"
+		fi
+	fi
+
+	test_must_fail git "$cmd" "$GIT_DAEMON_URL/$repo" 2>output &&
+	echo "fatal: remote error: $msg: /$repo" >expect &&
+	test_cmp expect output
+	ret=$?
+	chmod +x "$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git"
+	(exit $ret)
+}
+
+msg="access denied or repository not exported"
+test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git '$msg'"
+test_expect_success 'push disabled'      "test_remote_error    push  repo.git    '$msg'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    '$msg'"
+test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    '$msg'"
+
+stop_git_daemon
+start_git_daemon --informative-errors
+
+test_expect_success 'clone non-existent' "test_remote_error    clone nowhere.git 'no such repository'"
+test_expect_success 'push disabled'      "test_remote_error    push  repo.git    'service not enabled'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git    'no such repository'"
+test_expect_success 'not exported'       "test_remote_error -n fetch repo.git    'repository not exported'"
+
+stop_git_daemon
+test_done
-- 
1.7.8

[PATCH 4/5] git-daemon: produce output when ready

[Clemens Buchacher]

If a client tries to connect after git-daemon starts, but before it
opens a listening socket, the connection will fail. Output "[PID]
Ready to rumble]" after opening the socket successfully in order to
inform the user that the daemon is now ready to receive
connections.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 daemon.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/daemon.c b/daemon.c
index 15ce918..ab21e66 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1086,6 +1086,8 @@ static int serve(struct string_list *listen_addr, int listen_port,
 
 	drop_privileges(cred);
 
+	loginfo("Ready to rumble");
+
 	return service_loop(&socklist);
 }
 
@@ -1270,10 +1272,8 @@ int main(int argc, char **argv)
 	if (inetd_mode || serve_mode)
 		return execute();
 
-	if (detach) {
+	if (detach)
 		daemonize();
-		loginfo("Ready to rumble");
-	}
 	else
 		sanitize_stdfds();
 
-- 
1.7.8

[PATCH 5/5] git-daemon tests: wait until daemon is ready

[Clemens Buchacher]

In start_daemon, git-daemon is started as a background process.  In
theory, the tests may try to connect before the daemon had a chance
to open a listening socket. Avoid this race condition by waiting
for it to output "Ready to rumble". Any other output is considered
an error and the test is aborted.

Should git-daemon produce no output at all, lib-git-daemon would
block forever. This could be fixed by introducing a timeout.  On
the other hand, we have no timeout for other git commands which
could suffer from the same problem. Since such a mechanism adds
some complexity, I have decided against it.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/lib-git-daemon.sh |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 5e81a25..ef2d01f 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -23,12 +23,27 @@ start_git_daemon() {
 	trap 'code=$?; stop_git_daemon; (exit $code); die' EXIT
 
 	say >&3 "Starting git daemon ..."
+	mkfifo git_daemon_output
 	git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
 		--reuseaddr --verbose \
 		--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
 		"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
-		>&3 2>&4 &
+		>&3 2>git_daemon_output &
 	GIT_DAEMON_PID=$!
+	{
+		read line
+		echo >&4 "$line"
+		cat >&4 &
+
+		# Check expected output
+		if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
+		then
+			kill "$GIT_DAEMON_PID"
+			wait "$GIT_DAEMON_PID"
+			trap 'die' EXIT
+			error "git daemon failed to start"
+		fi
+	} <git_daemon_output
 }
 
 stop_git_daemon() {
@@ -50,4 +65,5 @@ stop_git_daemon() {
 		error "git daemon exited with status: $ret"
 	fi
 	GIT_DAEMON_PID=
+	rm -f git_daemon_output
 }
-- 
1.7.8

Re: [PATCH 1/2] daemon: add tests

[Jakub Narebski]

Jeff King <peff@peff.net> writes:

> As a side note, it looks like we just start the daemon with "git daemon
> &". Doesn't that create a race condition with the tests which
> immediately try to access it (i.e., the first test may run before the
> daemon actually opens the socket)?

Hmmm... perhaps the trick that git-instaweb does for "plackup" web
server would be of use here, waiting for socket to be ready?

-- 
Jakub Narebski

Re: [PATCH 1/2] daemon: add tests

[Jeff King]

On Wed, Jan 04, 2012 at 06:24:16PM -0800, Jakub Narebski wrote:

> Jeff King <peff@peff.net> writes:
> 
> > As a side note, it looks like we just start the daemon with "git daemon
> > &". Doesn't that create a race condition with the tests which
> > immediately try to access it (i.e., the first test may run before the
> > daemon actually opens the socket)?
> 
> Hmmm... perhaps the trick that git-instaweb does for "plackup" web
> server would be of use here, waiting for socket to be ready?

It looks like it busy loops, which is kind of ugly.

The credential-cache helper has a similar problem. It wants to kick off
a daemon if one is not already running, and then connect to it. So the
daemon does:

  printf("ok\n");
  fclose(stdout);

when it has set up the socket, and the client does:

  r = read_in_full(daemon.out, buf, sizeof(buf));
  if (r < 0)
          die_errno("unable to read result code from cache daemon");
  if (r != 3 || memcmp(buf, "ok\n", 3))
          die("cache daemon did not start: %.*s", r, buf);
  /* now we can connect over the socket */

We could probably add a "--notify-when-ready" option to git-daemon to
do something similar.

-Peff

Re: [PATCH 1/2] daemon: add tests

[Jakub Narebski]

On Thu, 5 Jan 2012, Jeff King wrote:
> On Wed, Jan 04, 2012 at 06:24:16PM -0800, Jakub Narebski wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > As a side note, it looks like we just start the daemon with "git daemon
> > > &". Doesn't that create a race condition with the tests which
> > > immediately try to access it (i.e., the first test may run before the
> > > daemon actually opens the socket)?
> > 
> > Hmmm... perhaps the trick that git-instaweb does for "plackup" web
> > server would be of use here, waiting for socket to be ready?
> 
> It looks like it busy loops, which is kind of ugly.

Well, as far as I know you can wait for data on socket or pipe, but
you can't wait for socket to be created.

Anyway this busy-wait is not too busy, and it is better than just
adding 'sleep 1' in testsuite.

> The credential-cache helper has a similar problem. It wants to kick off
> a daemon if one is not already running, and then connect to it. So the
> daemon does:
> 
>   printf("ok\n");
>   fclose(stdout);
> 
> when it has set up the socket, and the client does:
> 
>   r = read_in_full(daemon.out, buf, sizeof(buf));
>   if (r < 0)
>           die_errno("unable to read result code from cache daemon");
>   if (r != 3 || memcmp(buf, "ok\n", 3))
>           die("cache daemon did not start: %.*s", r, buf);
>   /* now we can connect over the socket */
> 
> We could probably add a "--notify-when-ready" option to git-daemon to
> do something similar.

What would git-daemon do what it is ready?  Write to socket, raise signal,
print to STDOUT / STDERR?

BTW. I wonder if it would be worth it to add something a la systemd
trick creating sockets first to git-daemon.  Adding systemd support
doesn't make sense for daemon that is to be run from inetd / xinetd,
I guess.

-- 
Jakub Narebski
Poland

Re: [PATCH 1/2] daemon: add tests

[Clemens Buchacher]

On Sat, Jan 07, 2012 at 12:35:50AM +0100, Jakub Narebski wrote:
> > 
> > We could probably add a "--notify-when-ready" option to git-daemon to
> > do something similar.
> 
> What would git-daemon do what it is ready?  Write to socket, raise signal,
> print to STDOUT / STDERR?

Please have a look at my "git-daemon: produce output when ready" patch.
After opening the socket, git-daemon --verbose writes "Ready to rumble"
to stderr.

Re: [PATCH 1/2] daemon: add tests

[Brian Gernhardt]

On Jan 4, 2012, at 10:55 AM, Clemens Buchacher wrote:

> On Tue, Jan 03, 2012 at 11:34:08AM -0800, Junio C Hamano wrote:
>>> +	# kill git-daemon child of git
>>> +	say >&3 "Stopping git daemon ..."
>>> +	pkill -P "$DAEMON_PID"
>> 
>> How portable is this one (I usually do not trust use of pkill anywhere)?
> 
> I read that it is supposed to be more portable than skill or killall.
> But I have no way to research this. I have implemented a workaround
> using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.

As a data point:  pkill and skill do not exist on OS X 10.7, but killall does.

~~ Brian Gernhardt

Re: [PATCH] transport: do not allow to push over git:// protocol

[Jakub Narebski]

Johannes Sixt <j.sixt@viscovery.net> writes:
> Am 10/3/2011 9:42, schrieb Jeff King:
> > I still think push-over-git:// is a bit insane, and especially now with
> > smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
> > seeing it deprecated.
> 
> You must be kidding ;) It is so much easier to type
> 
>   git daemon --export-all --enable=receive-pack
> 
> for a one-shot, temporary git connection compared to setting up a
> smart-http, ssh, or even a rsh server.

I wonder if that is the case... but 48% responders of "Git User's
Survey 2011" (3424 out of 7100 responders who answered queston 
"23) How do you publish/propagate your changes?") answered that they
use push via git protocol.

See https://www.survs.com/results/Q5CA9SKQ/P7DE07F0PL

-- 
Jakub Narębski

Re: [PATCH] transport: do not allow to push over git:// protocol

[Jeff King]

On Mon, Oct 03, 2011 at 02:49:15AM -0700, Jakub Narebski wrote:

> I wonder if that is the case... but 48% responders of "Git User's
> Survey 2011" (3424 out of 7100 responders who answered queston 
> "23) How do you publish/propagate your changes?") answered that they
> use push via git protocol.
> 
> See https://www.survs.com/results/Q5CA9SKQ/P7DE07F0PL

I refuse to believe that 48% of people are using git:// to push. Surely
they are interpreting that response to overlap with "git over ssh" and
"git over http".

-Peff

Re: [PATCH] transport: do not allow to push over git:// protocol

[Ilari Liusvaara]

On Mon, Oct 03, 2011 at 03:42:51AM -0400, Jeff King wrote:
> On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyen Thai Ngoc Duy wrote:
> 
> The real problem here seems to be that instead of communicating "no, we
> don't support that", git-daemon just hangs up. It would be a much nicer
> fix if we could change that. I'm not sure it's possible, though. There's
> not much room in the beginning of the room to make that communication in
> a way that's backwards compatible.

Oh, sure it is possible (except for remote snapshot):

$ /usr/bin/git fetch git://localhost/foobar
fatal: remote error: R access for foobar DENIED to anonymous
$ /usr/bin/git push git://localhost/foobar
fatal: remote error: W access for foobar DENIED to anonymous
$ /usr/bin/git archive --remote=git://localhost/foobar HEAD
fatal: git archive: protocol error
$ /usr/bin/git --version
git version 1.7.6.3

Supported for fetch and push since 1.6.1-rc1 (And 1.6.1 was over
2.5 years ago). Oh, and even before that, but with slightly more
ugly error message.

Oh, and adding interpretation of ERR packets to git archive is easy
(and I even happen to have git:// server that can send those to
test against):

$ git archive --remote=git://localhost/foobar HEAD
fatal: remote error: R access for foobar DENIED to anonymous

(I also tested that remote snapshotting of repository that should be
readable succeeds, it does).

--- >8 ----
From ce3a402e4fa72cf603f92801d6f021ff89d3ac35 Mon Sep 17 00:00:00 2001
From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Date: Mon, 3 Oct 2011 13:55:37 +0300
Subject: [PATCH] Support ERR in remote archive like in fetch/push

Make ERR as first packet of remote snapshot reply work like it does in
fetch/push. Lets servers decline remote snapshot with message the same
way as declining fetch/push with a message.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 builtin/archive.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index 883c009..931956d 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,
 	if (strcmp(buf, "ACK")) {
 		if (len > 5 && !prefixcmp(buf, "NACK "))
 			die(_("git archive: NACK %s"), buf + 5);
+		if (len > 4 && !prefixcmp(buf, "ERR "))
+			die(_("remote error: %s"), buf + 4);
 		die(_("git archive: protocol error"));
 	}
 
-- 
1.7.7.3.g2791de.dirty


-Ilari

[PATCH] Support ERR in remote archive like in fetch/push

[Jonathan Nieder]

Ilari Liusvaara wrote:

> Oh, and adding interpretation of ERR packets to git archive is easy
> (and I even happen to have git:// server that can send those to
> test against):
>
> $ git archive --remote=git://localhost/foobar HEAD
> fatal: remote error: R access for foobar DENIED to anonymous
>
> (I also tested that remote snapshotting of repository that should be
> readable succeeds, it does).

Sounds like a good idea to me.  Let's see what René thinks; also
changing the subject line to attract other reviewers.

> --- >8 ----
> From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> Date: Mon, 3 Oct 2011 13:55:37 +0300
> Subject: [PATCH] Support ERR in remote archive like in fetch/push
> 
> Make ERR as first packet of remote snapshot reply work like it does in
> fetch/push. Lets servers decline remote snapshot with message the same
> way as declining fetch/push with a message.
> 
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> ---
>  builtin/archive.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin/archive.c b/builtin/archive.c
> index 883c009..931956d 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,
>  	if (strcmp(buf, "ACK")) {
>  		if (len > 5 && !prefixcmp(buf, "NACK "))
>  			die(_("git archive: NACK %s"), buf + 5);
> +		if (len > 4 && !prefixcmp(buf, "ERR "))
> +			die(_("remote error: %s"), buf + 4);
>  		die(_("git archive: protocol error"));
>  	}
>  
> -- 
> 1.7.7.3.g2791de.dirty
> 

Re: [PATCH] Support ERR in remote archive like in fetch/push

[René Scharfe]

Am 03.10.2011 13:26, schrieb Jonathan Nieder:
> Ilari Liusvaara wrote:
> 
>> Oh, and adding interpretation of ERR packets to git archive is easy
>> (and I even happen to have git:// server that can send those to
>> test against):
>>
>> $ git archive --remote=git://localhost/foobar HEAD
>> fatal: remote error: R access for foobar DENIED to anonymous
>>
>> (I also tested that remote snapshotting of repository that should be
>> readable succeeds, it does).
> 
> Sounds like a good idea to me.  Let's see what René thinks; also
> changing the subject line to attract other reviewers.

Looks good to me, but I'm not too familiar with the remote protocol.

>> --- >8 ----
>> From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
>> Date: Mon, 3 Oct 2011 13:55:37 +0300
>> Subject: [PATCH] Support ERR in remote archive like in fetch/push
>>
>> Make ERR as first packet of remote snapshot reply work like it does in
>> fetch/push. Lets servers decline remote snapshot with message the same
>> way as declining fetch/push with a message.
>>
>> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
>> ---
>>  builtin/archive.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/builtin/archive.c b/builtin/archive.c
>> index 883c009..931956d 100644
>> --- a/builtin/archive.c
>> +++ b/builtin/archive.c
>> @@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,
>>  	if (strcmp(buf, "ACK")) {
>>  		if (len > 5 && !prefixcmp(buf, "NACK "))
>>  			die(_("git archive: NACK %s"), buf + 5);
>> +		if (len > 4 && !prefixcmp(buf, "ERR "))
>> +			die(_("remote error: %s"), buf + 4);
>>  		die(_("git archive: protocol error"));
>>  	}
>>  
>> -- 
>> 1.7.7.3.g2791de.dirty
>>

Re: [PATCH] transport: do not allow to push over git:// protocol

[Nguyen Thai Ngoc Duy]

On Mon, Oct 03, 2011 at 02:01:59PM +0300, Ilari Liusvaara wrote:
> On Mon, Oct 03, 2011 at 03:42:51AM -0400, Jeff King wrote:
> > On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyen Thai Ngoc Duy wrote:
> > 
> > The real problem here seems to be that instead of communicating "no, we
> > don't support that", git-daemon just hangs up. It would be a much nicer
> > fix if we could change that. I'm not sure it's possible, though. There's
> > not much room in the beginning of the room to make that communication in
> > a way that's backwards compatible.
> 
> Oh, sure it is possible (except for remote snapshot):
> 
> $ /usr/bin/git fetch git://localhost/foobar
> fatal: remote error: R access for foobar DENIED to anonymous
> $ /usr/bin/git push git://localhost/foobar
> fatal: remote error: W access for foobar DENIED to anonymous
> $ /usr/bin/git archive --remote=git://localhost/foobar HEAD
> fatal: git archive: protocol error
> $ /usr/bin/git --version
> git version 1.7.6.3
> 
> Supported for fetch and push since 1.6.1-rc1 (And 1.6.1 was over
> 2.5 years ago). Oh, and even before that, but with slightly more
> ugly error message.
> 
> Oh, and adding interpretation of ERR packets to git archive is easy
> (and I even happen to have git:// server that can send those to
> test against):
> 
> $ git archive --remote=git://localhost/foobar HEAD
> fatal: remote error: R access for foobar DENIED to anonymous
> 
> (I also tested that remote snapshotting of repository that should be
> readable succeeds, it does).
> 
> --- >8 ----
> From ce3a402e4fa72cf603f92801d6f021ff89d3ac35 Mon Sep 17 00:00:00 2001
> From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> Date: Mon, 3 Oct 2011 13:55:37 +0300
> Subject: [PATCH] Support ERR in remote archive like in fetch/push
> 
> Make ERR as first packet of remote snapshot reply work like it does in
> fetch/push. Lets servers decline remote snapshot with message the same
> way as declining fetch/push with a message.
> 
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>

Yeah, maybe with this patch also?

-- 8< --
Subject: [PATCH] pack-protocol: document "ERR" line

Since a807328 (connect.c: add a way for git-daemon to pass an error
back to client), git client recognizes "ERR" line and prints a
friendly message to user if an error happens at server side.

Document this.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/technical/pack-protocol.txt |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index a7004c6..546980c 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -60,6 +60,13 @@ process on the server side over the Git protocol is this:
      "0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" |
      nc -v example.com 9418
 
+If the server refuses the request for some reasons, it could abort
+gracefully with an error message.
+
+----
+  error-line     =  PKT-LINE("ERR" SP explanation-text)
+----
+
 
 SSH Transport
 -------------
-- 
-- 8< --

Re: [PATCH] transport: do not allow to push over git:// protocol

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>> From ce3a402e4fa72cf603f92801d6f021ff89d3ac35 Mon Sep 17 00:00:00 2001
>> From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
>> Date: Mon, 3 Oct 2011 13:55:37 +0300
>> Subject: [PATCH] Support ERR in remote archive like in fetch/push
>> 
>> Make ERR as first packet of remote snapshot reply work like it does in
>> fetch/push. Lets servers decline remote snapshot with message the same
>> way as declining fetch/push with a message.
>> 
>> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
>
> Yeah, maybe with this patch also?
>
> -- 8< --
> Subject: [PATCH] pack-protocol: document "ERR" line
>
> Since a807328 (connect.c: add a way for git-daemon to pass an error
> back to client), git client recognizes "ERR" line and prints a
> friendly message to user if an error happens at server side.
>
> Document this.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Mmakes sense; thanks, both of you.