{"thread":{"id":"4402","subject":"[PATCH] Fix git_setup_directory_gently when GIT_DIR is set","startedAt":"2006-06-05T17:46:09Z","lastAt":"2006-06-06T05:39:01Z","messageCount":8,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"21266","messageId":"Pine.LNX.4.63.0606051943540.29608@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4402","inReplyTo":null,"subject":"[PATCH] Fix git_setup_directory_gently when GIT_DIR is set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-06-05T17:46:09Z","receivedAt":"2006-06-05T17:46:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nWhen calling git_setup_directory_gently, and GIT_DIR was set, it just\nignored the variable nongit_ok.\n\nSigned-off-by: Johannes Schindelin \n---\n setup.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex fe7f884..74301c2 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -184,6 +184,10 @@ const char *setup_git_directory_gently(i\n \t\t}\n \t\treturn NULL;\n \tbad_dir_environ:\n+\t\tif (nongit_ok) {\n+\t\t\t*nongit_ok = 1;\n+\t\t\treturn NULL;\n+\t\t}\n \t\tpath[len] = 0;\n \t\tdie(\"Not a git repository: '%s'\", path);\n \t}\n-- \n1.3.3.gdb440-dirty\n"},{"id":"21272","messageId":"7vodx74ca9.fsf@assigned-by-dhcp.cox.net","threadId":"4402","inReplyTo":"Pine.LNX.4.63.0606051943540.29608@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-05T19:45:18Z","receivedAt":"2006-06-05T19:45:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> When calling git_setup_directory_gently, and GIT_DIR was set, it just\n> ignored the variable nongit_ok.\n\nHmph. Is this really a breakage? That is, gently() is meant\nfor a case where you do not know if you even find a git\nrepository and tell it not to complain because you are prepared\nfor the case where you are not in a git repository.\n\nIf the environment has an incorrect GIT_DIR, I think that falls\ninto a different category. It is more like \"the user or calling\nscript says we have GIT_DIR there but it is corrupt and\nunusable\".\n\nI do not have a strong opinion on this, though. If you have\ntwo commands in your script, the first of which does gently()\nwith such an environment, your change may allow that first\ncommand to succeed, but if the second command does not say\nnongit_ok, it would die() there anyway.\n"},{"id":"21288","messageId":"Pine.LNX.4.63.0606060053440.25344@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4402","inReplyTo":"7vodx74ca9.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-06-05T22:57:19Z","receivedAt":"2006-06-05T22:57:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Jun 2006, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > When calling git_setup_directory_gently, and GIT_DIR was set, it just\n> > ignored the variable nongit_ok.\n> \n> Hmph. Is this really a breakage? That is, gently() is meant\n> for a case where you do not know if you even find a git\n> repository and tell it not to complain because you are prepared\n> for the case where you are not in a git repository.\n\nYes, it is a breakage: in git-clone, line 212, we explicitely set GIT_DIR \n(to the not-yet-existing repository path), and call git-init-db. Now, with \nthe alias thing we need to get the config if it exists, so we _got_ to \ncall gently(). Boom.\n\nCiao,\nDscho\n"},{"id":"21289","messageId":"7vk67v2o85.fsf@assigned-by-dhcp.cox.net","threadId":"4402","inReplyTo":"Pine.LNX.4.63.0606060053440.25344@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-05T23:10:18Z","receivedAt":"2006-06-05T23:10:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Mon, 5 Jun 2006, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n>> > When calling git_setup_directory_gently, and GIT_DIR was set, it just\n>> > ignored the variable nongit_ok.\n>> \n>> Hmph. Is this really a breakage? That is, gently() is meant\n>> for a case where you do not know if you even find a git\n>> repository and tell it not to complain because you are prepared\n>> for the case where you are not in a git repository.\n>\n> Yes, it is a breakage: in git-clone, line 212, we explicitely set GIT_DIR \n> (to the not-yet-existing repository path), and call git-init-db. Now, with \n> the alias thing we need to get the config if it exists, so we _got_ to \n> call gently(). Boom.\n\nHmph. Would it be a bug in clone that does not create GIT_DIR\nthen?\n"},{"id":"21292","messageId":"Pine.LNX.4.63.0606060117180.25685@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4402","inReplyTo":"7vk67v2o85.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-06-05T23:21:35Z","receivedAt":"2006-06-05T23:21:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Jun 2006, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Hi,\n> >\n> > On Mon, 5 Jun 2006, Junio C Hamano wrote:\n> >\n> >> Johannes Schindelin writes:\n> >> \n> >> > When calling git_setup_directory_gently, and GIT_DIR was set, it just\n> >> > ignored the variable nongit_ok.\n> >> \n> >> Hmph. Is this really a breakage? That is, gently() is meant\n> >> for a case where you do not know if you even find a git\n> >> repository and tell it not to complain because you are prepared\n> >> for the case where you are not in a git repository.\n> >\n> > Yes, it is a breakage: in git-clone, line 212, we explicitely set GIT_DIR \n> > (to the not-yet-existing repository path), and call git-init-db. Now, with \n> > the alias thing we need to get the config if it exists, so we _got_ to \n> > call gently(). Boom.\n> \n> Hmph. Would it be a bug in clone that does not create GIT_DIR\n> then?\n\nI don't think so. The whole point in calling git-init-db is to create \nthat. GIT_DIR is set so that the otherwise nice work-in-a-subdirectory \ndoes not kick in. Imagine for example:\n\n\tgit-clone ./. victim\n\n(taken straight out of t5400). If GIT_DIR was not set, git-init-db (which \nreads repositoryformat from the config if that exists, right?) would find \n.git/ in git/t/trash, and _not_ create git/t/trash/victim/.git/.\n\nCiao,\nDscho\n"},{"id":"21294","messageId":"7vfyij2mo8.fsf@assigned-by-dhcp.cox.net","threadId":"4402","inReplyTo":"Pine.LNX.4.63.0606060117180.25685@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-05T23:43:51Z","receivedAt":"2006-06-05T23:43:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Hmph. Would it be a bug in clone that does not create GIT_DIR\n>> then?\n>\n> I don't think so. The whole point in calling git-init-db is to create \n> that. GIT_DIR is set so that the otherwise nice work-in-a-subdirectory \n> does not kick in. Imagine for example:\n>\n> \tgit-clone ./. victim\n>\n> (taken straight out of t5400). If GIT_DIR was not set, git-init-db (which \n> reads repositoryformat from the config if that exists, right?) would find \n> .git/ in git/t/trash, and _not_ create git/t/trash/victim/.git/.\n\nI know clone currently relies on init-db to create the directory if\nit does not exist (I wrote the code after all).\n\nI am questioning if that was a wise thing to do. In the case of\nclone, we _know_ where we want the directory to be, so creating\nthe directory upfront before calling init-db feels like the\nright thing to do. In all the case other than this \"clone calls\ninit-db\" I can think of, if we have GIT_DIR set and it is set to\na non-existent location, it would be a bug in the code/script\nand I think it is saner to error out in such a case.\n"},{"id":"21301","messageId":"7vmzcr14ao.fsf@assigned-by-dhcp.cox.net","threadId":"4402","inReplyTo":"7vfyij2mo8.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-06T01:06:07Z","receivedAt":"2006-06-06T01:06:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>>> Hmph. Would it be a bug in clone that does not create GIT_DIR\n>>> then?\n>>\n>> I don't think so. The whole point in calling git-init-db is to create \n>> that. GIT_DIR is set so that the otherwise nice work-in-a-subdirectory \n>> does not kick in. Imagine for example:\n>>\n>> \tgit-clone ./. victim\n>>\n>> (taken straight out of t5400). If GIT_DIR was not set, git-init-db (which \n>> reads repositoryformat from the config if that exists, right?) would find \n>> .git/ in git/t/trash, and _not_ create git/t/trash/victim/.git/.\n>\n> I know clone currently relies on init-db to create the directory if\n> it does not exist (I wrote the code after all).\n\nAh, I think I see the real problem is. Alias handling is done\ntoo early, and for commands like init-db that does _not_ even\nwant to look at an existing repository it tries to use GIT_DIR.\n\nSo how about this patch instead on top of yours?\n\n-- >8 --\ngit alias: try alias last.\n\nThis disables alias \"foo\" from being used for git-foo, and when\nwe do use alias we check the built-in and then existing command\nnames first and then alias as the fallback. This avoids the\nproblem of common commands used in scripts getting clobbered by\nuser specific aliases.\n\n---\ndiff --git a/git.c b/git.c\nindex 8854472..6db8f2b 100644\n--- a/git.c\n+++ b/git.c\n@@ -202,6 +202,7 @@ int main(int argc, const char **argv, ch\n \tchar *slash = strrchr(cmd, '/');\n \tchar git_command[PATH_MAX + 1];\n \tconst char *exec_path = NULL;\n+\tint done_alias = 0;\n \n \t/*\n \t * Take the basename of argv[0] as the command\n@@ -229,7 +230,6 @@ int main(int argc, const char **argv, ch\n \tif (!strncmp(cmd, \"git-\", 4)) {\n \t\tcmd += 4;\n \t\targv[0] = cmd;\n-\t\thandle_alias(&argc, &argv);\n \t\thandle_internal_command(argc, argv, envp);\n \t\tdie(\"cannot handle %s internally\", cmd);\n \t}\n@@ -287,13 +287,21 @@ int main(int argc, const char **argv, ch\n \texec_path = git_exec_path();\n \tprepend_to_path(exec_path, strlen(exec_path));\n \n-\thandle_alias(&argc, &argv);\n+\twhile (1) {\n+\t\t/* See if it's an internal command */\n+\t\thandle_internal_command(argc, argv, envp);\n \n-\t/* See if it's an internal command */\n-\thandle_internal_command(argc, argv, envp);\n+\t\t/* .. then try the external ones */\n+\t\texecv_git_cmd(argv);\n \n-\t/* .. then try the external ones */\n-\texecv_git_cmd(argv);\n+\t\t/* It could be an alias -- this works around the insanity\n+\t\t * of overriding \"git log\" with \"git show\" by having\n+\t\t * alias.log = show\n+\t\t */\n+\t\tif (done_alias || !handle_alias(&argc, &argv))\n+\t\t\tbreak;\n+\t\tdone_alias = 1;\n+\t}\n \n \tif (errno == ENOENT)\n \t\tcmd_usage(0, exec_path, \"'%s' is not a git-command\", cmd);\n"},{"id":"21302","messageId":"Pine.LNX.4.63.0606060735400.29431@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4402","inReplyTo":"7vmzcr14ao.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix git_setup_directory_gently when GIT_DIR is set","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-06-06T05:39:01Z","receivedAt":"2006-06-06T05:39:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Jun 2006, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > I know clone currently relies on init-db to create the directory if\n> > it does not exist (I wrote the code after all).\n> \n> Ah, I think I see the real problem is. Alias handling is done\n> too early, and for commands like init-db that does _not_ even\n> want to look at an existing repository it tries to use GIT_DIR.\n> \n> So how about this patch instead on top of yours?\n\nYes, I like it. I was so focused on reading the config early to be done \nwith it, and avoiding reading the config twice (which it can do now, with \nyour patch, but only if one is so insane to use nested aliases).\n\nYour method of avoiding shadowing proper git commands is elegant, if maybe \nincomplete: you have no way of warning the user that the alias is not \nused. But then, I do not think that matters.\n\nCiao,\nDscho\n"}]}