# [PATCH] Fix git_setup_directory_gently when GIT_DIR is set 8 messages from 2006-06-05 to 2006-06-06. Participants: Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/4402 ## Johannes Schindelin, 2006-06-05 17:46 Subject: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0606051943540.29608%40wbgn013.biozentrum.uni-wuerzburg.de ``` When calling git_setup_directory_gently, and GIT_DIR was set, it just ignored the variable nongit_ok. Signed-off-by: Johannes Schindelin --- 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 ``` ## Junio C Hamano, 2006-06-05 19:45 Subject: Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set Message-ID: <7vodx74ca9.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vodx74ca9.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin 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. ``` ## Johannes Schindelin, 2006-06-05 22:57 Subject: Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0606060053440.25344%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7vodx74ca9.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Mon, 5 Jun 2006, Junio C Hamano wrote: > Johannes Schindelin 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 ``` ## Junio C Hamano, 2006-06-05 23:10 Subject: Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set Message-ID: <7vk67v2o85.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vk67v2o85.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin writes: > Hi, > > On Mon, 5 Jun 2006, Junio C Hamano wrote: > >> Johannes Schindelin 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? ``` ## Johannes Schindelin, 2006-06-05 23:21 Subject: Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0606060117180.25685%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7vk67v2o85.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Mon, 5 Jun 2006, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Hi, > > > > On Mon, 5 Jun 2006, Junio C Hamano wrote: > > > >> Johannes Schindelin 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 ``` ## Junio C Hamano, 2006-06-05 23:43 Subject: Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set Message-ID: <7vfyij2mo8.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vfyij2mo8.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin 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. ``` ## Junio C Hamano, 2006-06-06 01:06 Subject: Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set Message-ID: <7vmzcr14ao.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vmzcr14ao.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <7vfyij2mo8.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano writes: > Johannes Schindelin 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); ``` ## Johannes Schindelin, 2006-06-06 05:39 Subject: Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0606060735400.29431%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7vmzcr14ao.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Mon, 5 Jun 2006, Junio C Hamano wrote: > Junio C Hamano 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 ```