[PATCH 2/4] Replace "GIT_DIR" with GIT_DIR_ENVIRONMENT.

[PATCH 2/4] Replace "GIT_DIR" with GIT_DIR_ENVIRONMENT.

[Shawn O. Pearce]

We tend to use the nice constant GIT_DIR_ENVIRONMENT when we
are referring to the "GIT_DIR" constant, but git.c didn't do
so.  Now it does.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 git.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index ec897dc..c82ca45 100644
--- a/git.c
+++ b/git.c
@@ -63,14 +63,14 @@ static int handle_options(const char*** argv, int* argc)
 				fprintf(stderr, "No directory given for --git-dir.\n" );
 				usage(git_usage_string);
 			}
-			setenv("GIT_DIR", (*argv)[1], 1);
+			setenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);
 			(*argv)++;
 			(*argc)--;
 		} else if (!strncmp(cmd, "--git-dir=", 10)) {
-			setenv("GIT_DIR", cmd + 10, 1);
+			setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);
 		} else if (!strcmp(cmd, "--bare")) {
 			static char git_dir[PATH_MAX+1];
-			setenv("GIT_DIR", getcwd(git_dir, sizeof(git_dir)), 1);
+			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 1);
 		} else {
 			fprintf(stderr, "Unknown option: %s\n", cmd);
 			usage(git_usage_string);
-- 
1.5.0.rc0.g6bb1

[PATCH 3/4] Automatically detect a bare git repository.

[Shawn O. Pearce]

Many users find it unfriendly that they can create a bare git
repository easily with `git clone --bare` but are then unable to
run simple commands like `git log` once they cd into that newly
created bare repository.  This occurs because we do not check to
see if the current working directory is a git repository.

Instead of failing out with "fatal: Not a git repository" we should
try to automatically detect if the current working directory is
a bare repository and use that for GIT_DIR, and fail out only if
that doesn't appear to be true.

We test the current working directory only after we have tried
searching up the directory tree.  This is to retain backwards
compatibility with our previous behavior on the off chance that
a user has a 'refs' and 'objects' subdirectories and a 'HEAD'
file that looks like a symref, all stored within a repository's
associated working directory.

This change also consolidates the validation logic between the case
of GIT_DIR being supplied and GIT_DIR not being supplied, cleaning
up the code.

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

 This is in response to Theodore Tso's email asking why 'git log'
 doesn't work in a bare repository.  Now it does.  :-)

 setup.c |   73 +++++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/setup.c b/setup.c
index 2afdba4..2ae57f7 100644
--- a/setup.c
+++ b/setup.c
@@ -131,28 +131,46 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 }
 
 /*
- * Test if it looks like we're at the top level git directory.
+ * Test if it looks like we're at a git directory.
  * We want to see:
  *
- *  - either a .git/objects/ directory _or_ the proper
+ *  - either a objects/ directory _or_ the proper
  *    GIT_OBJECT_DIRECTORY environment variable
- *  - a refs/ directory under ".git"
+ *  - a refs/ directory
  *  - either a HEAD symlink or a HEAD file that is formatted as
  *    a proper "ref:".
  */
-static int is_toplevel_directory(void)
+static int is_git_directory(const char *suspect)
 {
-	if (access(".git/refs/", X_OK) ||
-	    access(getenv(DB_ENVIRONMENT) ?
-		   getenv(DB_ENVIRONMENT) : ".git/objects/", X_OK) ||
-	    validate_symref(".git/HEAD"))
+	char path[PATH_MAX];
+	size_t len = strlen(suspect);
+
+	strcpy(path, suspect);
+	if (getenv(DB_ENVIRONMENT)) {
+		if (access(getenv(DB_ENVIRONMENT), X_OK))
+			return 0;
+	}
+	else {
+		strcpy(path + len, "/objects");
+		if (access(path, X_OK))
+			return 0;
+	}
+
+	strcpy(path + len, "/refs");
+	if (access(path, X_OK))
 		return 0;
+
+	strcpy(path + len, "/HEAD");
+	if (validate_symref(path))
+		return 0;
+
 	return 1;
 }
 
 const char *setup_git_directory_gently(int *nongit_ok)
 {
 	static char cwd[PATH_MAX+1];
+	const char *gitdirenv;
 	int len, offset;
 
 	/*
@@ -160,36 +178,17 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	 * to do any discovery, but we still do repository
 	 * validation.
 	 */
-	if (getenv(GIT_DIR_ENVIRONMENT)) {
-		char path[PATH_MAX];
-		int len = strlen(getenv(GIT_DIR_ENVIRONMENT));
-		if (sizeof(path) - 40 < len)
+	gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
+	if (gitdirenv) {
+		if (PATH_MAX - 40 < strlen(gitdirenv))
 			die("'$%s' too big", GIT_DIR_ENVIRONMENT);
-		memcpy(path, getenv(GIT_DIR_ENVIRONMENT), len);
-		
-		strcpy(path + len, "/refs");
-		if (access(path, X_OK))
-			goto bad_dir_environ;
-		strcpy(path + len, "/HEAD");
-		if (validate_symref(path))
-			goto bad_dir_environ;
-		if (getenv(DB_ENVIRONMENT)) {
-			if (access(getenv(DB_ENVIRONMENT), X_OK))
-				goto bad_dir_environ;
-		}
-		else {
-			strcpy(path + len, "/objects");
-			if (access(path, X_OK))
-				goto bad_dir_environ;
-		}
-		return NULL;
-	bad_dir_environ:
+		if (is_git_directory(gitdirenv))
+			return NULL;
 		if (nongit_ok) {
 			*nongit_ok = 1;
 			return NULL;
 		}
-		path[len] = 0;
-		die("Not a git repository: '%s'", path);
+		die("Not a git repository: '%s'", gitdirenv);
 	}
 
 	if (!getcwd(cwd, sizeof(cwd)) || cwd[0] != '/')
@@ -197,11 +196,17 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	offset = len = strlen(cwd);
 	for (;;) {
-		if (is_toplevel_directory())
+		if (is_git_directory(".git"))
 			break;
 		chdir("..");
 		do {
 			if (!offset) {
+				if (is_git_directory(cwd)) {
+					if (chdir(cwd))
+						die("Cannot come back to cwd");
+					setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
+					return NULL;
+				}
 				if (nongit_ok) {
 					if (chdir(cwd))
 						die("Cannot come back to cwd");
-- 
1.5.0.rc0.g6bb1

[RFC/PATCH 4/4] Disallow working directory commands in a bare repository.

[Shawn O. Pearce]

If the user tries to run a porcelainish command which requires
a working directory in a bare repository they may get unexpected
results which are difficult to predict and may differ from command
to command.

Instead we should detect that the current repository is a bare
repository and refuse to run the command there, as there is no
working directory associated with it.

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

 This is more of an RFC than an actual patch.  I think its a good
 idea (clearly, as I spent the time to write it) but this may break
 users who explicitly set GIT_DIR to some path which doesn't end in
 "/.git" (e.g. GIT_DIR=$HOME/foo.git).

 I've tried to only alter poreclainish and leave plumbing alone,
 under the rationale that a different Porcelain may have different
 behavior with regards to GIT_DIR.

 git-am.sh       |    1 +
 git-checkout.sh |    1 +
 git-clean.sh    |    1 +
 git-commit.sh   |    1 +
 git-merge.sh    |    1 +
 git-pull.sh     |    1 +
 git-rebase.sh   |    1 +
 git-reset.sh    |    1 +
 git-revert.sh   |    1 +
 git-sh-setup.sh |    7 +++++++
 git.c           |   11 +++++++----
 11 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index c3bbd78..8487e75 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -7,6 +7,7 @@ USAGE='[--signoff] [--dotest=<dir>] [--utf8] [--binary] [--3way]
   or, when resuming [--skip | --resolved]'
 . git-sh-setup
 set_reflog_action am
+require_not_bare
 
 git var GIT_COMMITTER_IDENT >/dev/null || exit
 
diff --git a/git-checkout.sh b/git-checkout.sh
index 92ec069..b2d2dfa 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -3,6 +3,7 @@
 USAGE='[-f] [-b <new_branch>] [-m] [<branch>] [<paths>...]'
 SUBDIRECTORY_OK=Sometimes
 . git-sh-setup
+require_not_bare
 
 old_name=HEAD
 old=$(git-rev-parse --verify $old_name 2>/dev/null)
diff --git a/git-clean.sh b/git-clean.sh
index 3834323..79c5bad 100755
--- a/git-clean.sh
+++ b/git-clean.sh
@@ -14,6 +14,7 @@ When optional <paths>... arguments are given, the paths
 affected are further limited to those that match them.'
 SUBDIRECTORY_OK=Yes
 . git-sh-setup
+require_not_bare
 
 ignored=
 ignoredonly=
diff --git a/git-commit.sh b/git-commit.sh
index 6bce41a..813f41c 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -6,6 +6,7 @@
 USAGE='[-a] [-s] [-v] [--no-verify] [-m <message> | -F <logfile> | (-C|-c) <commit>] [-u] [--amend] [-e] [--author <author>] [[-i | -o] <path>...]'
 SUBDIRECTORY_OK=Yes
 . git-sh-setup
+require_not_bare
 
 git-rev-parse --verify HEAD >/dev/null 2>&1 || initial_commit=t
 branch=$(GIT_DIR="$GIT_DIR" git-symbolic-ref HEAD)
diff --git a/git-merge.sh b/git-merge.sh
index ba42260..d91ff0d 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -7,6 +7,7 @@ USAGE='[-n] [--no-commit] [--squash] [-s <strategy>] [-m=<merge-message>] <commi
 
 . git-sh-setup
 set_reflog_action "merge $*"
+require_not_bare
 
 LF='
 '
diff --git a/git-pull.sh b/git-pull.sh
index 28d0819..1d91386 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -8,6 +8,7 @@ USAGE='[-n | --no-summary] [--no-commit] [-s strategy]... [<fetch-options>] <rep
 LONG_USAGE='Fetch one or more remote refs and merge it/them into the current HEAD.'
 . git-sh-setup
 set_reflog_action "pull $*"
+require_not_bare
 
 strategy_args= no_summary= no_commit= squash=
 while case "$#,$1" in 0) break ;; *,-*) ;; *) break ;; esac
diff --git a/git-rebase.sh b/git-rebase.sh
index 828c59c..fd58e8e 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -29,6 +29,7 @@ Example:       git-rebase master~1 topic
 '
 . git-sh-setup
 set_reflog_action rebase
+require_not_bare
 
 RESOLVEMSG="
 When you have resolved this problem run \"git rebase --continue\".
diff --git a/git-reset.sh b/git-reset.sh
index a969370..90164e1 100755
--- a/git-reset.sh
+++ b/git-reset.sh
@@ -6,6 +6,7 @@ USAGE='[--mixed | --soft | --hard]  [<commit-ish>] [ [--] <paths>...]'
 SUBDIRECTORY_OK=Yes
 . git-sh-setup
 set_reflog_action "reset $*"
+require_not_bare
 
 update= reset_type=--mixed
 unset rev
diff --git a/git-revert.sh b/git-revert.sh
index 50cc47b..f0d2829 100755
--- a/git-revert.sh
+++ b/git-revert.sh
@@ -19,6 +19,7 @@ case "$0" in
 	die "What are you talking about?" ;;
 esac
 . git-sh-setup
+require_not_bare
 
 no_commit=
 while case "$#" in 0) break ;; esac
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 87b939c..7e1d024 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -28,6 +28,13 @@ set_reflog_action() {
 	fi
 }
 
+require_not_bare() {
+	case "$GIT_DIR" in
+	.git|*/.git) : ok;;
+	*) die "fatal: $0 cannot be used in a bare git directory."
+	esac
+}
+
 if [ -z "$LONG_USAGE" ]
 then
 	LONG_USAGE="Usage: $0 $USAGE"
diff --git a/git.c b/git.c
index c82ca45..61c6390 100644
--- a/git.c
+++ b/git.c
@@ -199,6 +199,7 @@ const char git_version_string[] = GIT_VERSION;
 
 #define RUN_SETUP	(1<<0)
 #define USE_PAGER	(1<<1)
+#define NOT_BARE 	(1<<2)
 
 static void handle_internal_command(int argc, const char **argv, char **envp)
 {
@@ -208,7 +209,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 		int (*fn)(int, const char **, const char *);
 		int option;
 	} commands[] = {
-		{ "add", cmd_add, RUN_SETUP },
+		{ "add", cmd_add, RUN_SETUP | NOT_BARE },
 		{ "annotate", cmd_annotate, },
 		{ "apply", cmd_apply },
 		{ "archive", cmd_archive },
@@ -238,7 +239,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 		{ "mailinfo", cmd_mailinfo },
 		{ "mailsplit", cmd_mailsplit },
 		{ "merge-file", cmd_merge_file },
-		{ "mv", cmd_mv, RUN_SETUP },
+		{ "mv", cmd_mv, RUN_SETUP | NOT_BARE },
 		{ "name-rev", cmd_name_rev, RUN_SETUP },
 		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
 		{ "pickaxe", cmd_blame, RUN_SETUP | USE_PAGER },
@@ -251,8 +252,8 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 		{ "rerere", cmd_rerere, RUN_SETUP },
 		{ "rev-list", cmd_rev_list, RUN_SETUP },
 		{ "rev-parse", cmd_rev_parse, RUN_SETUP },
-		{ "rm", cmd_rm, RUN_SETUP },
-		{ "runstatus", cmd_runstatus, RUN_SETUP },
+		{ "rm", cmd_rm, RUN_SETUP | NOT_BARE },
+		{ "runstatus", cmd_runstatus, RUN_SETUP | NOT_BARE },
 		{ "shortlog", cmd_shortlog, RUN_SETUP | USE_PAGER },
 		{ "show-branch", cmd_show_branch, RUN_SETUP },
 		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
@@ -289,6 +290,8 @@ static void handle_internal_command(int argc, const char **argv, char **envp)
 			prefix = setup_git_directory();
 		if (p->option & USE_PAGER)
 			setup_pager();
+		if (p->option & NOT_BARE && is_bare_git_dir(get_git_dir()))
+			die("%s cannot be used in a pare git directory", cmd);
 		trace_argv_printf(argv, argc, "trace: built-in: git");
 
 		exit(p->fn(argc, argv, prefix));
-- 
1.5.0.rc0.g6bb1

Re: [PATCH 3/4] Automatically detect a bare git repository.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

>  This is in response to Theodore Tso's email asking why 'git log'
>  doesn't work in a bare repository.  Now it does.  :-)

Does it?

> +static int is_git_directory(const char *suspect)
>  {
> +	char path[PATH_MAX];
> ...
> +
>  	return 1;
>  }

I think this is a good refactoring.

> @@ -160,36 +178,17 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  	 * to do any discovery, but we still do repository
>  	 * validation.
>  	 */
> -	if (getenv(GIT_DIR_ENVIRONMENT)) {
> -		char path[PATH_MAX];
> -		int len = strlen(getenv(GIT_DIR_ENVIRONMENT));
> -		if (sizeof(path) - 40 < len)
> +	gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
> +	if (gitdirenv) {
> +		if (PATH_MAX - 40 < strlen(gitdirenv))
>  			die("'$%s' too big", GIT_DIR_ENVIRONMENT);
> -		memcpy(path, getenv(GIT_DIR_ENVIRONMENT), len);
> -		
> -		strcpy(path + len, "/refs");
> -		if (access(path, X_OK))
> -			goto bad_dir_environ;
> -		strcpy(path + len, "/HEAD");
> -		if (validate_symref(path))
> -			goto bad_dir_environ;
> -		if (getenv(DB_ENVIRONMENT)) {
> -			if (access(getenv(DB_ENVIRONMENT), X_OK))
> -				goto bad_dir_environ;
> -		}
> -		else {
> -			strcpy(path + len, "/objects");
> -			if (access(path, X_OK))
> -				goto bad_dir_environ;
> -		}
> -		return NULL;
> -	bad_dir_environ:
> +		if (is_git_directory(gitdirenv))
> +			return NULL;
>  		if (nongit_ok) {
>  			*nongit_ok = 1;
>  			return NULL;
>  		}

I do not think this is correct.

What happens when GIT_DIR is set, and nongit_ok is passed?
Earlier code returned NULL after setting *nongit_ok so that the
caller knows the environment points at a directory that is not
yet a git repository control area.

> @@ -197,11 +196,17 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  
>  	offset = len = strlen(cwd);
>  	for (;;) {
> -		if (is_toplevel_directory())
> +		if (is_git_directory(".git"))
>  			break;
>  		chdir("..");
>  		do {
>  			if (!offset) {
> +				if (is_git_directory(cwd)) {
> +					if (chdir(cwd))
> +						die("Cannot come back to cwd");
> +					setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
> +					return NULL;
> +				}
>  				if (nongit_ok) {
>  					if (chdir(cwd))
>  						die("Cannot come back to cwd");

I do not know what the new behaviour of this part of the code is
trying to do.  This is supposed to see if "." is the toplevel
(equivalently, ".git" is the git_dir, in your implementation),
otherwise chdir("..") repeatedly until it finds one, and the
normal return condition is for the working directory of the
process to be at the toplevel.  So chdir(cwd) you introduced is
obviously changing the behaviour.

The existing chdir(cwd) is for an error return -- when there was
no directory that has ".git" even when you went all the way up
to the root level, we give up and come back to where we started,
only when the caller suspected that there was no git directory
and is prepared to handle that case, which is signalled by us
storing 1 to *nongit_ok.

Re: [PATCH 3/4] Automatically detect a bare git repository.

[Shawn Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> >  This is in response to Theodore Tso's email asking why 'git log'
> >  doesn't work in a bare repository.  Now it does.  :-)
> 
> Does it?

It did in my testing.  ;-)
 
> > @@ -160,36 +178,17 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >  	 * to do any discovery, but we still do repository
> >  	 * validation.
> >  	 */
> > -	if (getenv(GIT_DIR_ENVIRONMENT)) {
> > -		char path[PATH_MAX];
> > -		int len = strlen(getenv(GIT_DIR_ENVIRONMENT));
> > -		if (sizeof(path) - 40 < len)
> > +	gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
> > +	if (gitdirenv) {
> > +		if (PATH_MAX - 40 < strlen(gitdirenv))
> >  			die("'$%s' too big", GIT_DIR_ENVIRONMENT);
> > -		memcpy(path, getenv(GIT_DIR_ENVIRONMENT), len);
> > -		
> > -		strcpy(path + len, "/refs");
> > -		if (access(path, X_OK))
> > -			goto bad_dir_environ;
> > -		strcpy(path + len, "/HEAD");
> > -		if (validate_symref(path))
> > -			goto bad_dir_environ;
> > -		if (getenv(DB_ENVIRONMENT)) {
> > -			if (access(getenv(DB_ENVIRONMENT), X_OK))
> > -				goto bad_dir_environ;
> > -		}
> > -		else {
> > -			strcpy(path + len, "/objects");
> > -			if (access(path, X_OK))
> > -				goto bad_dir_environ;
> > -		}
> > -		return NULL;
> > -	bad_dir_environ:
> > +		if (is_git_directory(gitdirenv))
> > +			return NULL;
> >  		if (nongit_ok) {
> >  			*nongit_ok = 1;
> >  			return NULL;
> >  		}
> 
> I do not think this is correct.
>
> What happens when GIT_DIR is set, and nongit_ok is passed?
> Earlier code returned NULL after setting *nongit_ok so that the
> caller knows the environment points at a directory that is not
> yet a git repository control area.

The new code is correct, or at least does what the old code did.

If GIT_DIR is set we call is_git_directory(); if that returns 1
then the checks passed.  In this case the old code returned NULL
and ignored nongit_ok.  We do the same in the new code.

If GIT_DIR is set and we fail is_git_directory() then one of the
checks failed.  In this case the old code jumped to bad_dir_env.
In the new code we don't return NULL and fall through into the if
nongit_ok testing, or into the die("Not a git repository").
 
> > @@ -197,11 +196,17 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >  
> >  	offset = len = strlen(cwd);
> >  	for (;;) {
> > -		if (is_toplevel_directory())
> > +		if (is_git_directory(".git"))
> >  			break;
> >  		chdir("..");
> >  		do {
> >  			if (!offset) {
> > +				if (is_git_directory(cwd)) {
> > +					if (chdir(cwd))
> > +						die("Cannot come back to cwd");
> > +					setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
> > +					return NULL;
> > +				}
> >  				if (nongit_ok) {
> >  					if (chdir(cwd))
> >  						die("Cannot come back to cwd");
> 
> I do not know what the new behaviour of this part of the code is
> trying to do.  This is supposed to see if "." is the toplevel
> (equivalently, ".git" is the git_dir, in your implementation),
> otherwise chdir("..") repeatedly until it finds one, and the
> normal return condition is for the working directory of the
> process to be at the toplevel.  So chdir(cwd) you introduced is
> obviously changing the behaviour.

No, its not.

We only bother to look to see if the original cwd (before we started
chdir("..")'ing up) is a git directory if we did not find one during
that chdir up loop.  This means our current working directory is now
"/" but we found a valid repository in cwd.

In this case we chdir back to the repository directory before
returning back to the caller, as what's the point of being in "/"
when running in a bare repository?  Probably better to be in the
repository directory itself.

One could argue that maybe we should run in "/", or in "/tmp" in
this case as there is no working directory associated with this
repository, but that argument is about the same as just saying we
go back into the now discovered GIT_DIR.

-- 
Shawn.

Re: [RFC/PATCH 4/4] Disallow working directory commands in a bare repository.

[Junio C Hamano]

I think the intent is good.

But execution has room for improvement, and it is not your
fault.  The shell script version of require_not_bare is as
fragile as is_bare_git_dir(), which I already do not like, and I
think if we are going to use is_bare_git_dir() more, I think we
would want to have something a bit more robust.

If we could outlaw $GIT_DIR/index in a bare repository, then
lack of $GIT_DIR/index combined with nonexistence of ref that
is pointed at by $GIT_DIR/HEAD could become a good indication
that the repository is bare ("the current branch unborn" check
is needed not to mistake a repository before the initial commit
as a bare one).

Alas, many public repositories you would see (e.g. check
kernel.org) have been primed with rsync of .git/ from
developer's working repository and have leftover index that is
otherwise unused.  Because of this heavy historical baggage, I
suspect that it is rather hard to convince people to allow us to
use this technique.

When you have $GIT_DIR in your environment, no working-tree
command is expected to work unless you are at the toplevel of
the working-tree.  In the past, people talked about their
workflows using more than one working trees that are associated
with a single $GIT_DIR and that is certainly supposed to work.
But I wonder how widely such a set-up is employed in practice.
If we outlawed working-tree commands when $GIT_DIR environment
exists, how much hurt are we talking about, I wonder.

Another thing to think about is if we are happy with the above
restriction that makes environment $GIT_DIR to imply you are
always working at the toplevel.  Maybe it could be a good idea
to kill this bird as well by introducing $GIT_WORKTREE_TOP
environment variable.  Presence of it obviously means we are not
in a bare repository, but at the same time it would allow us to
teach setup_git_directory_gentry() to cd up to that directory to
make the commands behave as expected.

Re: [PATCH 3/4] Automatically detect a bare git repository.

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> Junio C Hamano <junkio@cox.net> wrote:
> ...
>
> If GIT_DIR is set we call is_git_directory(); if that returns 1
> then the checks passed.  In this case the old code returned NULL
> and ignored nongit_ok.  We do the same in the new code.

Ok.

>> >  	for (;;) {
>> > -		if (is_toplevel_directory())
>> > +		if (is_git_directory(".git"))
>> >  			break;
>> >  		chdir("..");
>> >  		do {
>> >  			if (!offset) {
>> > +				if (is_git_directory(cwd)) {
>> > +					if (chdir(cwd))
>> > +						die("Cannot come back to cwd");
>> > +					setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
>> > +					return NULL;
>> > +				}
>> >  				if (nongit_ok) {
>> >  					if (chdir(cwd))
>> >  						die("Cannot come back to cwd");
>> 
>> I do not know what the new behaviour of this part of the code is
>> trying to do.  This is supposed to see if "." is the toplevel
>> (equivalently, ".git" is the git_dir, in your implementation),
>> otherwise chdir("..") repeatedly until it finds one, and the
>> normal return condition is for the working directory of the
>> process to be at the toplevel.  So chdir(cwd) you introduced is
>> obviously changing the behaviour.
>
> No, its not.

Ah, "changing the behaviour" is the correct thing to do, because
the code is now allowing a new fallback to allow the original
directory to be a bare git repository, and the new chdir/setenv
makes perfect sense.  Thanks for clarification.

For a short time I wondered if this should be a fallback
position, or we might want to make this the very first thing to
check, but I think adding this as the last ditch fallback
position as you did is much safer than making it the first thing
to check.

Thanks.

Re: [RFC/PATCH 4/4] Disallow working directory commands in a bare repository.

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> But execution has room for improvement, and it is not your
> fault.  The shell script version of require_not_bare is as
> fragile as is_bare_git_dir(), which I already do not like, and I
> think if we are going to use is_bare_git_dir() more, I think we
> would want to have something a bit more robust.

Agreed.  But as you point out...
 
> If we could outlaw $GIT_DIR/index in a bare repository, then
> lack of $GIT_DIR/index combined with nonexistence of ref that
> is pointed at by $GIT_DIR/HEAD could become a good indication
> that the repository is bare ("the current branch unborn" check
> is needed not to mistake a repository before the initial commit
> as a bare one).
> 
> Alas, many public repositories you would see (e.g. check
> kernel.org) have been primed with rsync of .git/ from
> developer's working repository and have leftover index that is
> otherwise unused.  Because of this heavy historical baggage, I
> suspect that it is rather hard to convince people to allow us to
> use this technique.

I almost coded the require_not_bare() to look for $GIT_DIR/index
but didn't for the reasons you point out above.  It is too bad that
we didn't enforce the existance of the index file better in the past.

> When you have $GIT_DIR in your environment, no working-tree
> command is expected to work unless you are at the toplevel of
> the working-tree.  In the past, people talked about their
> workflows using more than one working trees that are associated
> with a single $GIT_DIR 

What happens with $GIT_DIR/index in this case?  Or $GIT_DIR/HEAD?
Its insane because both need to be affiliated with the working
directory, yet there's only the one set.  Sure the user could also
arrange for $GIT_INDEX_FILE to be set based on the working directory,
but what about $GIT_DIR/HEAD?  There is no equivilant.

> and that is certainly supposed to work.
> But I wonder how widely such a set-up is employed in practice.
> If we outlawed working-tree commands when $GIT_DIR environment
> exists, how much hurt are we talking about, I wonder.

I wouldn't be hurt, but I don't call Porcelain-ish unless I'm
entering commands directly on the command line, and I never set
GIT_DIR except in scripts, and even then its very rare and is more
to point at a bare repository than one with a working directory.
I suspect that probably isn't true for everyone.

> Another thing to think about is if we are happy with the above
> restriction that makes environment $GIT_DIR to imply you are
> always working at the toplevel.  Maybe it could be a good idea
> to kill this bird as well by introducing $GIT_WORKTREE_TOP
> environment variable.  Presence of it obviously means we are not
> in a bare repository, but at the same time it would allow us to
> teach setup_git_directory_gentry() to cd up to that directory to
> make the commands behave as expected.

I'm not sure what value that environment variable offers here.

Users who set GIT_DIR but don't today set GIT_WORKTREE_TOP are
broken by this change because its not set and we assume its bare.
So they need to also set this environment variable.  But that
is probably going to be annoying for those users as I doubt they
are setting GIT_DIR on a per-directory basis.

Why not just tell these users to setup the working directories with
local .git directories and not use GIT_DIR?

-- 
Shawn.

Re: [RFC/PATCH 4/4] Disallow working directory commands in a bare repository.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Agreed.  But as you point out...
>  
>> If we could outlaw $GIT_DIR/index in a bare repository, then
>> ...
>> Alas, many public repositories you would see (e.g. check
>> kernel.org) have been primed with rsync of .git/ from
>> developer's working repository and have leftover index that is
>> otherwise unused.  Because of this heavy historical baggage, I
>> suspect that it is rather hard to convince people to allow us to
>> use this technique.
>
> I almost coded the require_not_bare() to look for $GIT_DIR/index
> but didn't for the reasons you point out above.  It is too bad that
> we didn't enforce the existance of the index file better in the past.

This, and this,...

>> ...
>> If we outlawed working-tree commands when $GIT_DIR environment
>> exists, how much hurt are we talking about, I wonder.
>
> I wouldn't be hurt, but I don't call Porcelain-ish unless I'm
> entering commands directly on the command line, and I never set
> GIT_DIR except in scripts, and even then its very rare and is more
> to point at a bare repository than one with a working directory.
> I suspect that probably isn't true for everyone.
> ...
> Why not just tell these users to setup the working directories with
> local .git directories and not use GIT_DIR?

suggest that we might want to bite the bullet and declare that
these things are not supported anymore in v1.5.0.

So far, especially after fixing the i18n issue in response to
qgit problem report, I do not think we have any serious backward
incompatibility that the users cannot opt out of that we need to
mention in v1.5.0 announcement.  Even not the incompatibilities
the 'old news' section talks about are.

I think an index file in a bare repository is not what the user
wanted to have because it was useful, but is left there because
it does not hurt.  So in that sense, I do not think we need to
ask users' permission to do this change, as long as we make sure
we give them enough advance warning.

I do not offhand think of a valid workflow that relies on
setting GIT_DIR in the environment and being able to run
working-tree commands, that cannot be worked around if we declar
that GIT_DIR is for our internal use (and no, naming the
directory other people call .git to .svn does not count as a
valid workflow), but I think this may be a bit more serious
change than the index file one.

Comments?

Re: [RFC/PATCH 4/4] Disallow working directory commands in a bare repository.

[Theodore Tso]

On Sun, Dec 31, 2006 at 12:01:01AM -0800, Junio C Hamano wrote:
> > Why not just tell these users to setup the working directories with
> > local .git directories and not use GIT_DIR?
> 
> suggest that we might want to bite the bullet and declare that
> these things are not supported anymore in v1.5.0.

While we're talking about potentially deprecating GIT_DIR for users,
out of curiosity, what valid workflows would cause users to want to
use GIT_INDEX_FILE and GIT_OBJECT_DIRECTORY?  Seems like they would
cause more confusion and support problems than anything else.  

						- Ted

Re: [PATCH 3/4] Automatically detect a bare git repository.

[Theodore Tso]

On Sat, Dec 30, 2006 at 11:30:19PM -0500, Shawn O. Pearce wrote:
> Many users find it unfriendly that they can create a bare git
> repository easily with `git clone --bare` but are then unable to
> run simple commands like `git log` once they cd into that newly
> created bare repository.  This occurs because we do not check to
> see if the current working directory is a git repository.

Thanks for coding this up!   

If you do this, does this mean that we can also eliminate the global
variable --bare, since git will be able to figure out we're in a bare
repository all by itself?

						- Ted

Re: [RFC/PATCH 4/4] Disallow working directory commands in a bare repository.

[Johannes Schindelin]

Hi,

On Sun, 31 Dec 2006, Theodore Tso wrote:

> On Sun, Dec 31, 2006 at 12:01:01AM -0800, Junio C Hamano wrote:
> > > Why not just tell these users to setup the working directories with
> > > local .git directories and not use GIT_DIR?
> > 
> > suggest that we might want to bite the bullet and declare that
> > these things are not supported anymore in v1.5.0.
> 
> While we're talking about potentially deprecating GIT_DIR for users,
> out of curiosity, what valid workflows would cause users to want to
> use GIT_INDEX_FILE and GIT_OBJECT_DIRECTORY?  Seems like they would
> cause more confusion and support problems than anything else.  

Easy, guys.

It is a valid -- indeed, useful -- thing to be able to script nice 
helpers. For example, in one project I track tar balls. So, I wrote a 
script which will unpack the tar ball in a directory, build a new index 
from it, and commit the corresponding tree on top of the tracking branch. 
This setup relies _heavily_ on being able to redirect GIT_INDEX_FILE and 
GIT_DIR.

So, these things are _useful_. Please don't break them.

As for the presence of "index" in a bare repo: I think this is not a 
problem, _as long_ as things continue to work as before, i.e.

	$ GIT_DIR=$(pwd) git log

and

	$ git --bare log

do _not_ complain if "index" is there. Now, if somebody starts git in a 
bare repo, where "index" is present, it could die with a helpful message 
like

	It seems that this is a bare git repository, but there is an index 
	file present, which contradicts that assumption. If the repository
	is indeed bare, please remove the index file.

Ciao,
Dscho

Re: [PATCH 3/4] Automatically detect a bare git repository.

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

hoi :)

On Sat, Dec 30, 2006 at 11:30:19PM -0500, Shawn O. Pearce wrote:
> We test the current working directory only after we have tried
> searching up the directory tree.  This is to retain backwards
> compatibility with our previous behavior on the off chance that
> a user has a 'refs' and 'objects' subdirectories and a 'HEAD'
> file that looks like a symref, all stored within a repository's
> associated working directory.

Hmm, I have my dot files under GIT control so I can't use this mechanism
to use bare GIT repositories under ~/git/*.git.

Perhaps we should test the current directory first, but check that it
ends in .git?

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC/PATCH 4/4] Disallow working directory commands in a bare repository.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> While we're talking about potentially deprecating GIT_DIR for users,
>> out of curiosity, what valid workflows would cause users to want to
>> use GIT_INDEX_FILE and GIT_OBJECT_DIRECTORY?  Seems like they would
>> cause more confusion and support problems than anything else.  
>
> Easy, guys.
>
> It is a valid -- indeed, useful -- thing to be able to script nice 
> helpers. For example, in one project I track tar balls. So, I wrote a 
> script which will unpack the tar ball in a directory, build a new index 
> from it, and commit the corresponding tree on top of the tracking branch. 
> This setup relies _heavily_ on being able to redirect GIT_INDEX_FILE and 
> GIT_DIR.

I do agree INDEX_FILE and OBJECT_DIRECTORY are handy things for
the user to muck around.  What I am not sure about is GIT_DIR,
in the sense that I suspect it is not such a pain to do without
for such a script.

> ...  Now, if somebody starts git in a 
> bare repo, where "index" is present, it could die with a helpful message 
> like
>
> 	It seems that this is a bare git repository, but there is an index 
> 	file present, which contradicts that assumption. If the repository
> 	is indeed bare, please remove the index file.

That is probably worse.  

 * there is no reason for non working-tree operations such as
   git-log to fail when you go to a bare repository (or for that
   matter .git in a repository with a working-tree).  we should
   not have to error out nor remove the index we will not use.

 * if you did the above in response to a misguided 'git
   checkout' in a bare repository, the next error message the
   user will get will be 'huh?  you are in a bare repository,
   bozo'.

So I do not think the helpful message should even be necessary.

Re: [PATCH 3/4] Automatically detect a bare git repository.

[Shawn O. Pearce]

Martin Waitz <tali@admingilde.org> wrote:
> On Sat, Dec 30, 2006 at 11:30:19PM -0500, Shawn O. Pearce wrote:
> > We test the current working directory only after we have tried
> > searching up the directory tree.  This is to retain backwards
> > compatibility with our previous behavior on the off chance that
> > a user has a 'refs' and 'objects' subdirectories and a 'HEAD'
> > file that looks like a symref, all stored within a repository's
> > associated working directory.
> 
> Hmm, I have my dot files under GIT control so I can't use this mechanism
> to use bare GIT repositories under ~/git/*.git.

Whoops.  I assume you have your .gitignore populated with the other
files/directories in your home directory that shouldn't be under
Git control?
 
> Perhaps we should test the current directory first, but check that it
> ends in .git?

Junio already stated why that may not be a good idea.  My original
version of the patch did check the current directory first, but it
could match a working directory which isn't really a Git repository
but just looks sort of like one.

That wouldn't be backwards compatible, so I moved the current
directory check to just before failure.  That version is the one
that I submitted, and that Junio accepted...

-- 
Shawn.

Re: [PATCH 3/4] Automatically detect a bare git repository.

[Shawn O. Pearce]

Theodore Tso <tytso@mit.edu> wrote:
> On Sat, Dec 30, 2006 at 11:30:19PM -0500, Shawn O. Pearce wrote:
> > Many users find it unfriendly that they can create a bare git
> > repository easily with `git clone --bare` but are then unable to
> > run simple commands like `git log` once they cd into that newly
> > created bare repository.  This occurs because we do not check to
> > see if the current working directory is a git repository.
> 
> Thanks for coding this up!   
> 
> If you do this, does this mean that we can also eliminate the global
> variable --bare, since git will be able to figure out we're in a bare
> repository all by itself?

Probably not.  `git --bare foo` is a Poreclain-ish level option
which has been supported for several versions.  Removing it may
cause pain for users to retrain their fingers to just ignore it.

Besides `git --bare` can be used in cases where the bare Git
repository is actually located within another repository's working
directory and the automatic detection is picking up the other
repository's metadata and not the bare repository's metadata.

So I think --bare is still here to stay for a while.  The new
automatic detection just offers slightly less surprise for users.

-- 
Shawn.

Re: [RFC/PATCH 4/4] Disallow working directory commands in a bare repository.

[Johannes Schindelin]

Hi,

On Sun, 31 Dec 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> While we're talking about potentially deprecating GIT_DIR for users,
> >> out of curiosity, what valid workflows would cause users to want to
> >> use GIT_INDEX_FILE and GIT_OBJECT_DIRECTORY?  Seems like they would
> >> cause more confusion and support problems than anything else.  
> >
> > Easy, guys.
> >
> > It is a valid -- indeed, useful -- thing to be able to script nice 
> > helpers. For example, in one project I track tar balls. So, I wrote a 
> > script which will unpack the tar ball in a directory, build a new index 
> > from it, and commit the corresponding tree on top of the tracking branch. 
> > This setup relies _heavily_ on being able to redirect GIT_INDEX_FILE and 
> > GIT_DIR.
> 
> I do agree INDEX_FILE and OBJECT_DIRECTORY are handy things for
> the user to muck around.  What I am not sure about is GIT_DIR,
> in the sense that I suspect it is not such a pain to do without
> for such a script.

Uhm. In the example I illustrated, I need to set GIT_DIR, because I want 
to commit a new version into a branch of my current repository, but 
without touching the working tree and the index.

Thus, I have to create a temporary directory, unpack the new 
(upstream) version, set GIT_DIR _and_ GIT_INDEX_FILE, do a write-tree and 
a commit-tree with some automatic message, and then update the ref. Yes, I 
could separate the steps, but why make it harder than it needs be?

> > ...  Now, if somebody starts git in a 
> > bare repo, where "index" is present, it could die with a helpful message 
> > like
> >
> > 	It seems that this is a bare git repository, but there is an index 
> > 	file present, which contradicts that assumption. If the repository
> > 	is indeed bare, please remove the index file.
> 
> That is probably worse.  
> 
>  * there is no reason for non working-tree operations such as
>    git-log to fail when you go to a bare repository (or for that
>    matter .git in a repository with a working-tree).  we should
>    not have to error out nor remove the index we will not use.
> 
>  * if you did the above in response to a misguided 'git
>    checkout' in a bare repository, the next error message the
>    user will get will be 'huh?  you are in a bare repository,
>    bozo'.
> 
> So I do not think the helpful message should even be necessary.

Yeah, I meant this message only to appear when you call a program which 
needs a working directory. But I agree that this is probably stupid.

Ciao,
Dscho