{"thread":{"id":"14712","subject":"[PATCH 1/2] Refactor, adding prepare_git_cmd(const char **argv)","startedAt":"2008-07-28T05:50:27Z","lastAt":"2008-07-29T08:39:57Z","messageCount":11,"participants":["Steffen Prohaska","Junio C Hamano","Johannes Schindelin","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"85242","messageId":"1217224228-31303-1-git-send-email-prohaska@zib.de","threadId":"14712","inReplyTo":null,"subject":"[PATCH 1/2] Refactor, adding prepare_git_cmd(const char **argv)","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-07-28T05:50:27Z","receivedAt":"2008-07-28T05:50:27Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"prepare_git_cmd(const char **argv) adds a first entry \"git\" to\nthe array argv. The new array is allocated on the heap. It's\nthe caller's responsibility to release it with free(). The code\nwas already present in execv_git_cmd() but could not be used from\noutside. Now it can also be called for preparing the command list\nin the MinGW codepath in run-command.c.\n\nSigned-off-by: Steffen Prohaska \n---\n exec_cmd.c | 7 ++++++-\n exec_cmd.h | 1 +\n 2 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 0ed768d..ce6741e 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -78,7 +78,7 @@ void setup_path(void)\n \tstrbuf_release(&new_path);\n }\n \n-int execv_git_cmd(const char **argv)\n+const char **prepare_git_cmd(const char **argv)\n {\n \tint argc;\n \tconst char **nargv;\n@@ -91,6 +91,11 @@ int execv_git_cmd(const char **argv)\n \tfor (argc = 0; argv[argc]; argc++)\n \t\tnargv[argc + 1] = argv[argc];\n \tnargv[argc + 1] = NULL;\n+\treturn nargv;\n+}\n+\n+int execv_git_cmd(const char **argv) {\n+\tconst char **nargv = prepare_git_cmd(argv);\n \ttrace_argv_printf(nargv, \"trace: exec:\");\n \n \t/* execvp() can only ever return if it fails */\ndiff --git a/exec_cmd.h b/exec_cmd.h\nindex 0c46cd5..594f961 100644\n--- a/exec_cmd.h\n+++ b/exec_cmd.h\n@@ -5,6 +5,7 @@ extern void git_set_argv_exec_path(const char *exec_path);\n extern void git_set_argv0_path(const char *path);\n extern const char* git_exec_path(void);\n extern void setup_path(void);\n+extern const char **prepare_git_cmd(const char **argv);\n extern int execv_git_cmd(const char **argv); /* NULL terminated */\n extern int execl_git_cmd(const char *cmd, ...);\n extern const char *system_path(const char *path);\n-- \n1.6.0.rc0.79.gb0320\n"},{"id":"85243","messageId":"1217224228-31303-2-git-send-email-prohaska@zib.de","threadId":"14712","inReplyTo":"1217224228-31303-1-git-send-email-prohaska@zib.de","subject":"[Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless \"git \"","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-07-28T05:50:28Z","receivedAt":"2008-07-28T05:50:28Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"This might solve a fundamental problem we have with the\ncomputation of system directories based on relative paths\nin combination with the new gitexecpath 'libexec/git-core'.\nThe problem is that the program 'git' is hardlinked to\ndirectories with different depth. It is either used as\n'bin/git' (1 directory) or as 'libexec/git-core/git-*'\n(2 directories). Thus, using the same relative path\nin system_path() yields different results when starting from the\ntwo locations. I recognized the problem because /etc/gitconfig\nis no longer be read.\n\nThe patch below might fix the problem by always calling 'bin/git'\nfor builtin commands. The computation in system_path() would\nalways start from 'bin' and thus yields predictable results. I\nam not sure however if it fully solves the problem because other\ncode paths might run the dashed forms directly.\n\nI think the only way to verify correctness would be to stop\ninstalling the dashed forms for builtins. If they were not\ninstalled they could not be called. The only entry point for all\nbuiltins would be 'bin/git'. I don't think we want to stop\ninstalling the dashed forms right away.\n\nSo what shall we do?\n\n\tSteffen\n\n-- 8< --\nWe prefer running the dashless form, so we should use it in\nMinGW's start_command(), too.\n\nSigned-off-by: Steffen Prohaska \n---\n run-command.c | 11 ++++-------\n 1 files changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 6e29fdf..a3b28a6 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -119,9 +119,8 @@ int start_command(struct child_process *cmd)\n \t}\n #else\n \tint s0 = -1, s1 = -1, s2 = -1;\t/* backups of stdin, stdout, stderr */\n-\tconst char *sargv0 = cmd->argv[0];\n+\tconst char **sargv = cmd->argv;\n \tchar **env = environ;\n-\tstruct strbuf git_cmd;\n \n \tif (cmd->no_stdin) {\n \t\ts0 = dup(0);\n@@ -165,9 +164,7 @@ int start_command(struct child_process *cmd)\n \t}\n \n \tif (cmd->git_cmd) {\n-\t\tstrbuf_init(&git_cmd, 0);\n-\t\tstrbuf_addf(&git_cmd, \"git-%s\", cmd->argv[0]);\n-\t\tcmd->argv[0] = git_cmd.buf;\n+\t\tcmd->argv = prepare_git_cmd(cmd->argv);\n \t}\n \n \tcmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);\n@@ -175,9 +172,9 @@ int start_command(struct child_process *cmd)\n \tif (cmd->env)\n \t\tfree_environ(env);\n \tif (cmd->git_cmd)\n-\t\tstrbuf_release(&git_cmd);\n+\t\tfree(cmd->argv);\n \n-\tcmd->argv[0] = sargv0;\n+\tcmd->argv = sargv;\n \tif (s0 >= 0)\n \t\tdup2(s0, 0), close(s0);\n \tif (s1 >= 0)\n-- \n1.6.0.rc0.79.gb0320\n"},{"id":"85244","messageId":"7vvdyqv9bf.fsf@gitster.siamese.dyndns.org","threadId":"14712","inReplyTo":"1217224228-31303-2-git-send-email-prohaska@zib.de","subject":"Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless \"git \"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-28T05:58:44Z","receivedAt":"2008-07-28T05:58:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> ... It is either used as\n> 'bin/git' (1 directory) or as 'libexec/git-core/git-*'\n> (2 directories).\n\nI thought Hannes already fixed that one; we shouldn't have the latter. \n"},{"id":"85261","messageId":"7vfxputq45.fsf@gitster.siamese.dyndns.org","threadId":"14712","inReplyTo":"7vvdyqv9bf.fsf@gitster.siamese.dyndns.org","subject":"Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless \"git \"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-28T07:38:50Z","receivedAt":"2008-07-28T07:38:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Steffen Prohaska writes:\n>\n>> ... It is either used as\n>> 'bin/git' (1 directory) or as 'libexec/git-core/git-*'\n>> (2 directories).\n>\n> I thought Hannes already fixed that one; we shouldn't have the latter. \n\nOops, I misread your message. You are worried about the builtins. Sorry.\n"},{"id":"85274","messageId":"alpine.DEB.1.00.0807281310170.2725@eeepc-johanness","threadId":"14712","inReplyTo":"1217224228-31303-2-git-send-email-prohaska@zib.de","subject":"Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless \"git \"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-28T11:13:16Z","receivedAt":"2008-07-28T11:13:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 28 Jul 2008, Steffen Prohaska wrote:\n\n> This might solve a fundamental problem we have with the computation of \n> system directories based on relative paths in combination with the new \n> gitexecpath 'libexec/git-core'. The problem is that the program 'git' is \n> hardlinked to directories with different depth. It is either used as \n> 'bin/git' (1 directory) or as 'libexec/git-core/git-*' (2 directories). \n> Thus, using the same relative path in system_path() yields different \n> results when starting from the two locations. I recognized the problem \n> because /etc/gitconfig is no longer be read.\n\nI seem to recall that I already suggested stripping \n\"/libexec/git-core/\" if it is found, and fall back to \nstripping one directory level (catching \"/bin/\").\n\nIMHO system_path() should really be that intelligent.\n\n(Of course, the way it is set up now, the _caller_ of git_set_argv0_path() \nhas to do the intelligent thing...)\n\nCiao,\nDscho\n"},{"id":"85336","messageId":"1217277453.488e2e0db0f41@webmail.nextra.at","threadId":"14712","inReplyTo":"1217224228-31303-2-git-send-email-prohaska@zib.de","subject":"Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless \"git \"","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-07-28T20:37:33Z","receivedAt":"2008-07-28T20:37:33Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Zitat von Steffen Prohaska :\n\n> This might solve a fundamental problem we have with the\n> computation of system directories based on relative paths\n> in combination with the new gitexecpath 'libexec/git-core'.\n> The problem is that the program 'git' is hardlinked to\n> directories with different depth. It is either used as\n> 'bin/git' (1 directory) or as 'libexec/git-core/git-*'\n> (2 directories). Thus, using the same relative path\n> in system_path() yields different results when starting from the\n> two locations. I recognized the problem because /etc/gitconfig\n> is no longer be read.\n>\n> The patch below might fix the problem by always calling 'bin/git'\n> for builtin commands. The computation in system_path() would\n> always start from 'bin' and thus yields predictable results. I\n> am not sure however if it fully solves the problem because other\n> code paths might run the dashed forms directly.\n\nThis paragraph should go into the commit message.\n\n> I think the only way to verify correctness would be to stop\n> installing the dashed forms for builtins. If they were not\n> installed they could not be called. The only entry point for all\n> builtins would be 'bin/git'. I don't think we want to stop\n> installing the dashed forms right away.\n>\n> So what shall we do?\n\nYour patches make a lot of sense.\n\n> -- 8< --\n> We prefer running the dashless form, so we should use it in\n> MinGW's start_command(), too.\n>\n> Signed-off-by: Steffen Prohaska \n\nAcked-by: Johannes Sixt \n\n-- Hannes\n"},{"id":"85400","messageId":"766FEC61-619E-4A13-A21B-0734D83074C5@zib.de","threadId":"14712","inReplyTo":"1217277453.488e2e0db0f41@webmail.nextra.at","subject":"Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless \"git \"","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-07-29T05:15:41Z","receivedAt":"2008-07-29T05:15:41Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jul 28, 2008, at 10:37 PM, Johannes Sixt wrote:\n\n> Zitat von Steffen Prohaska :\n>\n>> This might solve a fundamental problem we have with the\n>> computation of system directories based on relative paths\n>> in combination with the new gitexecpath 'libexec/git-core'.\n>> The problem is that the program 'git' is hardlinked to\n>> directories with different depth. It is either used as\n>> 'bin/git' (1 directory) or as 'libexec/git-core/git-*'\n>> (2 directories). Thus, using the same relative path\n>> in system_path() yields different results when starting from the\n>> two locations. I recognized the problem because /etc/gitconfig\n>> is no longer be read.\n>>\n>> The patch below might fix the problem by always calling 'bin/git'\n>> for builtin commands. The computation in system_path() would\n>> always start from 'bin' and thus yields predictable results. I\n>> am not sure however if it fully solves the problem because other\n>> code paths might run the dashed forms directly.\n>\n> This paragraph should go into the commit message.\n\nI'll send an updated patch including a commit message defending\nthe change more offensively.\n\n\n>> I think the only way to verify correctness would be to stop\n>> installing the dashed forms for builtins. If they were not\n>> installed they could not be called. The only entry point for all\n>> builtins would be 'bin/git'. I don't think we want to stop\n>> installing the dashed forms right away.\n>>\n>> So what shall we do?\n>\n> Your patches make a lot of sense.\n\nUnfortunately, I am sure that they do not solve the problem fully, even\nif we removed the dashed forms completely. I'll send a second patch \nthat\nmodifies the Makefile to stop installing the BUILT_INS. It might be\nuseful for debugging.\n\n\tSteffen\n"},{"id":"85406","messageId":"1217308784-25408-1-git-send-email-prohaska@zib.de","threadId":"14712","inReplyTo":"766FEC61-619E-4A13-A21B-0734D83074C5@zib.de","subject":"[PATCH 2/2 v2] run-command (Windows): Run dashless \"git \" (solves part of problem with system_path)","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-07-29T05:19:43Z","receivedAt":"2008-07-29T05:19:43Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"We prefer running the dashless form, so we should use it in MinGW's\nstart_command(), too.\n\nThis solves a fundamental problem we have with the computation of system\ndirectories based on relative paths in combination with the new\ngitexecpath 'libexec/git-core'. The problem is that the program 'git'\nis hardlinked to directories with different depth. It is either used as\n'bin/git' (1 directory) or as 'libexec/git-core/git-*' (2 directories).\nThus, using the same relative path in system_path() yields different\nresults when starting from the two locations. I recognized the problem\nbecause /etc/gitconfig is no longer read.\n\nThis commit fixes the problem by always calling 'bin/git' for builtin\ncommands. The computation in system_path() thus always starts from\n'bin' and yields predictable results. This is however only part of a\nfull solution to the problem outlined above. Remaining problems are:\n\n - Other code paths might run the dashed forms directly.\n\n - We have non-builtins that are implemented in C, e.g. fast-import.c.\n These non-builtins will still compute wrong paths.\n\nSigned-off-by: Steffen Prohaska \nAcked-by: Johannes Sixt \n---\n run-command.c | 11 ++++-------\n 1 files changed, 4 insertions(+), 7 deletions(-)\n\nMy previous mail was in the wrong thread. Apologies. This one\nwill be correctly threaded.\n\n Steffen\n\ndiff --git a/run-command.c b/run-command.c\nindex 6e29fdf..a3b28a6 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -119,9 +119,8 @@ int start_command(struct child_process *cmd)\n \t}\n #else\n \tint s0 = -1, s1 = -1, s2 = -1;\t/* backups of stdin, stdout, stderr */\n-\tconst char *sargv0 = cmd->argv[0];\n+\tconst char **sargv = cmd->argv;\n \tchar **env = environ;\n-\tstruct strbuf git_cmd;\n \n \tif (cmd->no_stdin) {\n \t\ts0 = dup(0);\n@@ -165,9 +164,7 @@ int start_command(struct child_process *cmd)\n \t}\n \n \tif (cmd->git_cmd) {\n-\t\tstrbuf_init(&git_cmd, 0);\n-\t\tstrbuf_addf(&git_cmd, \"git-%s\", cmd->argv[0]);\n-\t\tcmd->argv[0] = git_cmd.buf;\n+\t\tcmd->argv = prepare_git_cmd(cmd->argv);\n \t}\n \n \tcmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);\n@@ -175,9 +172,9 @@ int start_command(struct child_process *cmd)\n \tif (cmd->env)\n \t\tfree_environ(env);\n \tif (cmd->git_cmd)\n-\t\tstrbuf_release(&git_cmd);\n+\t\tfree(cmd->argv);\n \n-\tcmd->argv[0] = sargv0;\n+\tcmd->argv = sargv;\n \tif (s0 >= 0)\n \t\tdup2(s0, 0), close(s0);\n \tif (s1 >= 0)\n-- \n1.6.0.rc0.79.gb0320\n"},{"id":"85405","messageId":"1217308784-25408-2-git-send-email-prohaska@zib.de","threadId":"14712","inReplyTo":"1217308784-25408-1-git-send-email-prohaska@zib.de","subject":"[FOR DEBUGGING] Stop installing BUILT_INS","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-07-29T05:19:44Z","receivedAt":"2008-07-29T05:19:44Z","isPatch":false,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"---\n Makefile | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex b2bc0ef..e416ec7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1366,7 +1366,7 @@ endif\n \t\tln -f \"$$bindir/git$X\" \"$$execdir/git$X\" || \\\n \t\tcp \"$$bindir/git$X\" \"$$execdir/git$X\"; \\\n \tfi && \\\n-\t{ $(foreach p,$(BUILT_INS), $(RM) \"$$execdir/$p\" && ln \"$$execdir/git$X\" \"$$execdir/$p\" ;) } && \\\n+\t{ $(foreach p,$(BUILT_INS), $(RM) \"$$execdir/$p\" ;) } && \\\n \t$(RM) \"$$execdir/git$X\" && \\\n \t./check_bindir \"z$$bindir\" \"z$$execdir\" \"$$bindir/git-add$X\"\n \n-- \n1.6.0.rc0.79.gb0320\n"},{"id":"85409","messageId":"7v8wvlmf6k.fsf@gitster.siamese.dyndns.org","threadId":"14712","inReplyTo":"1217277453.488e2e0db0f41@webmail.nextra.at","subject":"Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless \"git \"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-29T05:29:07Z","receivedAt":"2008-07-29T05:29:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Zitat von Steffen Prohaska :\n> ...\n>> The patch below might fix the problem by always calling 'bin/git'\n>> for builtin commands. The computation in system_path() would\n>> always start from 'bin' and thus yields predictable results. I\n>> am not sure however if it fully solves the problem because other\n>> code paths might run the dashed forms directly.\n>\n> This paragraph should go into the commit message.\n\n> ...\n> Your patches make a lot of sense.\n\nI was almost going to suggest doing this everywhere not just on Windows,\nbut execv_git_cmd() on the POSIX side already runs \"git\" wrapper, so this\npatch makes them in line, finally.\n\n>> -- 8< --\n>> We prefer running the dashless form, so we should use it in\n>> MinGW's start_command(), too.\n>>\n>> Signed-off-by: Steffen Prohaska \n>\n> Acked-by: Johannes Sixt \n>\n> -- Hannes\n"},{"id":"85436","messageId":"1217320797.488ed75ddb14b@webmail.nextra.at","threadId":"14712","inReplyTo":"7v8wvlmf6k.fsf@gitster.siamese.dyndns.org","subject":"Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless \"git \"","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-07-29T08:39:57Z","receivedAt":"2008-07-29T08:39:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Zitat von Junio C Hamano :\n\n> Johannes Sixt writes:\n>\n> > Zitat von Steffen Prohaska :\n> > ...\n> >> The patch below might fix the problem by always calling 'bin/git'\n> >> for builtin commands. The computation in system_path() would\n> >> always start from 'bin' and thus yields predictable results. I\n> >> am not sure however if it fully solves the problem because other\n> >> code paths might run the dashed forms directly.\n> >\n> > This paragraph should go into the commit message.\n>\n> > ...\n> > Your patches make a lot of sense.\n>\n> I was almost going to suggest doing this everywhere not just on Windows,\n> but execv_git_cmd() on the POSIX side already runs \"git\" wrapper, so this\n> patch makes them in line, finally.\n\nFor this reason I'm in favor of these patches. I didn't run the full test suite\nwith them, yet, (you know, that takes a while on Windows), but \"make *clone*\n*fetch* *pack*\" worked out OK.\n\n-- Hannes\n"}]}