[PATCH 1/4] Refactor list of environment variables to be sanitized

[PATCH 1/4] Refactor list of environment variables to be sanitized

[Junio C Hamano]

When local process-to-process pipe transport spawns a subprocess,
it cleans up various git related variables to give the new process
a fresh environment.  The list of variables to cleanse is useful
in other places.
---
 cache.h   |    2 ++
 connect.c |   26 +++++++++++++++-----------
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index 189151d..b72434f 100644
--- a/cache.h
+++ b/cache.h
@@ -389,6 +389,8 @@ extern void set_git_work_tree(const char *tree);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
+extern const char *sanitize_git_env[];
+
 extern const char **get_pathspec(const char *prefix, const char **pathspec);
 extern void setup_work_tree(void);
 extern const char *setup_git_directory_gently(int *);
diff --git a/connect.c b/connect.c
index 2f23ab3..b4705ff 100644
--- a/connect.c
+++ b/connect.c
@@ -6,6 +6,20 @@
 #include "run-command.h"
 #include "remote.h"
 
+/*
+ * When spawning a subprocess in a fresh environment,
+ * these variables may need to be cleared
+ */
+const char *sanitize_git_env[] = {
+	ALTERNATE_DB_ENVIRONMENT,
+	DB_ENVIRONMENT,
+	GIT_DIR_ENVIRONMENT,
+	GIT_WORK_TREE_ENVIRONMENT,
+	GRAFT_ENVIRONMENT,
+	INDEX_ENVIRONMENT,
+	NULL
+};
+
 static char *server_capabilities;
 
 static int check_ref(const char *name, int len, unsigned int flags)
@@ -625,17 +639,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 		*arg++ = host;
 	}
 	else {
-		/* remove these from the environment */
-		const char *env[] = {
-			ALTERNATE_DB_ENVIRONMENT,
-			DB_ENVIRONMENT,
-			GIT_DIR_ENVIRONMENT,
-			GIT_WORK_TREE_ENVIRONMENT,
-			GRAFT_ENVIRONMENT,
-			INDEX_ENVIRONMENT,
-			NULL
-		};
-		conn->env = env;
+		conn->env = sanitize_git_env;
 		*arg++ = "sh";
 		*arg++ = "-c";
 	}
-- 
1.6.2.rc2.99.g9f3bb

[PATCH 2/4] git-init: inject some sanity to the option parser

[Junio C Hamano]

The parsing loop was a mess full of side effects.

This commit separates the loop that parses and understand the options
given, and the part that makes side effects based on given options.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-init-db.c |   35 ++++++++++++++++++++++++-----------
 1 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index ee3911f..9628803 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -377,27 +377,40 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	const char *git_dir;
 	const char *template_dir = NULL;
 	unsigned int flags = 0;
+	const char *shared_given = NULL;
+	int bare_given = 0;
 	int i;
 
-	for (i = 1; i < argc; i++, argv++) {
-		const char *arg = argv[1];
+	/* Parse without side effects that is hard to undo or unparse */
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
 		if (!prefixcmp(arg, "--template="))
-			template_dir = arg+11;
-		else if (!strcmp(arg, "--bare")) {
-			static char git_dir[PATH_MAX+1];
-			is_bare_repository_cfg = 1;
-			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
-						sizeof(git_dir)), 0);
-		} else if (!strcmp(arg, "--shared"))
-			init_shared_repository = PERM_GROUP;
+			template_dir = arg + 11;
+		else if (!strcmp(arg, "--bare"))
+			bare_given = 1;
+		else if (!strcmp(arg, "--shared"))
+			shared_given = "";
 		else if (!prefixcmp(arg, "--shared="))
-			init_shared_repository = git_config_perm("arg", arg+9);
+			shared_given = arg + 9;
 		else if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet"))
 			flags |= INIT_DB_QUIET;
 		else
 			usage(init_db_usage);
 	}
 
+	if (bare_given) {
+		static char git_dir[PATH_MAX+1];
+		is_bare_repository_cfg = 1;
+		setenv(GIT_DIR_ENVIRONMENT,
+		       getcwd(git_dir, sizeof(git_dir)), 0);
+	}
+
+	if (shared_given)
+		init_shared_repository =
+			((!*shared_given)
+			 ? PERM_GROUP
+			 : git_config_perm("arg", shared_given));
+
 	/*
 	 * GIT_WORK_TREE makes sense only in conjunction with GIT_DIR
 	 * without --bare.  Catch the error early.
-- 
1.6.2.rc2.99.g9f3bb

[PATCH 3/4] Add init-serve, the remote side of "git init --remote=host:path"

[Junio C Hamano]

This still has a few NEEDSWORK, but should be good enough to start
developing and testing the requesting side of the protocol pair.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile             |    1 +
 builtin-init-serve.c |  139 ++++++++++++++++++++++++++++++++++++++++++++++++++
 builtin.h            |    1 +
 git.c                |    1 +
 4 files changed, 142 insertions(+), 0 deletions(-)
 create mode 100644 builtin-init-serve.c

diff --git a/Makefile b/Makefile
index 0675c43..c0d0cfd 100644
--- a/Makefile
+++ b/Makefile
@@ -544,6 +544,7 @@ BUILTIN_OBJS += builtin-gc.o
 BUILTIN_OBJS += builtin-grep.o
 BUILTIN_OBJS += builtin-help.o
 BUILTIN_OBJS += builtin-init-db.o
+BUILTIN_OBJS += builtin-init-serve.o
 BUILTIN_OBJS += builtin-log.o
 BUILTIN_OBJS += builtin-ls-files.o
 BUILTIN_OBJS += builtin-ls-remote.o
diff --git a/builtin-init-serve.c b/builtin-init-serve.c
new file mode 100644
index 0000000..2a06631
--- /dev/null
+++ b/builtin-init-serve.c
@@ -0,0 +1,139 @@
+#include "cache.h"
+#include "builtin.h"
+#include "pkt-line.h"
+#include "run-command.h"
+#include "strbuf.h"
+
+/*
+ * Notice any command line argument that we may not want to invoke
+ * "git init" with when we are doing this remotely, and reject the
+ * request.
+ */
+static int forbidden_arg(const char *arg)
+{
+	if (!prefixcmp(arg, "--shared=") ||
+	    !strcmp(arg, "--shared") ||
+	    !strcmp(arg, "--bare"))
+		return 0;
+	return 1;
+}
+
+/*
+ * The other end gives the command line arguments to "git init" one by
+ * one over pkt-line protocol, and then expects a message back.
+ *
+ * We need to read them all, even when we know we will reject the
+ * request before responding.
+ */
+static int serve(const char *errmsg)
+{
+	int argc = 0;
+	const char *argv[64];
+
+	argv[argc++] = "git";
+	argv[argc++] = "init";
+	for (;;) {
+		static char err[1000];
+		char line[1000];
+		int len;
+
+		len = packet_read_line(0, line, sizeof(line));
+		if (!len)
+			break;
+		if (*errmsg)
+			continue; /* just read and discard */
+
+		if (line[len - 1] == '\n')
+			line[--len] = '\0';
+
+		if (!prefixcmp(line, "arg ")) {
+			const char *cmd = line + 4;
+			if (forbidden_arg(cmd)) {
+				snprintf(err, sizeof(err),
+					 "forbidden option to 'git init': '%s'",
+					 cmd);
+				errmsg = err;
+			} else if (argc + 1 < ARRAY_SIZE(argv))
+				argv[argc++] = xstrdup(cmd);
+			else
+				errmsg = "arg list too long";
+			continue;
+		}
+
+		/* Possible protocol extensions here... */
+		snprintf(err, sizeof(err),
+			 "unrecognized protocol extension: %s", line);
+		errmsg = err;
+
+		/* Do not break out of the loop here */
+	}
+
+	if (!*errmsg) {
+		struct child_process child;
+
+		argv[argc] = NULL;
+		memset(&child, 0, sizeof(child));
+		child.argv = argv;
+		child.env = sanitize_git_env;
+
+		/*
+		 * NEEDSWORK: I do not currently think it is worth it,
+		 * but this might want to set up and use the sideband
+		 * to capture and send output from the child back to
+		 * the requestor.  At least this comment needs to be removed
+		 * once we make the decision.
+		 */
+		child.stdout_to_stderr = 1;
+
+		/*
+		 * NEEDSWORK: we might want to distinguish various
+		 * error codes from run_command() and return different
+		 * messages back.  I am too lazy to be bothered.
+		 */
+		if (run_command(&child))
+			errmsg = "bad";
+	}
+
+	if (*errmsg)
+		packet_write(1, "ng init %s\n", errmsg);
+	else
+		packet_write(1, "ok init\n");
+
+	return !!*errmsg;
+}
+
+int cmd_init_serve(int argc, const char **argv, const char *prefix)
+{
+	const char *dir;
+	struct strbuf errmsg = STRBUF_INIT;
+	char here[PATH_MAX];
+	int must_rmdir = 0;
+
+	if (argc != 2)
+		return serve("usage: init /p/a/th");
+	dir = argv[1];
+
+	if (!getcwd(here, sizeof(here)))
+		return serve("init cannot determine the $cwd");
+
+	/*
+	 * Perhaps lift avoid_alias() from daemon.c and check
+	 * dir with it, as programs like gitosis may
+	 * want to restrict the arguments to this service.
+	 */
+	if (mkdir(dir, 0777))
+		strbuf_addf(&errmsg,
+			    "cannot mkdir('%s'): %s", dir, strerror(errno));
+	else {
+		must_rmdir = 1;
+		if (chdir(dir))
+			strbuf_addf(&errmsg,
+				    "cannot chdir('%s'): %s", dir, strerror(errno));
+	}
+	if (serve(errmsg.buf)) {
+		if (must_rmdir && !chdir(here))
+			rmdir(dir);
+		return 1;
+	}
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index 1495cf6..e9f9ffb 100644
--- a/builtin.h
+++ b/builtin.h
@@ -59,6 +59,7 @@ extern int cmd_grep(int argc, const char **argv, const char *prefix);
 extern int cmd_help(int argc, const char **argv, const char *prefix);
 extern int cmd_http_fetch(int argc, const char **argv, const char *prefix);
 extern int cmd_init_db(int argc, const char **argv, const char *prefix);
+extern int cmd_init_serve(int argc, const char **argv, const char *prefix);
 extern int cmd_log(int argc, const char **argv, const char *prefix);
 extern int cmd_log_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_ls_files(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index c2b181e..1df8584 100644
--- a/git.c
+++ b/git.c
@@ -311,6 +311,7 @@ static void handle_internal_command(int argc, const char **argv)
 #endif
 		{ "init", cmd_init_db },
 		{ "init-db", cmd_init_db },
+		{ "init-serve", cmd_init_serve },
 		{ "log", cmd_log, RUN_SETUP | USE_PAGER },
 		{ "ls-files", cmd_ls_files, RUN_SETUP },
 		{ "ls-tree", cmd_ls_tree, RUN_SETUP },
-- 
1.6.2.rc2.99.g9f3bb

[PATCH 4/4] "git init --remote=host:path"

[Junio C Hamano]

This implements the requesting side of the pair.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-init-db.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 t/t0001-init.sh   |   12 ++++++++++++
 2 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index 9628803..18754d7 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -6,6 +6,7 @@
 #include "cache.h"
 #include "builtin.h"
 #include "exec_cmd.h"
+#include "pkt-line.h"
 
 #ifndef DEFAULT_GIT_TEMPLATE_DIR
 #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
@@ -363,6 +364,37 @@ static int guess_repository_type(const char *git_dir)
 	return 1;
 }
 
+static int remote_init(const char *remote, const char *init_serve,
+		       int argc, const char **argv)
+{
+	struct child_process *child;
+	int fd[2], i, len, status;
+	char line[1000];
+
+	if (!init_serve)
+		init_serve = "git init-serve";
+	child = git_connect(fd, remote, init_serve, 0);
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (!prefixcmp(arg, "--remote=") ||
+		    !prefixcmp(arg, "--exec="))
+			continue;
+		packet_write(fd[1], "arg %s\n", argv[i]);
+	}
+	packet_flush(fd[1]);
+
+	status = 0;
+	len = packet_read_line(fd[0], line, sizeof(line));
+	if (len < 3 ||
+	    (memcmp(line, "ok ", 3) && memcmp(line, "ng ", 3)))
+		die("protocol error: %s\n", line);
+
+	if (line[0] != 'o')
+		status = error("%s", line);
+	status |= finish_connect(child);
+	return status;
+}
+
 static const char init_db_usage[] =
 "git init [-q | --quiet] [--bare] [--template=<template-directory>] [--shared[=<permissions>]]";
 
@@ -378,6 +410,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	const char *template_dir = NULL;
 	unsigned int flags = 0;
 	const char *shared_given = NULL;
+	const char *remote_given = NULL;
+	const char *exec_given = NULL;
 	int bare_given = 0;
 	int i;
 
@@ -394,10 +428,20 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			shared_given = arg + 9;
 		else if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet"))
 			flags |= INIT_DB_QUIET;
+		else if (!prefixcmp(arg, "--remote="))
+			remote_given = arg + 9;
+		else if (!prefixcmp(arg, "--exec="))
+			exec_given = arg + 7;
 		else
 			usage(init_db_usage);
 	}
 
+	if (remote_given || exec_given) {
+		if (!remote_given)
+			die("--exec= without --remote=?");
+		return remote_init(remote_given, exec_given, argc, argv);
+	}
+
 	if (bare_given) {
 		static char git_dir[PATH_MAX+1];
 		is_bare_repository_cfg = 1;
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 5ac0a27..6f47930 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -199,4 +199,16 @@ test_expect_success 'init honors global core.sharedRepository' '
 	x`git config -f shared-honor-global/.git/config core.sharedRepository`
 '
 
+test_expect_success 'init --remote' '
+	R="$(pwd)/test-of-remote" &&
+	test_must_fail git init --remote="$R" --bare --template=frotz &&
+	git init --remote="$R" --bare &&
+	test -d "$R/objects/pack" &&
+	test_must_fail git init --remote="$R" &&
+	rm -fr "$R" &&
+	test_must_fail git init --remote="$R" --exec="false is not git" &&
+	git init --remote="$R" --exec="$TEST_DIRECTORY/../git-init-serve" &&
+	test -d "$R/.git/objects/pack"
+'
+
 test_done
-- 
1.6.2.rc2.99.g9f3bb

Re: [PATCH 3/4] Add init-serve, the remote side of "git init --remote=host:path"

[Jeff King]

On Sat, Feb 28, 2009 at 04:03:41PM -0800, Junio C Hamano wrote:

> +/*
> + * Notice any command line argument that we may not want to invoke
> + * "git init" with when we are doing this remotely, and reject the
> + * request.
> + */
> +static int forbidden_arg(const char *arg)
> +{
> +	if (!prefixcmp(arg, "--shared=") ||
> +	    !strcmp(arg, "--shared") ||
> +	    !strcmp(arg, "--bare"))
> +		return 0;
> +	return 1;
> +}

I started this mail to complain that this function was "disallow known
bad" instead of "allow known good". But then after reading it carefully
three times, I see that it is in fact "not allow known good". Can we
make it "allowed_arg" to prevent double negation?

> +		/*
> +		 * NEEDSWORK: I do not currently think it is worth it,
> +		 * but this might want to set up and use the sideband
> +		 * to capture and send output from the child back to
> +		 * the requestor.  At least this comment needs to be removed
> +		 * once we make the decision.
> +		 */
> +		child.stdout_to_stderr = 1;

I guess there is a potential information leak to say "directory does not
exist" versus "permission denied". Stopping such leaks often ends up
creating more harm (in confused users who don't know why it failed) than
good, but I think the fetch protocol is intentionally quiet here.

...

Actually, I just checked. Over ssh, you get:

  $ git fetch host:/nonexistent
  fatal: '/foo': unable to chdir or not a git archive
  fatal: The remote end hung up unexpectedly

But over git://, you get:

  $ git fetch git://host/nonexistent
  fatal: The remote end hung up unexpectedly

which I think is just because ssh relays stderr but the git daemon does
not.

So we are leaking the information to people authenticated via ssh (who
still might not be trusted or have full shell access, but are more
likely to be), but not to the whole world.

> +		/*
> +		 * NEEDSWORK: we might want to distinguish various
> +		 * error codes from run_command() and return different
> +		 * messages back.  I am too lazy to be bothered.
> +		 */
> +		if (run_command(&child))
> +			errmsg = "bad";

I think this somewhat falls into the same category as above (though
perhaps the information is less interesting).

-Peff

Re: [PATCH 3/4] Add init-serve, the remote side of "git init --remote=host:path"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Feb 28, 2009 at 04:03:41PM -0800, Junio C Hamano wrote:
>
>> +/*
>> + * Notice any command line argument that we may not want to invoke
>> + * "git init" with when we are doing this remotely, and reject the
>> + * request.
>> + */
>> +static int forbidden_arg(const char *arg)
>> +{
>> +	if (!prefixcmp(arg, "--shared=") ||
>> +	    !strcmp(arg, "--shared") ||
>> +	    !strcmp(arg, "--bare"))
>> +		return 0;
>> +	return 1;
>> +}
>
> I started this mail to complain that this function was "disallow known
> bad" instead of "allow known good". But then after reading it carefully
> three times, I see that it is in fact "not allow known good". Can we
> make it "allowed_arg" to prevent double negation?

I originally had "arg_ok()", but the implementation checked that the
argument is not --template, which is the only known to be bad one at the
moment, so that we would allow things by default.  Later I switched both
name and the implementation ;-)

I think renaming it to affirmative form (and swapping the return value, of
course) would be a good idea.

One issue I did not describe in the message was to what extent we would
want to allow operations other than the creating of a new repository
itself.

"Other than the creation" includes things like these:

 - "chgrp project-group" and "chmod g+s".

   It is typical for the system administrator to set up two classes of
   groups in /etc/groups file.  One group per user that is the primary
   group for a user, so that $HOME/ can be owned by that primary group and
   its permission bits set to 2775, and one group per project, so a user
   can belong to groups of the projects he works on.

   With the patch series:

    $ git init --remote=central.xz:/pub/projects/mine.git --shared --bare

   would take care of the "shared" (i.e. g+w) part, but the "mine.git"
   repository will either inherit the group ownership from /pub/projects
   (if /pub/projects is marked with g+s) or owned by the creating user's
   primary group, which would not be usable for use by a project the user
   belongs to.  IOW, --shared by itself is not very useful with its
   current form.

 - Writing to .git/description

   Primarily for use with gitweb; the need for this goes without saying.
   This shouldn't have security implications (even if the current
   implementation had one, it could easily be squashed).

 - Other administrative bits that have security implications:

   In a friendly environment without security concerns, e.g. company
   intranet setting, the user may want to do these things:

   - Installing custom hooks
   - Updating .git/config

   But these should never be allowed in a hosting-site setting.

My current thinking is that "init --remote" should not cater to the third
kind.  In a friendly environment the user would have a shell access, and
if the system does not give a shell access, then the user should not
allowed to muck freely with the repository.

I think the "chgrp/chmod g+s" part is necessary, and I envisioned that it
would be done by a new option to "git init", but I haven't thought things
through.  The options to "init" will not be visible to git-shell because
they are carried over pkt-line protocol as the payload, and programs like
gitosis may have hard time implementing limited access to certain groups,
even if they wanted to; I do not think gitosis would care, as it does not
rely on the UNIX group permission model for its access control, but other
implementations may care.

Re: [PATCH 3/4] Add init-serve, the remote side of "git init --remote=host:path"

[Jeff King]

On Sat, Feb 28, 2009 at 09:54:56PM -0800, Junio C Hamano wrote:

> One issue I did not describe in the message was to what extent we would
> want to allow operations other than the creating of a new repository
> itself.
> 
> "Other than the creation" includes things like these:

Hmph. I am not too excited by this list. What is the advantage of doing
them over the git protocol versus some out-of-band method?

It seems to me there are two main cases for dealing with a remote in
this way:

  1. You have shell access and a uid on the remote, but it is
     inconvenient to ssh across, find the repo (which may already be
     known locally by remote.*.url config), and then execute some
     commands.

     In this scenario, there really are no security concerns; you could
     have logged in and done all of this anyway. So it seems like a more
     natural fit would be a _client_ program that figures out what shell
     snippet you want to execute, connects to the remote, and does it.

  2. Your access on the remote is very restricted, you may not have your
     own uid, and hooks may be enforcing arbitrary security policy.

     In this case, even something as simple as chgrp seems like it could
     be a problem, depending on the remote's setup (e.g., because all
     users connect as one uid, but group permissions are somehow
     meaningful to the system; this implies that connecting users should
     not be able to arbitrarily chgrp their repos, even if chgrp itself
     allows it).

     Furthermore, in the case of many providers (e.g., github,
     repo.or.cz), there is already a separate out-of-band interface for
     doing "meta" stuff, like managing user accounts and repos. Isn't it
     more natural for them to integrate these features with their
     existing interfaces?

But let's say that there really is some value in setting up this
channel (because we want a uniform way of doing these things so we can
add more automated tool support from the client side). Then I think it
makes sense to look at what the people in (2) above are doing already.
That is, what sorts of things can you already do (and not do) in
github's or repo.or.cz's interface? On top of that, it probably makes
sense to ask them if they are interested in such a feature, as they
would be primary users. And if they are, what do they want out of it?

-Peff

Re: [PATCH 3/4] Add init-serve, the remote side of "git init --remote=host:path"

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> On Sat, Feb 28, 2009 at 09:54:56PM -0800, Junio C Hamano wrote:
> 
> > One issue I did not describe in the message was to what extent we would
> > want to allow operations other than the creating of a new repository
> > itself.
> > 
> > "Other than the creation" includes things like these:
> 
> Hmph. I am not too excited by this list. What is the advantage of doing
> them over the git protocol versus some out-of-band method?

My feelings echo Peff's here.
 
>      Furthermore, in the case of many providers (e.g., github,
>      repo.or.cz), there is already a separate out-of-band interface for
>      doing "meta" stuff, like managing user accounts and repos. Isn't it
>      more natural for them to integrate these features with their
>      existing interfaces?

Don't forget about my day-job project, Gerrit Code Review.

I'm already hosting 138 projects on review.source.android.com, and
based on weekly downloads of Gerrit builds I'd estimate another ~60
deployments at internal servers within organizations.  (I'm doing
weekly "stable" builds and I'm seeing an average of ~70 downloads
for each build.)
 
> But let's say that there really is some value in setting up this
> channel (because we want a uniform way of doing these things so we can
> add more automated tool support from the client side). Then I think it
> makes sense to look at what the people in (2) above are doing already.
> That is, what sorts of things can you already do (and not do) in
> github's or repo.or.cz's interface? On top of that, it probably makes
> sense to ask them if they are interested in such a feature, as they
> would be primary users. And if they are, what do they want out of it?

Earlier last week a co-worker who is new to git complained that he
can't create a repository easily from his desktop.  There may be
value in being able to create a repository remotely, as it removes
that confusion.

But I think that's only true for situations where you are likely the
owner of the repository in your own home directory, such as ~you
on kernel.org.  For "hosted repositories" like any of the systems
you described above, there is a lot more to the creation than just
executing "git init" somewhere.

FWIW, JGit automatically creates a repository over the amazon-s3://
transport during push if it doesn't exist yet.  In my mind, this is
somewhat like scp or rsync automatically creating the destination
directory when recursively copying a directory.  For the usage of
creating a new repository in your own home directory on some remote
server, why isn't this just an option to git push?

  git push --create me@george:my/new/repo.git master

It fits better with the push-creates-a-branch feature.


Gitosis allows repositories to be created during the first push
into that repository, if the repository is listed in the config
and the user is permitted to write to it.

GitHub and repo.or.cz allow you to create empty repositories,
but both do so through the web interface.

Gerrit Code Review still requires you to manually run "git init"
and also do a SQL INSERT by hand.  Its missing some sort of user
friendly repository creation feature.  But I'm starting to consider
doing it automatically during "git push", like gitosis does.

Repository creation in Gerrit is more than just "git init" and
a SQL INSERT.  I also have to run rsync a few times to replicate
the repository onto multiple servers, so that subsequent pushes
to those servers won't fail.

I suspect the situation is the same for GitHub; I know they have
a custom daemon that handles repository mapping, and some database
behind that daemon to store those translations.

IMHO, _if_ we do support remote repository creation, it should match
branch creation UI better (meaning, be part of git push, not part of
git init), and it should be something that the hosted providers can
all hook into so we can keep our non-git state management current.

-- 
Shawn.

Subject: [PATCH] Push to create

[Junio C Hamano]

This teaches receive-pack to create a new directory as a bare repository
when a user tries to push into a directory that does not exist.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 "Shawn O. Pearce" <spearce@spearce.org> writes:

 > But I think that's only true for situations where you are likely the
 > owner of the repository in your own home directory, such as ~you
 > on kernel.org.  For "hosted repositories" like any of the systems
 > you described above, there is a lot more to the creation than just
 > executing "git init" somewhere.
 > .
 > For the usage of
 > creating a new repository in your own home directory on some remote
 > server, why isn't this just an option to git push?

 I am not at all convinced that the use case people would for whatever
 reason do not want to "ssh-in, create and then push from here" is limited
 to "your own playpen in your $HOME", but it certainly limits the
 complexity and the scope of the damage.

 builtin-receive-pack.c |   29 ++++++++++++++++++++++++++++-
 t/t5541-push-create.sh |   20 ++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletions(-)
 create mode 100755 t/t5541-push-create.sh

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 849f1fe..2f2831c 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -652,6 +652,33 @@ static void add_alternate_refs(void)
 	foreach_alt_odb(add_refs_from_alternate, NULL);
 }
 
+static char *create_new_repo(char *dir)
+{
+	struct child_process child;
+	const char *argv[20];
+	int argc;
+
+	if (mkdir(dir, 0777)) {
+		error("cannot mkdir '%s': %s", dir, strerror(errno));
+		return NULL;
+	}
+	argc = 0;
+	argv[argc++] = "init";
+	argv[argc++] = "--bare";
+	argv[argc++] = NULL;
+	child.argv = argv;
+	if (run_command_v_opt_cd_env(argv,
+				     RUN_COMMAND_NO_STDIN |
+				     RUN_COMMAND_STDOUT_TO_STDERR |
+				     RUN_GIT_CMD,
+				     dir,
+				     NULL)) {
+		error("cannot run git init");
+		return NULL;
+	}
+	return enter_repo(dir, 0);
+}
+
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -674,7 +701,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 
 	setup_path();
 
-	if (!enter_repo(dir, 0))
+	if (!enter_repo(dir, 0) && !create_new_repo(dir))
 		die("'%s': unable to chdir or not a git archive", dir);
 
 	if (is_repository_shallow())
diff --git a/t/t5541-push-create.sh b/t/t5541-push-create.sh
new file mode 100755
index 0000000..52558a5
--- /dev/null
+++ b/t/t5541-push-create.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+test_description='push into nonexisting repository'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_commit A &&
+	test_commit B &&
+	test_commit C
+'
+
+test_expect_success 'push into nonexisting repository' '
+	this=$(git rev-parse B) &&
+	git push "file://$(pwd)/not-here.git" B:refs/heads/master &&
+	that=$(GIT_DIR=not-here.git git rev-parse HEAD) &&
+	test "$this" = "$that"
+'
+
+test_done
-- 
1.6.2.rc2.123.gab4478

Re: Subject: [PATCH] Push to create

[Jay Soffian]

On Tue, Mar 3, 2009 at 1:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> +       if (mkdir(dir, 0777)) {
> +               error("cannot mkdir '%s': %s", dir, strerror(errno));
> +               return NULL;
> +       }

How about "cannot create repository; mkdir failed '%s': %s" ...

j.

Re: Subject: [PATCH] Push to create

[Jeff King]

On Mon, Mar 02, 2009 at 10:50:46PM -0800, Junio C Hamano wrote:

>  I am not at all convinced that the use case people would for whatever
>  reason do not want to "ssh-in, create and then push from here" is limited
>  to "your own playpen in your $HOME", but it certainly limits the
>  complexity and the scope of the damage.

If you are going to limit it in that way, wouldn't it be better to do it
entirely client-side? As in, "git push --create remote" will literally
do:

    ssh remote_host "mkdir -p remote_dir && cd remote_dir && git init --bare"

? Then you don't have to care about whether the remote side is recent
enough to support this, and there are no potential security issues; git
is merely saving you from typing the commands you could have done
yourself.

> +test_expect_success 'push into nonexisting repository' '
> +	this=$(git rev-parse B) &&
> +	git push "file://$(pwd)/not-here.git" B:refs/heads/master &&
> +	that=$(GIT_DIR=not-here.git git rev-parse HEAD) &&
> +	test "$this" = "$that"
> +'

I think a feature like this needs to be triggered manually via
"--create" or similar. Otherwise a typo on a regular push will be very
confusing as your pushes appear to go nowhere. Though I suppose most
regular pushes happen using a configured remote, in which case it is not
as much of an issue.

-Peff

Re: Subject: [PATCH] Push to create

[Jay Soffian]

On Tue, Mar 3, 2009 at 2:09 AM, Jeff King <peff@peff.net> wrote:
> If you are going to limit it in that way, wouldn't it be better to do it
> entirely client-side? As in, "git push --create remote" will literally
> do:
>
>    ssh remote_host "mkdir -p remote_dir && cd remote_dir && git init --bare"
>
> ? Then you don't have to care about whether the remote side is recent
> enough to support this, and there are no potential security issues; git
> is merely saving you from typing the commands you could have done
> yourself.

So I was curious how Mercurial implemented this, and it is similar to
what Junio coded. It runs "hg init" on the remote end via ssh. (hg
init <path> takes care of creating the directory; I just also noticed
you can do hg init ssh://host/path to create the repo remotely, which
is I guess sorta interesting.)

> I think a featur like this needs to be triggered manually via
> "--create" or similar. Otherwise a typo on a regular push will be very
> confusing as your pushes appear to go nowhere. Though I suppose most
> regular pushes happen using a configured remote, in which case it is not
> as much of an issue.

So I could've sworn Mercurial creates a remote repo for you on push,
but it turns out it does not:

$ hg push ssh://localhost/~/bar
remote: abort: There is no Mercurial repository here (.hg not found)!
abort: no suitable response from remote hg!

I concur w/Jeff and I think git probably should not as well. I think
that instead adding it to init might be interesting

"git init <arg>" where <arg> is local (and it creates the directory
and repo for you) or arg is ssh://... and it creates the dir and repo
over there for you.

And have "git push" abort with a friendlier message than:

fatal: '~/bar': unable to chdir or not a git archive
fatal: The remote end hung up unexpectedly

If that's a plumbing message maybe it can't change, unless push
swallows it and says something nicer like:

fatal: '~/bar' not a git repository. Create the destination repository
with "git init" before pushing.

We can be even friendlier than Mercurial. :-)

j.

Re: Subject: [PATCH] Push to create

[Jay Soffian]

On Tue, Mar 3, 2009 at 2:37 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> I concur w/Jeff and I think git probably should not as well.

Er, w/o being told to do so explicitly. I'd argue "--init" is a saner
option name than "--create" though since you say "git init", not "git
create" to make a repo.

j.

Re: Subject: [PATCH] Push to create

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If you are going to limit it in that way, wouldn't it be better to do it
> entirely client-side? As in, "git push --create remote" will literally
> do:
>
>     ssh remote_host "mkdir -p remote_dir && cd remote_dir && git init --bare"
>
> ? Then you don't have to care about whether the remote side is recent
> enough to support this, and there are no potential security issues; git
> is merely saving you from typing the commands you could have done
> yourself.

As with the previous "git init --remote" patch, my design constraints
includes keeping the door open for "git shell" users to optionally allow
this mode of operation.

Re: Subject: [PATCH] Push to create

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> I concur w/Jeff and I think git probably should not as well. I think
> that instead adding it to init might be interesting

The thing is Jeff and Shawn already rejected that route.

Re: Subject: [PATCH] Push to create

[Jay Soffian]

On Tue, Mar 3, 2009 at 2:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> I concur w/Jeff and I think git probably should not as well. I think
>> that instead adding it to init might be interesting
>
> The thing is Jeff and Shawn already rejected that route.

Okay. I missed that, so I'll go search for it, but it still seems like
"init [<path>|ssh://]" should be the basis for "push --init".

j.

Re: Subject: [PATCH] Push to create

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Tue, Mar 3, 2009 at 2:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jay Soffian <jaysoffian@gmail.com> writes:
>>
>>> I concur w/Jeff and I think git probably should not as well. I think
>>> that instead adding it to init might be interesting
>>
>> The thing is Jeff and Shawn already rejected that route.
>
> Okay. I missed that, so I'll go search for it, but it still seems like
> "init [<path>|ssh://]" should be the basis for "push --init".

It was called "init --remote".

Re: Subject: [PATCH] Push to create

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Tue, Mar 3, 2009 at 2:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jay Soffian <jaysoffian@gmail.com> writes:
>>
>>> I concur w/Jeff and I think git probably should not as well. I think
>>> that instead adding it to init might be interesting
>>
>> The thing is Jeff and Shawn already rejected that route.
>
> Okay. I missed that, so I'll go search for it, but it still seems like
> "init [<path>|ssh://]" should be the basis for "push --init".

It was called "init --remote".

No need to look for it; it is one of the message on the References line of
this message, iow, in *this* thread.

Re: Subject: [PATCH] Push to create

[Jeff King]

On Mon, Mar 02, 2009 at 11:55:51PM -0800, Junio C Hamano wrote:

> As with the previous "git init --remote" patch, my design constraints
> includes keeping the door open for "git shell" users to optionally allow
> this mode of operation.

OK, I thought your original comment was "I don't think this constraint
(thinking only of normal shell users) is right, but here is a patch
anyway". Which did leave me confused, since it seemed like your patch
did not cater just to such users. But I see now what you meant.

However, if you are thinking of "git shell" users, then is it not a
potential security problem to allow them to create new repositories
without the admin explicitly enabling it? If a site is depending on
hooks in existing repositories to implement some kind of policy, then
isn't this a way to bypass it (not to make changes in those existing
repos, obviously, but let's say there is a policy about how disk usage
is counted).

Even if it isn't a security issue, it might simply be broken. Shawn has
said that Gerrit needs extra magic when creating a repository, and I
wouldn't be surprised if github and repo.or.cz were the same. With your
patch, what switch should a Gerrit admin flip to prevent people from
creating broken repos?

What about places that might simply want to put some policy in place,
like kernel.org having all linux repos point to Linus as alternates?

-Peff

Re: Subject: [PATCH] Push to create

[Jay Soffian]

On Tue, Mar 3, 2009 at 3:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> It was called "init --remote".
>
> No need to look for it; it is one of the message on the References line of
> this message, iow, in *this* thread.

Ah, not in gmail's web interface. :-(

j.

Re: Subject: [PATCH] Push to create

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Mar 02, 2009 at 11:55:51PM -0800, Junio C Hamano wrote:
>
>> As with the previous "git init --remote" patch, my design constraints
>> includes keeping the door open for "git shell" users to optionally allow
>> this mode of operation.
>
> OK, I thought your original comment was "I don't think this constraint
> (thinking only of normal shell users) is right, but here is a patch
> anyway". Which did leave me confused, since it seemed like your patch
> did not cater just to such users. But I see now what you meant.

Yeah, I wanted to see if we can give git-shell only people a sane way to
host a group project, so that was why I mentioned "chgrp/chmod" in the
follow-up message to the "init --remote" series.

> However, if you are thinking of "git shell" users, then is it not a
> potential security problem to allow them to create new repositories
> without the admin explicitly enabling it? If a site is depending on
> hooks in existing repositories to implement some kind of policy, then
> isn't this a way to bypass it (not to make changes in those existing
> repos, obviously, but let's say there is a policy about how disk usage
> is counted).

Yes and no.  I think "git shell" sites fall broadly into two categories.
The ones arranged ala gitosis without per-user UNIX account, it certainly
is an issue.  The ones with per-user UNIX account would not let anywhere
other than $HOME written, so it is not.

My sole interest lies in building a track record of suggested patches to
eliminate the excuse people would use to complain that we do not attempt
to allow repositories to be created remotely.  I've shown two possible
ways.  It now is turn for those who want to have the feature to fill in
the details.  These are weatherbaloon patches, and it is not my itch to
scratch anyway.

> Even if it isn't a security issue, it might simply be broken. Shawn has
> said that Gerrit needs extra magic when creating a repository, and I
> wouldn't be surprised if github and repo.or.cz were the same. With your
> patch, what switch should a Gerrit admin flip to prevent people from
> creating broken repos?
>
> What about places that might simply want to put some policy in place,
> like kernel.org having all linux repos point to Linus as alternates?

These are all valid points and people who are interested in creating
repositories remotely must think about them when they finally decide to
scratch their own itch.  I am merely helping by showing where to add
hooks.

Re: Subject: [PATCH] Push to create

[Jeff King]

On Mon, Mar 02, 2009 at 11:56:57PM -0800, Junio C Hamano wrote:

> > I concur w/Jeff and I think git probably should not as well. I think
> > that instead adding it to init might be interesting
> 
> The thing is Jeff and Shawn already rejected that route.

Since when do you listen to me? ;P

I think there are actually two issues to be resolved, though:

  1. What is a standardized way for clients to ask for repo creation?

  2. How do users trigger that repo creation.

I think once you have (1), then (2) is easy. You can have "git init
host:dir", "git push --init remote", or whatever, and they can all
trigger the same mechanism. For systems which have an out-of-band
method, they can continue to behave as they have, or they can hook into
the standardized mechanism as if they were clients themselves. So there
is not that much point in debating (2), I think, until there is a
working (1).

Now (1) is much harder. Some parameters come from the user, but some
(including whether creation is allowed at all) must come from the site
administrator. And some site administrators will a hook to do whatever
site-specific magic they need.

What about the client just calling init-serve on the server as a program
which does whatever it wants to create a repo? The shipped default would
be:

  #!/bin/sh
  echo >&2 Sorry, repo creation not allowed.
  exit 1

Sites who want to give their users full creation access would do (and
obviously the --mkdir option would need to be added):

  #!/bin/sh
  exec git init --mkdir "$@"

Sites which want to restrict can do:

  #!/bin/sh
  for i in "$@"; do
    case "$i" in
    --bare) ;;
         *) echo >&2 Forbidden argument: $i; exit 1 ;;
    esac
  done
  exec git init --mkdir "$@"

Sites like GitHub or Gerrit can munge the arguments as appropriate. They
could even allow site-specific options if they wanted as long as they
were syntactically correct (i.e., "git init --gerrit-base=foo remote"
would pass the argument through to the remote unharmed).

-Peff

Re: Subject: [PATCH] Push to create

[Jeff King]

On Tue, Mar 03, 2009 at 12:22:53AM -0800, Junio C Hamano wrote:

> Yes and no.  I think "git shell" sites fall broadly into two categories.
> The ones arranged ala gitosis without per-user UNIX account, it certainly
> is an issue.  The ones with per-user UNIX account would not let anywhere
> other than $HOME written, so it is not.

Right. My problem is that for the former case, there is no switch.
People upgrading git would just get this new feature which has security
implications. So I think any patch needs to default to "off".

> My sole interest lies in building a track record of suggested patches to
> eliminate the excuse people would use to complain that we do not attempt
> to allow repositories to be created remotely.  I've shown two possible
> ways.  It now is turn for those who want to have the feature to fill in
> the details.  These are weatherbaloon patches, and it is not my itch to
> scratch anyway.

Well, that's sneaky of you. ;P

But I think that coincides with what I was trying to say in my original
response to the series, which is "this issue is complex, and we need to
hear from the people who would really want this exactly what it is they
want".

-Peff

Re: Subject: [PATCH] Push to create

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But I think that coincides with what I was trying to say in my original
> response to the series, which is "this issue is complex, and we need to
> hear from the people who would really want this exactly what it is they
> want".

And we haven't heard from them at all, unless you and/or Shawn are
interested.  After all we may not have to worry about this at all ;-)

And that answers your question (1) in the other message.  The standard way
for users to create a repository becomes:

	mkdir this-new-directory
        cd this-new-directory
        git init

Re: Subject: [PATCH] Push to create

[Jay Soffian]

On Tue, Mar 3, 2009 at 3:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> And we haven't heard from them at all

Well, not here anyway, they seem to just complain about it on their
blogs and git's reputation suffers in silence. :-(

(I don't know when the heck I started to care about git's reputation.)

j.

Re: Subject: [PATCH] Push to create

[Theodore Tso]

On Tue, Mar 03, 2009 at 12:30:46AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > But I think that coincides with what I was trying to say in my original
> > response to the series, which is "this issue is complex, and we need to
> > hear from the people who would really want this exactly what it is they
> > want".
> 
> And we haven't heard from them at all, unless you and/or Shawn are
> interested.  After all we may not have to worry about this at all ;-)

Junio, I assume you saw Scott James Remnant blog posts, "Git Sucks"?

       http://www.netsplit.com/2009/02/17/git-sucks/
       http://www.netsplit.com/2009/02/17/git-sucks-2/
       http://www.netsplit.com/2009/02/17/git-sucks-3/
       http://www.netsplit.com/2009/02/23/revision-control-systems-suck/

My commentary on his complaint is found here:

http://thunk.org/tytso/blog/2009/02/23/reflections-on-a-complaint-from-a-frustrated-git-user/

Some (but not all) of the comments on my blog are also worth reading.

						- Ted

Re: Subject: [PATCH] Push to create

[Johannes Schindelin]

Hi,

On Tue, 3 Mar 2009, Theodore Tso wrote:

> On Tue, Mar 03, 2009 at 12:30:46AM -0800, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> > 
> > > But I think that coincides with what I was trying to say in my 
> > > original response to the series, which is "this issue is complex, 
> > > and we need to hear from the people who would really want this 
> > > exactly what it is they want".
> > 
> > And we haven't heard from them at all, unless you and/or Shawn are 
> > interested.  After all we may not have to worry about this at all ;-)
> 
> Junio, I assume you saw Scott James Remnant blog posts, "Git Sucks"?
> 
>        http://www.netsplit.com/2009/02/17/git-sucks/
>        http://www.netsplit.com/2009/02/17/git-sucks-2/
>        http://www.netsplit.com/2009/02/17/git-sucks-3/
>        http://www.netsplit.com/2009/02/23/revision-control-systems-suck/

I have to admit that I only skimmed the first two: happily, this guy just 
wanted to vent, and chose the correct forum.  His personal blog.

Because he would have had to do something completely different if he 
wanted to change things.  He would have had to:

- write in the public (i.e. this mailing list),

- guard his language much more,

- actually come up with something useful, constructive instead of 
  repeating several times that Git is hard to use,

- defend that the most important purpose of a revision control system is 
  the initial publication of a branch, as opposed to controlling 
  revisions.

It reminds me very much of the question: "Does a universe exist when there 
is nobody in it to observe it?"

Only in this case, I would rephrase it to: "Is a usability wart really 
serious when the guy does not even bother to report it where it can be 
fixed?"

Needless to say, I consider the answer to the latter to be "No.  Really, 
no."

I mean, it is easy, really, really easy to say that something sucks.  It 
is so easy that nobody should even pay attention.  Certainly so easy that 
I should not even have bothered writing this mail.

It is much harder work to come up with solutions, and that is what I am 
interested in.  So I'll try very hard to ignore everything else.

Ciao,
Dscho

Re: Subject: [PATCH] Push to create

[Shawn O. Pearce]

Theodore Tso <tytso@mit.edu> wrote:
> On Tue, Mar 03, 2009 at 12:30:46AM -0800, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> > 
> > > But I think that coincides with what I was trying to say in my original
> > > response to the series, which is "this issue is complex, and we need to
> > > hear from the people who would really want this exactly what it is they
> > > want".
> > 
> > And we haven't heard from them at all, unless you and/or Shawn are
> > interested.  After all we may not have to worry about this at all ;-)
> 
> Junio, I assume you saw Scott James Remnant blog posts, "Git Sucks"?
> 
>        http://www.netsplit.com/2009/02/17/git-sucks/

Funny, this very blog post is talking about why I think remote
creation should be under git push, not "git init --remote".

IMHO, maybe we also should change the error message that receive-pack
produces when the path its given isn't a Git repository.  Its really
not very human friendly to say "unable to chdir or not a git archive".
Hell, we don't even call them archives, we call them repositories.

-- 
Shawn.

Re: Subject: [PATCH] Push to create

[Jay Soffian]

On Tue, Mar 3, 2009 at 3:23 AM, Jeff King <peff@peff.net> wrote:
>
> What about the client just calling init-serve on the server as a program
> which does whatever it wants to create a repo? The shipped default would
> be:
>
>  #!/bin/sh
>  echo >&2 Sorry, repo creation not allowed.
>  exit 1
>
> Sites who want to give their users full creation access would do (and
> obviously the --mkdir option would need to be added):
>
>  #!/bin/sh
>  exec git init --mkdir "$@"
>
> Sites which want to restrict can do:
>
>  #!/bin/sh
>  for i in "$@"; do
>    case "$i" in
>    --bare) ;;
>         *) echo >&2 Forbidden argument: $i; exit 1 ;;
>    esac
>  done
>  exec git init --mkdir "$@"
>
> Sites like GitHub or Gerrit can munge the arguments as appropriate. They
> could even allow site-specific options if they wanted as long as they
> were syntactically correct (i.e., "git init --gerrit-base=foo remote"
> would pass the argument through to the remote unharmed).

FWIW, I like this proposal. The only issue I have which I think simply
cannot be reconciled is this: it doesn't do anything to help a user
that expects "git push --init ssh://..." to "just work". (And here I'm
assuming push --init just calls init --remote under the covers.) The
reason is that such a user is probably just using the git supplied by
their vendor, and thus remote creation is likely disabled by default.
The best I can come up with is a different error message:

"Sorry, remote repo creation not allowed. To enable it, blah blah blah"

So at least the user has a clue that git can help them here, but there
are security reasons it does not do so by default.

j.

Re: Subject: [PATCH] Push to create

[Daniel Barkalow]

On Mon, 2 Mar 2009, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > If you are going to limit it in that way, wouldn't it be better to do it
> > entirely client-side? As in, "git push --create remote" will literally
> > do:
> >
> >     ssh remote_host "mkdir -p remote_dir && cd remote_dir && git init --bare"
> >
> > ? Then you don't have to care about whether the remote side is recent
> > enough to support this, and there are no potential security issues; git
> > is merely saving you from typing the commands you could have done
> > yourself.
> 
> As with the previous "git init --remote" patch, my design constraints
> includes keeping the door open for "git shell" users to optionally allow
> this mode of operation.

One possibility would be to allow "git init" to create the directory (and 
its parents) if it is able. Then the command is "ssh remote_host 
GIT_DIR=remote_dir git init --bare". The "git shell" users can't use it, 
but only because "git shell" won't run "git init". But there's no reason 
it couldn't be configured (per-site or per-user) to allow it. Also, 
git-init could run the template's "pre-init" hook to do whatever it is 
that needs to be done for a new repository.

	-Daniel
*This .sig left intentionally blank*

Re: Subject: [PATCH] Push to create

[Jeff King]

On Tue, Mar 03, 2009 at 02:57:15PM -0500, Jay Soffian wrote:

> FWIW, I like this proposal. The only issue I have which I think simply
> cannot be reconciled is this: it doesn't do anything to help a user
> that expects "git push --init ssh://..." to "just work". (And here I'm
> assuming push --init just calls init --remote under the covers.) The
> reason is that such a user is probably just using the git supplied by
> their vendor, and thus remote creation is likely disabled by default.
> The best I can come up with is a different error message:
> 
> "Sorry, remote repo creation not allowed. To enable it, blah blah blah"
> 
> So at least the user has a clue that git can help them here, but there
> are security reasons it does not do so by default.

Yeah, the error messages should be more descriptive. Sites that have a
web interface can even do:

  Sorry, remote repo creation must be down through the web interface.
  Please go to http://host/$user/create/$repo.

where the URL can be customized based on the user and repo that made the
request.

Now we just need somebody who cares enough about this feature to work on
it. ;)

-Peff

Re: Subject: [PATCH] Push to create

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Now we just need somebody who cares enough about this feature to work on
> it. ;)

We actually do not need anything.  But somebody who cares enough might
;-).

[RFC/PATCH 1/2] improve missing repository error message

[Jeff King]

Certain remote commands, when asked to do something in a
particular directory that was not actually a git repository,
would say "unable to chdir or not a git archive". The
"chdir" bit is an unnecessary detail, and the term "git
archive" is much less common these days than "git repository".

So let's switch them all to:

  fatal: '%s' does not appear to be a git repository

Signed-off-by: Jeff King <peff@peff.net>
---
On Tue, Mar 03, 2009 at 10:41:06AM -0800, Shawn O. Pearce wrote:

> IMHO, maybe we also should change the error message that receive-pack
> produces when the path its given isn't a Git repository.  Its really
> not very human friendly to say "unable to chdir or not a git archive".
> Hell, we don't even call them archives, we call them repositories.

I agree completely.

Perhaps this should match the local "Not a git repository: %s". I prefer
this text, but maybe there is value in consistency.

 builtin-receive-pack.c   |    2 +-
 builtin-upload-archive.c |    2 +-
 daemon.c                 |    2 +-
 upload-pack.c            |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 849f1fe..a970b39 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -675,7 +675,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	setup_path();
 
 	if (!enter_repo(dir, 0))
-		die("'%s': unable to chdir or not a git archive", dir);
+		die("'%s' does not appear to be a git repository", dir);
 
 	if (is_repository_shallow())
 		die("attempt to push into a shallow repository");
diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index a9b02fa..0206b41 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -35,7 +35,7 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)
 	strcpy(buf, argv[1]); /* enter-repo smudges its argument */
 
 	if (!enter_repo(buf, 0))
-		die("not a git archive");
+		die("'%s' does not appear to be a git repository", buf);
 
 	/* put received options in sent_argv[] */
 	sent_argc = 1;
diff --git a/daemon.c b/daemon.c
index d93cf96..13401f1 100644
--- a/daemon.c
+++ b/daemon.c
@@ -229,7 +229,7 @@ static char *path_ok(char *directory)
 	}
 
 	if (!path) {
-		logerror("'%s': unable to chdir or not a git archive", dir);
+		logerror("'%s' does not appear to be a git repository", dir);
 		return NULL;
 	}
 
diff --git a/upload-pack.c b/upload-pack.c
index 19c24db..e15ebdc 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -645,7 +645,7 @@ int main(int argc, char **argv)
 	dir = argv[i];
 
 	if (!enter_repo(dir, strict))
-		die("'%s': unable to chdir or not a git archive", dir);
+		die("'%s' does not appear to be a git repository", dir);
 	if (is_repository_shallow())
 		die("attempt to fetch/clone from a shallow repository");
 	if (getenv("GIT_DEBUG_SEND_PACK"))
-- 
1.6.2.rc2.24.g55bc2

[RFC/PATCH 2/2] make remote hangup warnings more friendly

[Jeff King]

When a user asks to do a remote operation but the remote
side thinks the operation is invalid (e.g., because the user
pointed to a directory which is not actually a repository),
then we generally end up showing the user two fatal
messages: one from the remote explaining what went wrong, and
one from the local side complaining of an unexpected hangup.

For example:

  $ git push /nonexistent master
  fatal: '/nonexistent' does not appear to be a git repository
  fatal: The remote end hung up unexpectedly

In this case, the second message is useless noise, and the
user is better off seeing just the first.

This patch tries to suppress the "hung up" message when it
is redundant (but still exits with an error code, of
course).

One heuristic would be to suppress it whenever the remote
has said something on stderr. Unfortunately, in many
transports we don't actually handle stderr ourselves; it
is simply a clone of the parent program's stderr and goes
straight to the terminal.

Instead, we note that the remote end will typically perform
such an "expected" hangup at the beginning of a packet
instead of in the middle. Therefore if we are expecting a
packet and get an end-of-file from the remote, we assume
they have printed something useful and exit without further
messages. Any other short read or eof is reported as before.

Signed-off-by: Jeff King <peff@peff.net>
---
There are two possible problems with this patch:

  1. The "beginning of packet" heuristic may not be the best. I
     tried a few obvious test cases like pushing and fetching with
     invalid repos and bogus --receive-pack= settings. All of them have
     useful output from the remote. You would of course get no message
     if the remote was cut off randomly at just the right spot.

    The "remote said something on stderr" heuristic does seem better.
    But we would have to start processing stderr for local and ssh
    connections, which we don't do currently. On the other hand, that
    might be nicer in the long run, because you can mark the remote
    errors as remote, which makes it more obvious what is going on.
    E.g.,:

      $ git push host:bogus master
      remote: fatal: 'bogus' does not appear to be a git repository

  2. Even "remote said something on stderr" may not be a desirable
     heuristic. In the case of a bogus --receive-pack, you get:

       $ git push --receive-pack=bogus host:repo master
       sh: bogus: command not found

     So you are losing the part where git says "fatal: ". Maybe it
     is obvious that such an error is fatal. It is to me, but I am not
     the intended audience.

     I think this would be improved somewhat with stderr processing to:

       remote: sh: bogus: command not found

     Or you could even trigger the suppression only if stderr had a line
     matching "^fatal:".

     Or it may even be that adding "remote:" is enough to make things
     less confusing:

       remote: fatal: 'bogus' does not appear to be a git repository
       fatal: The remote end hung up unexpectedly

     I think I still favor suppression in that case, but it is at least
     more clear why there are two fatals.

So you can see, the possibilities are endless. The perfect bikeshed. ;)

 pkt-line.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index f5d0086..f2b387c 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -63,13 +63,16 @@ void packet_write(int fd, const char *fmt, ...)
 	safe_write(fd, buffer, n);
 }
 
-static void safe_read(int fd, void *buffer, unsigned size)
+static void safe_read(int fd, void *buffer, unsigned size, int eof_warn)
 {
 	ssize_t ret = read_in_full(fd, buffer, size);
 	if (ret < 0)
 		die("read error (%s)", strerror(errno));
-	else if (ret < size)
+	else if (ret < size) {
+		if (ret == 0 && !eof_warn)
+			exit(128);
 		die("The remote end hung up unexpectedly");
+	}
 }
 
 int packet_read_line(int fd, char *buffer, unsigned size)
@@ -78,7 +81,7 @@ int packet_read_line(int fd, char *buffer, unsigned size)
 	unsigned len;
 	char linelen[4];
 
-	safe_read(fd, linelen, 4);
+	safe_read(fd, linelen, 4, 0);
 
 	len = 0;
 	for (n = 0; n < 4; n++) {
@@ -103,7 +106,7 @@ int packet_read_line(int fd, char *buffer, unsigned size)
 	len -= 4;
 	if (len >= size)
 		die("protocol error: bad line length %d", len);
-	safe_read(fd, buffer, len);
+	safe_read(fd, buffer, len, 1);
 	buffer[len] = 0;
 	return len;
 }
-- 
1.6.2.rc2.24.g55bc2

Re: [RFC/PATCH 1/2] improve missing repository error message

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> +		die("'%s' does not appear to be a git repository", dir);

It may be sensible to distinguish the case where dir exists as a
directory but isn't a repository, and the case where it does not exist
at all, like "directory exists but does not appear to be a git
repository" Vs "no such directory".

Just in case someone does a "mkdir" on the server and doesn't know he
has to "git init" there too.

My 2 cents,

-- 
Matthieu

Re: [RFC/PATCH 1/2] improve missing repository error message

[Jeff King]

On Wed, Mar 04, 2009 at 10:19:25AM +0100, Matthieu Moy wrote:

> > +		die("'%s' does not appear to be a git repository", dir);
> 
> It may be sensible to distinguish the case where dir exists as a
> directory but isn't a repository, and the case where it does not exist
> at all, like "directory exists but does not appear to be a git
> repository" Vs "no such directory".
> 
> Just in case someone does a "mkdir" on the server and doesn't know he
> has to "git init" there too.

I agree it would be nice to be more descriptive; however, it's not quite
as simple as checking if the chdir failed. For "non-strict" repo lookup,
we check a number of variants. For $foo, we check:

  $foo.git/.git
  $foo/.git
  $foo.git
  $foo

If $foo exists but isn't a git directory, but $foo/.git does not exist,
then what is the correct response?  I guess we can say "no such
directory" only if $foo and $foo.git don't exist, and "not a git
repository" for the others.

I also don't know if we care about information leakage; with such a
patch can I now start probing the existence of arbitrary directories via
git-daemon (I haven't checked to see if there are other path
restrictions in place)?

-Peff

Re: Subject: [PATCH] Push to create

[Jay Soffian]

On Wed, Mar 4, 2009 at 12:42 AM, Jeff King <peff@peff.net> wrote:
> Now we just need somebody who cares enough about this feature to work on
> it. ;)

I'll see what I can do with the patches Junio sent.

(See Junio, I care enough.) :-)

j.

Re: Subject: [PATCH] Push to create

[Theodore Tso]

On Tue, Mar 03, 2009 at 11:39:16AM +0100, Johannes Schindelin wrote:
> Only in this case, I would rephrase it to: "Is a usability wart really 
> serious when the guy does not even bother to report it where it can be 
> fixed?"
> 
> It is much harder work to come up with solutions, and that is what I am 
> interested in.  So I'll try very hard to ignore everything else.

Well, I think there is a point hiding behind all of his whining, which
is that if you aren't joining a big established project, but are
rather the maintainer of an existing project that wants to move over
to git, or someone who is starting a new project, and decides to use
git because they've heard it's all the rage --- but the tutorials and
the documentation don't do that great of a job pointing people to the
right place.

For example, the git tutorial's "Using got for collaboration" assumes
that Alice and Bob have accounts on the same system.  C'mon!
Time-sharing systems are so.... 1970's.  Granted that arranging SSH
access is non-trivial, but there are plenty of web sites that offer
free git hosting services: repo.or.oz, gitorious, Github, and now
Sourceforge.  The tutorial should at least point people at the git
hosting sites.

One of the advantages that bzr has is that it is integrated fairly
tightly with Launchpad, which has its own bzr hosting service.  So the
documentation is easier, and with Launchpad's integration into bzr's
UI, a bzr user can publish a branch in a single command line (assuming
they are a registered launchpad user and have an account on launchpad):

	bzr push lp:~userid/project-name/branch-name

It's going to be hard for git to be able to match this usability,
given that we need to do something that works with multiple hosting
facilities, and not just tie ourselves to a single system like
gitorious, Github, or Sourceforge.  (Although I could imagine some
kind of plugin/hook system where once the plugin for a particular git
hosting facility was installed, git could be much more tightly
integrated with a particular hosting service.)

At the very least, though, it should be relatively easy to update our
documentation to at least acknowledge the many and varied free git
hosting services; it's not like we need to tell people that the only
way to share repositories involves setting up an ssh or Apache server.

					- Ted


P.S.  For more details about the bzr/Launchpad integration, see:

http://doc.bazaar-vcs.org/bzr.dev/en/tutorials/using_bazaar_with_launchpad.html

Re: [RFC/PATCH 1/2] improve missing repository error message

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> Certain remote commands, when asked to do something in a
> particular directory that was not actually a git repository,
> would say "unable to chdir or not a git archive". The
> "chdir" bit is an unnecessary detail, and the term "git
> archive" is much less common these days than "git repository".
> 
> So let's switch them all to:
> 
>   fatal: '%s' does not appear to be a git repository
> 
> Signed-off-by: Jeff King <peff@peff.net>

Acked-by: Shawn O. Pearce <spearce@spearce.org>

> On Tue, Mar 03, 2009 at 10:41:06AM -0800, Shawn O. Pearce wrote:
> 
> > IMHO, maybe we also should change the error message that receive-pack
> > produces when the path its given isn't a Git repository.  Its really
> > not very human friendly to say "unable to chdir or not a git archive".
> > Hell, we don't even call them archives, we call them repositories.

I really mean to write this patch myself, I haven't done much for
git lately.  But I got sidetracked yesterday and forgot.  Thank you
for doing it for me.

> Perhaps this should match the local "Not a git repository: %s". I prefer
> this text, but maybe there is value in consistency.

FWIW I also prefer the text you used in the patch...

-- 
Shawn.

Re: [RFC/PATCH 2/2] make remote hangup warnings more friendly

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> When a user asks to do a remote operation but the remote
> side thinks the operation is invalid (e.g., because the user
> pointed to a directory which is not actually a repository),
> then we generally end up showing the user two fatal
> messages: one from the remote explaining what went wrong, and
> one from the local side complaining of an unexpected hangup.
> 
> For example:
> 
>   $ git push /nonexistent master
>   fatal: '/nonexistent' does not appear to be a git repository
>   fatal: The remote end hung up unexpectedly
 
> In this case, the second message is useless noise, and the
> user is better off seeing just the first.
> 
> This patch tries to suppress the "hung up" message when it
> is redundant (but still exits with an error code, of
> course).

Hmm.  It would be nice to clean up these messages, but I
think the better way to do it is...
 
>      I think this would be improved somewhat with stderr processing to:
> 
>        remote: sh: bogus: command not found

... because then we can have positive proof that the remote said
something to the user, and we tagged it as being from the remote
side, just like we do with progress data in sideband, and then the
user can work from that information.  Any local side errors are
very likely caused by the remote errors, so we shouldn't bother
displaying them.

But its a lot more invasive of a patch to setup stderr processing
for pipe/SSH.

Actually, the remote stderr processing is desired for more than
just the bad path case.  Its good for when the remote aborts with
a write error from a write_or_die(), at least we know it was on
the remote side and not the local.  Its good for when the remote
shell says a "git-upload-pack: command not found".  Its good for
when the remote hook prints output, the client knows it came from
the remote side and not something local, so its messages should be
read in the context of the remote side.  Etc.
 
-- 
Shawn.

Re: [RFC/PATCH 1/2] improve missing repository error message

[Jeff King]

On Wed, Mar 04, 2009 at 10:57:42AM -0800, Shawn O. Pearce wrote:

> > > IMHO, maybe we also should change the error message that receive-pack
> > > produces when the path its given isn't a Git repository.  Its really
> > > not very human friendly to say "unable to chdir or not a git archive".
> > > Hell, we don't even call them archives, we call them repositories.
> 
> I really mean to write this patch myself, I haven't done much for
> git lately.  But I got sidetracked yesterday and forgot.  Thank you
> for doing it for me.

I think you are OK coasting on past glory for a while longer:

  $ git shortlog -ns | egrep -im2 'jeff|shawn'
  1313  Shawn O. Pearce
   305  Jeff King

:)

> > Perhaps this should match the local "Not a git repository: %s". I prefer
> > this text, but maybe there is value in consistency.
> 
> FWIW I also prefer the text you used in the patch...

I don't know if it is worth changing the _local_ one to match this,
then. I seem to recall some discussion about that message recently.
Personally I find the "Not a git repository (or any of the parent
directories)" wording to be quite awkward.

-Peff

Re: [RFC/PATCH 2/2] make remote hangup warnings more friendly

[Jeff King]

On Wed, Mar 04, 2009 at 11:04:52AM -0800, Shawn O. Pearce wrote:

> Hmm.  It would be nice to clean up these messages, but I
> think the better way to do it is...
>  
> >      I think this would be improved somewhat with stderr processing to:
> > 
> >        remote: sh: bogus: command not found
> 
> ... because then we can have positive proof that the remote said
> something to the user, and we tagged it as being from the remote
> side, just like we do with progress data in sideband, and then the
> user can work from that information.  Any local side errors are
> very likely caused by the remote errors, so we shouldn't bother
> displaying them.

OK. I was hoping to avoid that because it's more work, but I think it is
a better path in the long run. I'll add it to do my todo list. Unless
you want to redeem yourself by working on it first. ;)

-Peff

Re: Subject: [PATCH] Push to create

[Miles Bader]

Theodore Tso <tytso@mit.edu> writes:
> One of the advantages that bzr has is that it is integrated fairly
> tightly with Launchpad

To be honest, that seems vaguely creepy...

-Miles

-- 
Friendship, n. A ship big enough to carry two in fair weather, but only one
in foul.