{"thread":{"id":"14732","subject":"[FOR DEBUGGING] Stop installing BUILT_INS","startedAt":"2008-07-29T05:17:26Z","lastAt":"2008-07-29T11:31:14Z","messageCount":9,"participants":["Steffen Prohaska","Shawn O. Pearce","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"85403","messageId":"1217308647-23673-1-git-send-email-prohaska@zib.de","threadId":"14732","inReplyTo":"BF5B7CBE-ACA8-4D81-8FC0-8A7901205854@zib.de","subject":"[PATCH 2/2 v2] run-command (Windows): Run dashless \"git <cmd>\" (solves part of problem with system_path)","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-07-29T05:17:26Z","receivedAt":"2008-07-29T05:17:26Z","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 <prohaska@zib.de>\nAcked-by: Johannes Sixt <johannes.sixt@telecom.at>\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":"85402","messageId":"1217308647-23673-2-git-send-email-prohaska@zib.de","threadId":"14732","inReplyTo":"1217308647-23673-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:17:27Z","receivedAt":"2008-07-29T05:17:27Z","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":"85408","messageId":"20080729052459.GC11947@spearce.org","threadId":"14732","inReplyTo":"1217308647-23673-1-git-send-email-prohaska@zib.de","subject":"Re: [PATCH 2/2 v2] run-command (Windows): Run dashless \"git <cmd>\" (solves part of problem with system_path)","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-07-29T05:24:59Z","receivedAt":"2008-07-29T05:24:59Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Steffen Prohaska <prohaska@zib.de> wrote:\n> We prefer running the dashless form, so we should use it in MinGW's\n> start_command(), too.\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\nThis feels wrong to me.  fast-import probably won't be adversly\nimpacted by not being able to read /etc/gitconfig, unless the user\nhas set something like core.deltaBaseCacheLimit and is doing an\nincremental import.  But other non-builtins may be impacted.\n\nIt feels like we're fixing this in the wrong place.  If the issue\nis we don't find our installation directory correctly, we should\nfind our installation directory correctly, not work around it by\ncalling builtins through the git wrapper.\n\nThough I can see where it may be a good idea to at some point\nin the future (git 1.7?) stop creating the redundant builtin\nlinks under libexec/git-core.\n\n-- \nShawn.\n"},{"id":"85413","messageId":"7v4p69meka.fsf@gitster.siamese.dyndns.org","threadId":"14732","inReplyTo":"20080729052459.GC11947@spearce.org","subject":"Re: [PATCH 2/2 v2] run-command (Windows): Run dashless \"git <cmd>\" (solves part of problem with system_path)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-29T05:42:29Z","receivedAt":"2008-07-29T05:42:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> This feels wrong to me.  fast-import probably won't be adversly\n> impacted by not being able to read /etc/gitconfig, unless the user\n> has set something like core.deltaBaseCacheLimit and is doing an\n> incremental import.  But other non-builtins may be impacted.\n>\n> It feels like we're fixing this in the wrong place.  If the issue\n> is we don't find our installation directory correctly, we should\n> find our installation directory correctly, not work around it by\n> calling builtins through the git wrapper.\n>\n> Though I can see where it may be a good idea to at some point\n> in the future (git 1.7?) stop creating the redundant builtin\n> links under libexec/git-core.\n\nI agree; that is why I already applied Steffen's original patch with quite\na different justification from the updated one:\n\ncommit b048b9a803f48d88595877271b53bf9ec400e4ba\nAuthor: Steffen Prohaska <prohaska@zib.de>\nDate:   Mon Jul 28 07:50:28 2008 +0200\n\n    run-command (Windows): Run dashless \"git <cmd>\"\n    \n    We prefer running the dashless form, and POSIX side already does so; we\n    should use it in MinGW's start_command(), too.\n    \n    Signed-off-by: Steffen Prohaska <prohaska@zib.de>\n    Acked-by: Johannes Sixt <johannes.sixt@telecom.at>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"85417","messageId":"10AD8BDA-72E8-437D-8CFC-CDD71BB016F8@zib.de","threadId":"14732","inReplyTo":"20080729052459.GC11947@spearce.org","subject":"Re: [PATCH 2/2 v2] run-command (Windows): Run dashless \"git <cmd>\" (solves part of problem with system_path)","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-07-29T05:51:42Z","receivedAt":"2008-07-29T05:51:42Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jul 29, 2008, at 7:24 AM, Shawn O. Pearce wrote:\n\n> Steffen Prohaska <prohaska@zib.de> wrote:\n>> We prefer running the dashless form, so we should use it in MinGW's\n>> start_command(), too.\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>\n> This feels wrong to me.  fast-import probably won't be adversly\n> impacted by not being able to read /etc/gitconfig, unless the user\n> has set something like core.deltaBaseCacheLimit and is doing an\n> incremental import.  But other non-builtins may be impacted.\n>\n> It feels like we're fixing this in the wrong place.  If the issue\n> is we don't find our installation directory correctly, we should\n> find our installation directory correctly, not work around it by\n> calling builtins through the git wrapper.\n\nFor builtins it is not a work around but a real solution.  The\nsolution however needs to be tested without builtins installed\nin libexec/git-core.   Note that Unix runs builtins using the\ngit wrapper too (see execv_git_cmd), so the patch makes the MinGW\nport behaving more like the Unix version.\n\nMy patch however does not solve the problem for non-builtins, so I agree\nthat we are not fixing the real problem.\n\nMaybe we could do the following:\n\n  - Add a switch RELOCATABLE_PREFIX to the Makefile.\n\n  - If RELOCATABLE_PREFIX is defined, system paths will not be\n    interpreted as is, but a prefix will be computed at runtime.  For\n    example, if git is installed in /some/patch/bin/git, then\n    system_path(\"/etc\") will return \"/some/path/etc\".\n\n  - As Dscho proposed, system_path() needs to be intelligent enough\n    to find the prefix for executables located either in bindir or in\n    gitexecdir.\n\nAdding a switch to the Makefile would make the relocation magic\nexplicit, instead of relying on relative paths.  This has also the\nadvantage that we can just use the default paths for gitexecdir,\ntemplate_dir, ... , instead of overriding them in MINGW section of the\nMakefile.  We only would set RELOCATABLE_PREFIX there.  Actually some\nmore work is needed because prefix is used for defining system paths and\nalso for defining the installation directory.  Maybe we could add\ninstallprefix to explicitly distinguish between the two purposes.\n\n         Steffen\n"},{"id":"85418","messageId":"42EF8777-BABC-4757-AD49-627320F24D39@zib.de","threadId":"14732","inReplyTo":"7v4p69meka.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2 v2] run-command (Windows): Run dashless \"git <cmd>\" (solves part of problem with system_path)","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-07-29T05:55:40Z","receivedAt":"2008-07-29T05:55:40Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jul 29, 2008, at 7:42 AM, Junio C Hamano wrote:\n\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n>\n>> This feels wrong to me.  fast-import probably won't be adversly\n>> impacted by not being able to read /etc/gitconfig, unless the user\n>> has set something like core.deltaBaseCacheLimit and is doing an\n>> incremental import.  But other non-builtins may be impacted.\n>>\n>> It feels like we're fixing this in the wrong place.  If the issue\n>> is we don't find our installation directory correctly, we should\n>> find our installation directory correctly, not work around it by\n>> calling builtins through the git wrapper.\n>>\n>> Though I can see where it may be a good idea to at some point\n>> in the future (git 1.7?) stop creating the redundant builtin\n>> links under libexec/git-core.\n>\n> I agree; that is why I already applied Steffen's original patch with  \n> quite\n> a different justification from the updated one:\n>\n> commit b048b9a803f48d88595877271b53bf9ec400e4ba\n> Author: Steffen Prohaska <prohaska@zib.de>\n> Date:   Mon Jul 28 07:50:28 2008 +0200\n>\n>    run-command (Windows): Run dashless \"git <cmd>\"\n>\n>    We prefer running the dashless form, and POSIX side already does  \n> so; we\n>    should use it in MinGW's start_command(), too.\n\nThanks for reading my mind ;-)  This was the alternative justification\nI had in mind after reading my patch again.\n\n\tSteffen\n"},{"id":"85447","messageId":"alpine.DEB.1.00.0807291310390.4631@eeepc-johanness","threadId":"14732","inReplyTo":"42EF8777-BABC-4757-AD49-627320F24D39@zib.de","subject":"Re: [PATCH 2/2 v2] run-command (Windows): Run dashless \"git <cmd>\" (solves part of problem with system_path)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-29T11:13:08Z","receivedAt":"2008-07-29T11:13:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 29 Jul 2008, Steffen Prohaska wrote:\n\n> On Jul 29, 2008, at 7:42 AM, Junio C Hamano wrote:\n> \n> >   We prefer running the dashless form, and POSIX side already does so; \n> >   we should use it in MinGW's start_command(), too.\n> \n> Thanks for reading my mind ;-)  This was the alternative justification I \n> had in mind after reading my patch again.\n\nWell, given that the justification you gave had the obvious flaw -- which \nyou even pointed out -- that non-builtins are _still_ affected, i.e. that \nyou leave that bug unfixed (but your description purports that you want to \nfix that bug), it would have been wiser to give the alternative \njustification, which makes the commit obviously correct.\n\nCiao,\nDscho\n"},{"id":"85448","messageId":"alpine.DEB.1.00.0807291315140.4631@eeepc-johanness","threadId":"14732","inReplyTo":"10AD8BDA-72E8-437D-8CFC-CDD71BB016F8@zib.de","subject":"Re: [PATCH 2/2 v2] run-command (Windows): Run dashless \"git <cmd>\" (solves part of problem with system_path)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-29T11:18:07Z","receivedAt":"2008-07-29T11:18:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 29 Jul 2008, Steffen Prohaska wrote:\n\n> On Jul 29, 2008, at 7:24 AM, Shawn O. Pearce wrote:\n> \n> > Steffen Prohaska <prohaska@zib.de> wrote:\n> > > We prefer running the dashless form, so we should use it in MinGW's \n> > > start_command(), too.\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> >\n> > This feels wrong to me.  fast-import probably won't be adversly \n> > impacted by not being able to read /etc/gitconfig, unless the user has \n> > set something like core.deltaBaseCacheLimit and is doing an \n> > incremental import.  But other non-builtins may be impacted.\n> >\n> > It feels like we're fixing this in the wrong place.  If the issue is \n> > we don't find our installation directory correctly, we should find our \n> > installation directory correctly, not work around it by calling \n> > builtins through the git wrapper.\n> \n> For builtins it is not a work around but a real solution.\n\nNo, it is not a real solution.  At least not for the goal you stated: \ngetting the correct \"toplevel\" directory.  It is a workaround, because it \navoids the builtins being called as proper programs (which they should be \nperfectly capable of, and which scripts still _are allowed to do_).\n\nHowever, it is a solution.  To a totally other problem, namely \"I would \nlike to be able _not_ to install the builtins into the exec-path\".  Sure, \nyou break a few scripts that rely on the builtins being callable in the \ndash-form, but at least the scripts we ship in git.git are safe.\n\nCiao,\nDscho\n"},{"id":"85449","messageId":"92EF2709-424A-4DA8-AE2C-B54C69F6BB0B@zib.de","threadId":"14732","inReplyTo":"alpine.DEB.1.00.0807291310390.4631@eeepc-johanness","subject":"Re: [PATCH 2/2 v2] run-command (Windows): Run dashless \"git <cmd>\" (solves part of problem with system_path)","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2008-07-29T11:31:14Z","receivedAt":"2008-07-29T11:31:14Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Jul 29, 2008, at 1:13 PM, Johannes Schindelin wrote:\n\n> On Tue, 29 Jul 2008, Steffen Prohaska wrote:\n>\n>> On Jul 29, 2008, at 7:42 AM, Junio C Hamano wrote:\n>>\n>>>  We prefer running the dashless form, and POSIX side already does  \n>>> so;\n>>>  we should use it in MinGW's start_command(), too.\n>>\n>> Thanks for reading my mind ;-)  This was the alternative  \n>> justification I\n>> had in mind after reading my patch again.\n>\n> Well, given that the justification you gave had the obvious flaw --  \n> which\n> you even pointed out -- that non-builtins are _still_ affected, i.e.  \n> that\n> you leave that bug unfixed (but your description purports that you  \n> want to\n> fix that bug), it would have been wiser to give the alternative\n> justification, which makes the commit obviously correct.\n\nWe still need to fix the problem with system_path(), because currently\nwe cannot release Git-1.6.0 on Windows.  That is why I pointed out the\nreal problem we are facing.  The (good) side-effect is that the\nMSYS-codepath is now prepared for 1.7.\n\n\tSteffen\n"}]}