[PATCH v2] Do check_repository_format() early

[PATCH v2] Do check_repository_format() early

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

Repository version check is only performed when
setup_git_directory() is called. This makes sure
setup_git_directory_gently() does the check too.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 Comment updated.

 setup.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/setup.c b/setup.c
index faf4137..8fde2b2 100644
--- a/setup.c
+++ b/setup.c
@@ -246,8 +246,14 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			static char buffer[1024 + 1];
 			const char *retval;
 
-			if (!work_tree_env)
-				return set_work_tree(gitdirenv);
+			if (!work_tree_env) {
+				retval = set_work_tree(gitdirenv);
+				/* config may override worktree
+				 * see set_work_tree comment */
+				check_repository_format();
+				return retval;
+			}
+			check_repository_format();
 			retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
 					get_git_work_tree());
 			if (!retval || !*retval)
@@ -287,6 +293,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			if (!work_tree_env)
 				inside_work_tree = 0;
 			setenv(GIT_DIR_ENVIRONMENT, ".", 1);
+			check_repository_format();
 			return NULL;
 		}
 		chdir("..");
@@ -307,6 +314,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	if (!work_tree_env)
 		inside_work_tree = 1;
 	git_work_tree_cfg = xstrndup(cwd, offset);
+	check_repository_format();
 	if (offset == len)
 		return NULL;
 
@@ -367,7 +375,6 @@ int check_repository_format(void)
 const char *setup_git_directory(void)
 {
 	const char *retval = setup_git_directory_gently(NULL);
-	check_repository_format();
 
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
-- 
1.5.3.6.2041.g106f-dirty

[Funky] "git -p cmd" inside a bare repository

[Junio C Hamano]

If you have a bare repository and try this there:

	$ PAGER=head git show HEAD:gcc/ChangeLog

it works as expected, but if you explicitly ask for pagination, like
this:

	$ PAGER=head git -p show HEAD:gcc/ChangeLog

you would get a very funky error message:

	fatal: ambiguous argument 'HEAD:gcc/ChangeLog': unknown revision or path not in the working tree.
	Use '--' to separate paths from revisions

as if we are working with a repository with working tree.

I originally noticed this with ls-tree.  The symptom is a bit different:

	$ git -p ls-tree HEAD
	fatal: Not a valid object name HEAD



I _think_ what is happening is that setup_pager() tries to run
git_config(), which runs setup(), and then RUN_SETUP set for "ls-tree"
(or "show") internal command runs setup again.  HEAD is given to
resolve_ref() and git_path("%s", ref) makes it to ".git/HEAD", even
though in a bare repository git_dir should be set to ".", and of course
we cannot find such a path in the git directory.

[PATCH] make git start-up sequence a bit more robust

[Junio C Hamano]

The handle options loop called setup_git_dir() and setup_pager() that
would silently trigger repository discovery while it cycled.  This patch
changes it to collect the options from the user, and uses the
information in a saner, more controlled order, namely:

 - GIT_DIR, GIT_WORK_TREE and GIT_PAGER environments are set as needed;
 - setup_git_directory() is run if needed;
 - setup_work_tree(0 is run if needed;
 - setup_pager() is run if needed; 

The last two wants to read config, which means setup_git_directory()
should come before them, which in turn means GIT_DIR must be set up
before that.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Junio C Hamano <gitster@pobox.com> writes:

 > If you have a bare repository and try this there:
 >
 > 	$ PAGER=head git show HEAD:gcc/ChangeLog
 >
 > it works as expected, but if you explicitly ask for pagination, like
 > this:
 >
 > 	$ PAGER=head git -p show HEAD:gcc/ChangeLog
 >
 > you would get a very funky error message:
 >
 > 	fatal: ambiguous argument 'HEAD:gcc/ChangeLog': unknown revision or path not in the working tree.
 > 	Use '--' to separate paths from revisions
 >
 > as if we are working with a repository with working tree.
 >
 > I originally noticed this with ls-tree.  The symptom is a bit different:
 >
 > 	$ git -p ls-tree HEAD
 > 	fatal: Not a valid object name HEAD
 >
 > I _think_ what is happening is that setup_pager() tries to run
 > git_config(), which runs setup(), and then RUN_SETUP set for "ls-tree"
 > (or "show") internal command runs setup again.  HEAD is given to
 > resolve_ref() and git_path("%s", ref) makes it to ".git/HEAD", even
 > though in a bare repository git_dir should be set to ".", and of course
 > we cannot find such a path in the git directory.

 git.c |   73 ++++++++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/git.c b/git.c
index 15fec89..fd756cd 100644
--- a/git.c
+++ b/git.c
@@ -6,7 +6,21 @@
 const char git_usage_string[] =
 	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
 
-static int handle_options(const char*** argv, int* argc, int* envchanged)
+static int want_pager = -1;
+static const char *want_git_dir;
+static int want_git_dir_badly = 1;
+static const char *want_work_tree;
+
+static void no_env_change_from_alias(const char *alias_command)
+{
+	if (!alias_command)
+		return;
+	die("alias '%s' changes environment variables\n"
+	    "You can use '!git' in the alias to do this.",
+	    alias_command);
+}
+
+static int handle_options(const char ***argv, int *argc, const char *alias_command)
 {
 	int handled = 0;
 
@@ -35,46 +49,42 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 				exit(0);
 			}
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
-			setup_pager();
+			want_pager = 1;
 		} else if (!strcmp(cmd, "--no-pager")) {
-			setenv("GIT_PAGER", "cat", 1);
-			if (envchanged)
-				*envchanged = 1;
+			want_pager = 0;
 		} else if (!strcmp(cmd, "--git-dir")) {
+			no_env_change_from_alias(alias_command);
 			if (*argc < 2) {
 				fprintf(stderr, "No directory given for --git-dir.\n" );
 				usage(git_usage_string);
 			}
-			setenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);
-			if (envchanged)
-				*envchanged = 1;
+			want_git_dir = (*argv)[1];
 			(*argv)++;
 			(*argc)--;
 			handled++;
 		} else if (!prefixcmp(cmd, "--git-dir=")) {
-			setenv(GIT_DIR_ENVIRONMENT, cmd + 10, 1);
-			if (envchanged)
-				*envchanged = 1;
+			no_env_change_from_alias(alias_command);
+			want_git_dir = cmd + 10;
 		} else if (!strcmp(cmd, "--work-tree")) {
+			no_env_change_from_alias(alias_command);
 			if (*argc < 2) {
 				fprintf(stderr, "No directory given for --work-tree.\n" );
 				usage(git_usage_string);
 			}
-			setenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);
-			if (envchanged)
-				*envchanged = 1;
+			want_work_tree = (*argv)[1];
 			(*argv)++;
 			(*argc)--;
+			handled++;
 		} else if (!prefixcmp(cmd, "--work-tree=")) {
-			setenv(GIT_WORK_TREE_ENVIRONMENT, cmd + 12, 1);
-			if (envchanged)
-				*envchanged = 1;
+			no_env_change_from_alias(alias_command);
+			want_work_tree = cmd + 12;
 		} else if (!strcmp(cmd, "--bare")) {
 			static char git_dir[PATH_MAX+1];
+			no_env_change_from_alias(alias_command);
 			is_bare_repository_cfg = 1;
-			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
-			if (envchanged)
-				*envchanged = 1;
+			getcwd(git_dir, sizeof(git_dir));
+			want_git_dir = git_dir;
+			want_git_dir_badly = 0;
 		} else {
 			fprintf(stderr, "Unknown option: %s\n", cmd);
 			usage(git_usage_string);
@@ -153,7 +163,7 @@ static int split_cmdline(char *cmdline, const char ***argv)
 
 static int handle_alias(int *argcp, const char ***argv)
 {
-	int nongit = 0, envchanged = 0, ret = 0, saved_errno = errno;
+	int nongit = 0, ret = 0, saved_errno = errno;
 	const char *subdir;
 	int count, option_count;
 	const char** new_argv;
@@ -183,11 +193,7 @@ static int handle_alias(int *argcp, const char ***argv)
 			    alias_string + 1, alias_command);
 		}
 		count = split_cmdline(alias_string, &new_argv);
-		option_count = handle_options(&new_argv, &count, &envchanged);
-		if (envchanged)
-			die("alias '%s' changes environment variables\n"
-				 "You can use '!git' in the alias to do this.",
-				 alias_command);
+		option_count = handle_options(&new_argv, &count, alias_command);
 		memmove(new_argv - option_count, new_argv,
 				count * sizeof(char *));
 		new_argv -= option_count;
@@ -247,11 +253,12 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 	prefix = NULL;
 	if (p->option & RUN_SETUP)
 		prefix = setup_git_directory();
-	if (p->option & USE_PAGER)
-		setup_pager();
 	if (p->option & NEED_WORK_TREE)
 		setup_work_tree();
 
+	if (0 < want_pager || (p->option & USE_PAGER))
+		setup_pager();
+
 	trace_argv_printf(argv, "trace: built-in: git");
 
 	status = p->fn(argc, argv, prefix);
@@ -434,6 +441,13 @@ int main(int argc, const char **argv)
 	}
 	cmd = argv[0];
 
+	if (want_git_dir)
+		setenv(GIT_DIR_ENVIRONMENT, want_git_dir, want_git_dir_badly);
+	if (want_work_tree)
+		setenv(GIT_WORK_TREE_ENVIRONMENT, want_work_tree, 1);
+	if (!want_pager)
+		setenv("GIT_PAGER", "cat", 1);
+
 	/*
 	 * We use PATH to find git commands, but we prepend some higher
 	 * precidence paths: the "--exec-path" option, the GIT_EXEC_PATH
@@ -446,6 +460,9 @@ int main(int argc, const char **argv)
 		/* See if it's an internal command */
 		handle_internal_command(argc, argv);
 
+		if (!done_alias && 0 < want_pager)
+			setup_pager();
+
 		/* .. then try the external ones */
 		execv_git_cmd(argv);
 

Re: [Funky] "git -p cmd" inside a bare repository

[Jeff King]

On Thu, Dec 13, 2007 at 05:29:23PM -0800, Junio C Hamano wrote:

> I _think_ what is happening is that setup_pager() tries to run
> git_config(), which runs setup(), and then RUN_SETUP set for "ls-tree"
> (or "show") internal command runs setup again.  HEAD is given to
> resolve_ref() and git_path("%s", ref) makes it to ".git/HEAD", even
> though in a bare repository git_dir should be set to ".", and of course
> we cannot find such a path in the git directory.

I think that there is perhaps a larger bug here, which is that running
setup twice gives bad results, and should either be fixed or have its
own "don't run me twice" guard.

But it makes sense to always spawn the pager at the same time for
consistency. As a bonus, this makes "git -p bogus" a little more
friendly by not spawning the pager until we verify the command name.

-- >8 --
delay "git -p" page spawning until command runtime

This makes the timing consistent with those commands that always spawn a
pager. It also avoids a funny interaction related to calling
setup_pager() before setup().

---
diff --git a/git.c b/git.c
index c8b7e74..21d204f 100644
--- a/git.c
+++ b/git.c
@@ -6,6 +6,8 @@
 const char git_usage_string[] =
 	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
 
+static int user_asked_for_pager;
+
 static int handle_options(const char*** argv, int* argc, int* envchanged)
 {
 	int handled = 0;
@@ -35,7 +37,7 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 				exit(0);
 			}
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
-			setup_pager();
+			user_asked_for_pager = 1;
 		} else if (!strcmp(cmd, "--no-pager")) {
 			setenv("GIT_PAGER", "cat", 1);
 			if (envchanged)
@@ -256,7 +258,7 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 	prefix = NULL;
 	if (p->option & RUN_SETUP)
 		prefix = setup_git_directory();
-	if (p->option & USE_PAGER)
+	if (p->option & USE_PAGER || user_asked_for_pager)
 		setup_pager();
 	if (p->option & NEED_WORK_TREE)
 		setup_work_tree();

Re: [Funky] "git -p cmd" inside a bare repository

[Jeff King]

On Fri, Dec 14, 2007 at 12:12:23AM -0500, Jeff King wrote:

> -- >8 --
> delay "git -p" page spawning until command runtime

Hrmph. Bad timing. :)

Your patch is much nicer, though, so please ignore mine.

-Peff

Re: [Funky] "git -p cmd" inside a bare repository

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Dec 14, 2007 at 12:12:23AM -0500, Jeff King wrote:
>
>> -- >8 --
>> delay "git -p" page spawning until command runtime
>
> Hrmph. Bad timing. :)
>
> Your patch is much nicer, though, so please ignore mine.

It may look nicer, but I do not know if it is correct, though.  I do not
do much "work tree" stuff, and would really appreciate testing by people
who are more involved in that part of the system.

Re: [Funky] "git -p cmd" inside a bare repository

[Nguyen Thai Ngoc Duy]

On Dec 14, 2007 8:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
> If you have a bare repository and try this there:
>
>         $ PAGER=head git show HEAD:gcc/ChangeLog
>
> it works as expected, but if you explicitly ask for pagination, like
> this:
>
>         $ PAGER=head git -p show HEAD:gcc/ChangeLog
>
> you would get a very funky error message:
>
>         fatal: ambiguous argument 'HEAD:gcc/ChangeLog': unknown revision or path not in the working tree.
>         Use '--' to separate paths from revisions
>
> as if we are working with a repository with working tree.
>
> I originally noticed this with ls-tree.  The symptom is a bit different:
>
>         $ git -p ls-tree HEAD
>         fatal: Not a valid object name HEAD
>
>
>
> I _think_ what is happening is that setup_pager() tries to run
> git_config(), which runs setup(), and then RUN_SETUP set for "ls-tree"
> (or "show") internal command runs setup again.  HEAD is given to
> resolve_ref() and git_path("%s", ref) makes it to ".git/HEAD", even
> though in a bare repository git_dir should be set to ".", and of course
> we cannot find such a path in the git directory.

You meant setup_git_directory() or setup_git_env()? If find that
setup_git_env() is called too soon so it caches wrong git_dir and
friends if you setup_pager().

Another option (BTW I think your patch is fine) is to call
setup_git_env one more time to refresh its cache.
-- 
Duy

Re: [Funky] "git -p cmd" inside a bare repository

[Johannes Schindelin]

Hi,

On Thu, 13 Dec 2007, Junio C Hamano wrote:

> If you have a bare repository and try this there:
> 
> 	$ PAGER=head git show HEAD:gcc/ChangeLog
> 
> it works as expected, but if you explicitly ask for pagination, like
> this:
> 
> 	$ PAGER=head git -p show HEAD:gcc/ChangeLog

I have no time left to work on git for a few days, so I cannot even review 
your patch.  But Jeff's patch being smaller, I could, and AFAICT it solves 
the problem.

Ciao,
Dscho

Re: [Funky] "git -p cmd" inside a bare repository

[Junio C Hamano]

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

> I have no time left to work on git for a few days, so I cannot even review 
> your patch.  But Jeff's patch being smaller, I could, and AFAICT it solves 
> the problem.

We have more than a few days ;-) Hope you are feeling better now.

You got familialized yourself with the work-tree part because the
initial round was so broken and you had to step in during the last round
(unfortuantely ;-), and Nguyễn also is familiar with that part of the
code.  It would be nice if we can have a proper fix that is not papering
over deeper breakage.

Re: [Funky] "git -p cmd" inside a bare repository

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 899 bytes --]

Hi,

On Fri, 14 Dec 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > I have no time left to work on git for a few days, so I cannot even 
> > review your patch.  But Jeff's patch being smaller, I could, and 
> > AFAICT it solves the problem.
> 
> We have more than a few days ;-) Hope you are feeling better now.
> 
> You got familialized yourself with the work-tree part because the 
> initial round was so broken and you had to step in during the last round 
> (unfortuantely ;-), and Nguyễn also is familiar with that part of the 
> code.  It would be nice if we can have a proper fix that is not papering 
> over deeper breakage.

Like always, you are correct.

Will look into it over the weekend (although I will only be able to report 
back on Monday, due to the wonderful administrators who broke my internet 
connection at home).

Ciao,
Dscho