[PATCH] Fix git_setup_directory_gently when GIT_DIR is set

[PATCH] Fix git_setup_directory_gently when GIT_DIR is set

[Johannes Schindelin]

When calling git_setup_directory_gently, and GIT_DIR was set, it just
ignored the variable nongit_ok.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 setup.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/setup.c b/setup.c
index fe7f884..74301c2 100644
--- a/setup.c
+++ b/setup.c
@@ -184,6 +184,10 @@ const char *setup_git_directory_gently(i
 		}
 		return NULL;
 	bad_dir_environ:
+		if (nongit_ok) {
+			*nongit_ok = 1;
+			return NULL;
+		}
 		path[len] = 0;
 		die("Not a git repository: '%s'", path);
 	}
-- 
1.3.3.gdb440-dirty

Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set

[Junio C Hamano]

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

> When calling git_setup_directory_gently, and GIT_DIR was set, it just
> ignored the variable nongit_ok.

Hmph.  Is this really a breakage?  That is, gently() is meant
for a case where you do not know if you even find a git
repository and tell it not to complain because you are prepared
for the case where you are not in a git repository.

If the environment has an incorrect GIT_DIR, I think that falls
into a different category.  It is more like "the user or calling
script says we have GIT_DIR there but it is corrupt and
unusable".

I do not have a strong opinion on this, though.  If you have
two commands in your script, the first of which does gently()
with such an environment, your change may allow that first
command to succeed, but if the second command does not say
nongit_ok, it would die() there anyway.

Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set

[Johannes Schindelin]

Hi,

On Mon, 5 Jun 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > When calling git_setup_directory_gently, and GIT_DIR was set, it just
> > ignored the variable nongit_ok.
> 
> Hmph.  Is this really a breakage?  That is, gently() is meant
> for a case where you do not know if you even find a git
> repository and tell it not to complain because you are prepared
> for the case where you are not in a git repository.

Yes, it is a breakage: in git-clone, line 212, we explicitely set GIT_DIR 
(to the not-yet-existing repository path), and call git-init-db. Now, with 
the alias thing we need to get the config if it exists, so we _got_ to 
call gently(). Boom.

Ciao,
Dscho

Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set

[Junio C Hamano]

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

> Hi,
>
> On Mon, 5 Jun 2006, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > When calling git_setup_directory_gently, and GIT_DIR was set, it just
>> > ignored the variable nongit_ok.
>> 
>> Hmph.  Is this really a breakage?  That is, gently() is meant
>> for a case where you do not know if you even find a git
>> repository and tell it not to complain because you are prepared
>> for the case where you are not in a git repository.
>
> Yes, it is a breakage: in git-clone, line 212, we explicitely set GIT_DIR 
> (to the not-yet-existing repository path), and call git-init-db. Now, with 
> the alias thing we need to get the config if it exists, so we _got_ to 
> call gently(). Boom.

Hmph.  Would it be a bug in clone that does not create GIT_DIR
then?

Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set

[Johannes Schindelin]

Hi,

On Mon, 5 Jun 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Hi,
> >
> > On Mon, 5 Jun 2006, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> 
> >> > When calling git_setup_directory_gently, and GIT_DIR was set, it just
> >> > ignored the variable nongit_ok.
> >> 
> >> Hmph.  Is this really a breakage?  That is, gently() is meant
> >> for a case where you do not know if you even find a git
> >> repository and tell it not to complain because you are prepared
> >> for the case where you are not in a git repository.
> >
> > Yes, it is a breakage: in git-clone, line 212, we explicitely set GIT_DIR 
> > (to the not-yet-existing repository path), and call git-init-db. Now, with 
> > the alias thing we need to get the config if it exists, so we _got_ to 
> > call gently(). Boom.
> 
> Hmph.  Would it be a bug in clone that does not create GIT_DIR
> then?

I don't think so. The whole point in calling git-init-db is to create 
that. GIT_DIR is set so that the otherwise nice work-in-a-subdirectory 
does not kick in. Imagine for example:

	git-clone ./. victim

(taken straight out of t5400). If GIT_DIR was not set, git-init-db (which 
reads repositoryformat from the config if that exists, right?) would find 
.git/ in git/t/trash, and _not_ create git/t/trash/victim/.git/.

Ciao,
Dscho

Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set

[Junio C Hamano]

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

>> Hmph.  Would it be a bug in clone that does not create GIT_DIR
>> then?
>
> I don't think so. The whole point in calling git-init-db is to create 
> that. GIT_DIR is set so that the otherwise nice work-in-a-subdirectory 
> does not kick in. Imagine for example:
>
> 	git-clone ./. victim
>
> (taken straight out of t5400). If GIT_DIR was not set, git-init-db (which 
> reads repositoryformat from the config if that exists, right?) would find 
> .git/ in git/t/trash, and _not_ create git/t/trash/victim/.git/.

I know clone currently relies on init-db to create the directory if
it does not exist (I wrote the code after all).

I am questioning if that was a wise thing to do.  In the case of
clone, we _know_ where we want the directory to be, so creating
the directory upfront before calling init-db feels like the
right thing to do.  In all the case other than this "clone calls
init-db" I can think of, if we have GIT_DIR set and it is set to
a non-existent location, it would be a bug in the code/script
and I think it is saner to error out in such a case.

Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> Hmph.  Would it be a bug in clone that does not create GIT_DIR
>>> then?
>>
>> I don't think so. The whole point in calling git-init-db is to create 
>> that. GIT_DIR is set so that the otherwise nice work-in-a-subdirectory 
>> does not kick in. Imagine for example:
>>
>> 	git-clone ./. victim
>>
>> (taken straight out of t5400). If GIT_DIR was not set, git-init-db (which 
>> reads repositoryformat from the config if that exists, right?) would find 
>> .git/ in git/t/trash, and _not_ create git/t/trash/victim/.git/.
>
> I know clone currently relies on init-db to create the directory if
> it does not exist (I wrote the code after all).

Ah, I think I see the real problem is.  Alias handling is done
too early, and for commands like init-db that does _not_ even
want to look at an existing repository it tries to use GIT_DIR.

So how about this patch instead on top of yours?

-- >8 --
git alias: try alias last.

This disables alias "foo" from being used for git-foo, and when
we do use alias we check the built-in and then existing command
names first and then alias as the fallback.  This avoids the
problem of common commands used in scripts getting clobbered by
user specific aliases.

---
diff --git a/git.c b/git.c
index 8854472..6db8f2b 100644
--- a/git.c
+++ b/git.c
@@ -202,6 +202,7 @@ int main(int argc, const char **argv, ch
 	char *slash = strrchr(cmd, '/');
 	char git_command[PATH_MAX + 1];
 	const char *exec_path = NULL;
+	int done_alias = 0;
 
 	/*
 	 * Take the basename of argv[0] as the command
@@ -229,7 +230,6 @@ int main(int argc, const char **argv, ch
 	if (!strncmp(cmd, "git-", 4)) {
 		cmd += 4;
 		argv[0] = cmd;
-		handle_alias(&argc, &argv);
 		handle_internal_command(argc, argv, envp);
 		die("cannot handle %s internally", cmd);
 	}
@@ -287,13 +287,21 @@ int main(int argc, const char **argv, ch
 	exec_path = git_exec_path();
 	prepend_to_path(exec_path, strlen(exec_path));
 
-	handle_alias(&argc, &argv);
+	while (1) {
+		/* See if it's an internal command */
+		handle_internal_command(argc, argv, envp);
 
-	/* See if it's an internal command */
-	handle_internal_command(argc, argv, envp);
+		/* .. then try the external ones */
+		execv_git_cmd(argv);
 
-	/* .. then try the external ones */
-	execv_git_cmd(argv);
+		/* It could be an alias -- this works around the insanity
+		 * of overriding "git log" with "git show" by having
+		 * alias.log = show
+		 */
+		if (done_alias || !handle_alias(&argc, &argv))
+			break;
+		done_alias = 1;
+	}
 
 	if (errno == ENOENT)
 		cmd_usage(0, exec_path, "'%s' is not a git-command", cmd);

Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set

[Johannes Schindelin]

Hi,

On Mon, 5 Jun 2006, Junio C Hamano wrote:

> Junio C Hamano <junkio@cox.net> writes:
> 
> > I know clone currently relies on init-db to create the directory if
> > it does not exist (I wrote the code after all).
> 
> Ah, I think I see the real problem is.  Alias handling is done
> too early, and for commands like init-db that does _not_ even
> want to look at an existing repository it tries to use GIT_DIR.
> 
> So how about this patch instead on top of yours?

Yes, I like it. I was so focused on reading the config early to be done 
with it, and avoiding reading the config twice (which it can do now, with 
your patch, but only if one is so insane to use nested aliases).

Your method of avoiding shadowing proper git commands is elegant, if maybe 
incomplete: you have no way of warning the user that the alias is not 
used. But then, I do not think that matters.

Ciao,
Dscho