[PATCH 2/2 v2] run-command (Windows): Run dashless "git <cmd>" (solves part of problem with system_path)

We prefer running the dashless form, so we should use it in MinGW's
start_command(), too.

This solves a fundamental problem we have with the computation of system
directories based on relative paths in combination with the new
gitexecpath 'libexec/git-core'.  The problem is that the program 'git'
is hardlinked to directories with different depth.  It is either used as
'bin/git' (1 directory) or as 'libexec/git-core/git-*' (2 directories).
Thus, using the same relative path in system_path() yields different
results when starting from the two locations.  I recognized the problem
because /etc/gitconfig is no longer read.

This commit fixes the problem by always calling 'bin/git' for builtin
commands.  The computation in system_path() thus always starts from
'bin' and yields predictable results.  This is however only part of a
full solution to the problem outlined above.  Remaining problems are:

 - Other code paths might run the dashed forms directly.

 - We have non-builtins that are implemented in C, e.g. fast-import.c.
   These non-builtins will still compute wrong paths.

Signed-off-by: Steffen Prohaska <redacted>
Acked-by: Johannes Sixt <redacted>
---
 run-command.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/run-command.c b/run-command.c
index 6e29fdf..a3b28a6 100644
--- a/run-command.c
+++ b/run-command.c

@@ -119,9 +119,8 @@ int start_command(struct child_process *cmd)
 	}
 #else
 	int s0 = -1, s1 = -1, s2 = -1;	/* backups of stdin, stdout, stderr */
-	const char *sargv0 = cmd->argv[0];
+	const char **sargv = cmd->argv;
 	char **env = environ;
-	struct strbuf git_cmd;
 
 	if (cmd->no_stdin) {
 		s0 = dup(0);

@@ -165,9 +164,7 @@ int start_command(struct child_process *cmd)
 	}
 
 	if (cmd->git_cmd) {
-		strbuf_init(&git_cmd, 0);
-		strbuf_addf(&git_cmd, "git-%s", cmd->argv[0]);
-		cmd->argv[0] = git_cmd.buf;
+		cmd->argv = prepare_git_cmd(cmd->argv);
 	}
 
 	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);

@@ -175,9 +172,9 @@ int start_command(struct child_process *cmd)
 	if (cmd->env)
 		free_environ(env);
 	if (cmd->git_cmd)
-		strbuf_release(&git_cmd);
+		free(cmd->argv);
 
-	cmd->argv[0] = sargv0;
+	cmd->argv = sargv;
 	if (s0 >= 0)
 		dup2(s0, 0), close(s0);
 	if (s1 >= 0)

-- 
1.6.0.rc0.79.gb0320

---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index b2bc0ef..e416ec7 100644
--- a/Makefile
+++ b/Makefile

@@ -1366,7 +1366,7 @@ endif
 		ln -f "$$bindir/git$X" "$$execdir/git$X" || \
 		cp "$$bindir/git$X" "$$execdir/git$X"; \
 	fi && \
-	{ $(foreach p,$(BUILT_INS), $(RM) "$$execdir/$p" && ln "$$execdir/git$X" "$$execdir/$p" ;) } && \
+	{ $(foreach p,$(BUILT_INS), $(RM) "$$execdir/$p" ;) } && \
 	$(RM) "$$execdir/git$X" && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 

-- 
1.6.0.rc0.79.gb0320

Steffen Prohaska [off-list ref] wrote:

We prefer running the dashless form, so we should use it in MinGW's
start_command(), too.

...

 - We have non-builtins that are implemented in C, e.g. fast-import.c.
   These non-builtins will still compute wrong paths.

This feels wrong to me.  fast-import probably won't be adversly
impacted by not being able to read /etc/gitconfig, unless the user
has set something like core.deltaBaseCacheLimit and is doing an
incremental import.  But other non-builtins may be impacted.

It feels like we're fixing this in the wrong place.  If the issue
is we don't find our installation directory correctly, we should
find our installation directory correctly, not work around it by
calling builtins through the git wrapper.

Though I can see where it may be a good idea to at some point
in the future (git 1.7?) stop creating the redundant builtin
links under libexec/git-core.

-- 
Shawn.

On Jul 29, 2008, at 7:24 AM, Shawn O. Pearce wrote:

Steffen Prohaska [off-list ref] wrote:

quoted

We prefer running the dashless form, so we should use it in MinGW's
start_command(), too.

...

quoted

- We have non-builtins that are implemented in C, e.g. fast-import.c.
  These non-builtins will still compute wrong paths.

This feels wrong to me.  fast-import probably won't be adversly
impacted by not being able to read /etc/gitconfig, unless the user
has set something like core.deltaBaseCacheLimit and is doing an
incremental import.  But other non-builtins may be impacted.

It feels like we're fixing this in the wrong place.  If the issue
is we don't find our installation directory correctly, we should
find our installation directory correctly, not work around it by
calling builtins through the git wrapper.

For builtins it is not a work around but a real solution.  The
solution however needs to be tested without builtins installed
in libexec/git-core.   Note that Unix runs builtins using the
git wrapper too (see execv_git_cmd), so the patch makes the MinGW
port behaving more like the Unix version.

My patch however does not solve the problem for non-builtins, so I agree
that we are not fixing the real problem.

Maybe we could do the following:

  - Add a switch RELOCATABLE_PREFIX to the Makefile.

  - If RELOCATABLE_PREFIX is defined, system paths will not be
    interpreted as is, but a prefix will be computed at runtime.  For
    example, if git is installed in /some/patch/bin/git, then
    system_path("/etc") will return "/some/path/etc".

  - As Dscho proposed, system_path() needs to be intelligent enough
    to find the prefix for executables located either in bindir or in
    gitexecdir.

Adding a switch to the Makefile would make the relocation magic
explicit, instead of relying on relative paths.  This has also the
advantage that we can just use the default paths for gitexecdir,
template_dir, ... , instead of overriding them in MINGW section of the
Makefile.  We only would set RELOCATABLE_PREFIX there.  Actually some
more work is needed because prefix is used for defining system paths and
also for defining the installation directory.  Maybe we could add
installprefix to explicitly distinguish between the two purposes.

         Steffen

Hi,

On Tue, 29 Jul 2008, Steffen Prohaska wrote:

On Jul 29, 2008, at 7:24 AM, Shawn O. Pearce wrote:

quoted

Steffen Prohaska [off-list ref] wrote:

quoted

We prefer running the dashless form, so we should use it in MinGW's 
start_command(), too.

...

quoted

- We have non-builtins that are implemented in C, e.g. fast-import.c.
  These non-builtins will still compute wrong paths.

This feels wrong to me.  fast-import probably won't be adversly 
impacted by not being able to read /etc/gitconfig, unless the user has 
set something like core.deltaBaseCacheLimit and is doing an 
incremental import.  But other non-builtins may be impacted.

It feels like we're fixing this in the wrong place.  If the issue is 
we don't find our installation directory correctly, we should find our 
installation directory correctly, not work around it by calling 
builtins through the git wrapper.

For builtins it is not a work around but a real solution.

No, it is not a real solution.  At least not for the goal you stated: 
getting the correct "toplevel" directory.  It is a workaround, because it 
avoids the builtins being called as proper programs (which they should be 
perfectly capable of, and which scripts still _are allowed to do_).

However, it is a solution.  To a totally other problem, namely "I would 
like to be able _not_ to install the builtins into the exec-path".  Sure, 
you break a few scripts that rely on the builtins being callable in the 
dash-form, but at least the scripts we ship in git.git are safe.

Ciao,
Dscho