[PATCH 1/2] Refactor, adding prepare_git_cmd(const char **argv)

[PATCH 1/2] Refactor, adding prepare_git_cmd(const char **argv)

[Steffen Prohaska]

prepare_git_cmd(const char **argv) adds a first entry "git" to
the array argv.  The new array is allocated on the heap.  It's
the caller's responsibility to release it with free().  The code
was already present in execv_git_cmd() but could not be used from
outside.  Now it can also be called for preparing the command list
in the MinGW codepath in run-command.c.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 exec_cmd.c |    7 ++++++-
 exec_cmd.h |    1 +
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 0ed768d..ce6741e 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -78,7 +78,7 @@ void setup_path(void)
 	strbuf_release(&new_path);
 }
 
-int execv_git_cmd(const char **argv)
+const char **prepare_git_cmd(const char **argv)
 {
 	int argc;
 	const char **nargv;
@@ -91,6 +91,11 @@ int execv_git_cmd(const char **argv)
 	for (argc = 0; argv[argc]; argc++)
 		nargv[argc + 1] = argv[argc];
 	nargv[argc + 1] = NULL;
+	return nargv;
+}
+
+int execv_git_cmd(const char **argv) {
+	const char **nargv = prepare_git_cmd(argv);
 	trace_argv_printf(nargv, "trace: exec:");
 
 	/* execvp() can only ever return if it fails */
diff --git a/exec_cmd.h b/exec_cmd.h
index 0c46cd5..594f961 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -5,6 +5,7 @@ extern void git_set_argv_exec_path(const char *exec_path);
 extern void git_set_argv0_path(const char *path);
 extern const char* git_exec_path(void);
 extern void setup_path(void);
+extern const char **prepare_git_cmd(const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 extern int execl_git_cmd(const char *cmd, ...);
 extern const char *system_path(const char *path);
-- 
1.6.0.rc0.79.gb0320

[Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless "git <cmd>"

[Steffen Prohaska]

This might solve 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 be read.

The patch below might fix the problem by always calling 'bin/git'
for builtin commands.  The computation in system_path() would
always start from 'bin' and thus yields predictable results.  I
am not sure however if it fully solves the problem because other
code paths might run the dashed forms directly.

I think the only way to verify correctness would be to stop
installing the dashed forms for builtins.  If they were not
installed they could not be called.  The only entry point for all
builtins would be 'bin/git'.  I don't think we want to stop
installing the dashed forms right away.

So what shall we do?

	Steffen

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

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 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

Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless "git <cmd>"

[Junio C Hamano]

Steffen Prohaska <prohaska@zib.de> writes:

> ...  It is either used as
> 'bin/git' (1 directory) or as 'libexec/git-core/git-*'
> (2 directories).

I thought Hannes already fixed that one; we shouldn't have the latter. 

Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless "git <cmd>"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Steffen Prohaska <prohaska@zib.de> writes:
>
>> ...  It is either used as
>> 'bin/git' (1 directory) or as 'libexec/git-core/git-*'
>> (2 directories).
>
> I thought Hannes already fixed that one; we shouldn't have the latter. 

Oops, I misread your message.  You are worried about the builtins.  Sorry.

Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless "git <cmd>"

[Johannes Schindelin]

Hi,

On Mon, 28 Jul 2008, Steffen Prohaska wrote:

> This might solve 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 be read.

I seem to recall that I already suggested stripping 
"/libexec/git-core/<name>" if it is found, and fall back to 
stripping one directory level (catching "/bin/<name>").

IMHO system_path() should really be that intelligent.

(Of course, the way it is set up now, the _caller_ of git_set_argv0_path() 
has to do the intelligent thing...)

Ciao,
Dscho

Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless "git <cmd>"

[Johannes Sixt]

Zitat von Steffen Prohaska <prohaska@zib.de>:

> This might solve 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 be read.
>
> The patch below might fix the problem by always calling 'bin/git'
> for builtin commands.  The computation in system_path() would
> always start from 'bin' and thus yields predictable results.  I
> am not sure however if it fully solves the problem because other
> code paths might run the dashed forms directly.

This paragraph should go into the commit message.

> I think the only way to verify correctness would be to stop
> installing the dashed forms for builtins.  If they were not
> installed they could not be called.  The only entry point for all
> builtins would be 'bin/git'.  I don't think we want to stop
> installing the dashed forms right away.
>
> So what shall we do?

Your patches make a lot of sense.

> -- 8< --
> We prefer running the dashless form, so we should use it in
> MinGW's start_command(), too.
>
> Signed-off-by: Steffen Prohaska <prohaska@zib.de>

Acked-by: Johannes Sixt <johannes.sixt@telecom.at>

-- Hannes

Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless "git <cmd>"

[Steffen Prohaska]

On Jul 28, 2008, at 10:37 PM, Johannes Sixt wrote:

> Zitat von Steffen Prohaska <prohaska@zib.de>:
>
>> This might solve 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 be read.
>>
>> The patch below might fix the problem by always calling 'bin/git'
>> for builtin commands.  The computation in system_path() would
>> always start from 'bin' and thus yields predictable results.  I
>> am not sure however if it fully solves the problem because other
>> code paths might run the dashed forms directly.
>
> This paragraph should go into the commit message.

I'll send an updated patch including a commit message defending
the change more offensively.


>> I think the only way to verify correctness would be to stop
>> installing the dashed forms for builtins.  If they were not
>> installed they could not be called.  The only entry point for all
>> builtins would be 'bin/git'.  I don't think we want to stop
>> installing the dashed forms right away.
>>
>> So what shall we do?
>
> Your patches make a lot of sense.

Unfortunately, I am sure that they do not solve the problem fully, even
if we removed the dashed forms completely.  I'll send a second patch  
that
modifies the Makefile to stop installing the BUILT_INS.  It might be
useful for debugging.

	Steffen

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

[Steffen Prohaska]

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 <prohaska@zib.de>
Acked-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 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

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

[Steffen Prohaska]

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 <prohaska@zib.de>
Acked-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 run-command.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

My previous mail was in the wrong thread.  Apologies.  This one
will be correctly threaded.

    Steffen

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

[FOR DEBUGGING] Stop installing BUILT_INS

[Steffen Prohaska]

---
 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

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

[Shawn O. Pearce]

Steffen Prohaska <prohaska@zib.de> 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.

Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless "git <cmd>"

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> Zitat von Steffen Prohaska <prohaska@zib.de>:
> ...
>> The patch below might fix the problem by always calling 'bin/git'
>> for builtin commands.  The computation in system_path() would
>> always start from 'bin' and thus yields predictable results.  I
>> am not sure however if it fully solves the problem because other
>> code paths might run the dashed forms directly.
>
> This paragraph should go into the commit message.

> ...
> Your patches make a lot of sense.

I was almost going to suggest doing this everywhere not just on Windows,
but execv_git_cmd() on the POSIX side already runs "git" wrapper, so this
patch makes them in line, finally.

>> -- 8< --
>> We prefer running the dashless form, so we should use it in
>> MinGW's start_command(), too.
>>
>> Signed-off-by: Steffen Prohaska <prohaska@zib.de>
>
> Acked-by: Johannes Sixt <johannes.sixt@telecom.at>
>
> -- Hannes

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

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> 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.

I agree; that is why I already applied Steffen's original patch with quite
a different justification from the updated one:

commit b048b9a803f48d88595877271b53bf9ec400e4ba
Author: Steffen Prohaska <prohaska@zib.de>
Date:   Mon Jul 28 07:50:28 2008 +0200

    run-command (Windows): Run dashless "git <cmd>"
    
    We prefer running the dashless form, and POSIX side already does so; we
    should use it in MinGW's start_command(), too.
    
    Signed-off-by: Steffen Prohaska <prohaska@zib.de>
    Acked-by: Johannes Sixt <johannes.sixt@telecom.at>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

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

[Steffen Prohaska]

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

> Steffen Prohaska <prohaska@zib.de> 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.

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

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

[Steffen Prohaska]

On Jul 29, 2008, at 7:42 AM, Junio C Hamano wrote:

> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> 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.
>
> I agree; that is why I already applied Steffen's original patch with  
> quite
> a different justification from the updated one:
>
> commit b048b9a803f48d88595877271b53bf9ec400e4ba
> Author: Steffen Prohaska <prohaska@zib.de>
> Date:   Mon Jul 28 07:50:28 2008 +0200
>
>    run-command (Windows): Run dashless "git <cmd>"
>
>    We prefer running the dashless form, and POSIX side already does  
> so; we
>    should use it in MinGW's start_command(), too.

Thanks for reading my mind ;-)  This was the alternative justification
I had in mind after reading my patch again.

	Steffen

Re: [Fundamental problem with relative system paths] [PATCH 2/2] run-command (Windows): Run dashless "git <cmd>"

[Johannes Sixt]

Zitat von Junio C Hamano <gitster@pobox.com>:

> Johannes Sixt <johannes.sixt@telecom.at> writes:
>
> > Zitat von Steffen Prohaska <prohaska@zib.de>:
> > ...
> >> The patch below might fix the problem by always calling 'bin/git'
> >> for builtin commands.  The computation in system_path() would
> >> always start from 'bin' and thus yields predictable results.  I
> >> am not sure however if it fully solves the problem because other
> >> code paths might run the dashed forms directly.
> >
> > This paragraph should go into the commit message.
>
> > ...
> > Your patches make a lot of sense.
>
> I was almost going to suggest doing this everywhere not just on Windows,
> but execv_git_cmd() on the POSIX side already runs "git" wrapper, so this
> patch makes them in line, finally.

For this reason I'm in favor of these patches. I didn't run the full test suite
with them, yet, (you know, that takes a while on Windows), but "make *clone*
*fetch* *pack*" worked out OK.

-- Hannes

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

[Johannes Schindelin]

Hi,

On Tue, 29 Jul 2008, Steffen Prohaska wrote:

> On Jul 29, 2008, at 7:42 AM, Junio C Hamano wrote:
> 
> >   We prefer running the dashless form, and POSIX side already does so; 
> >   we should use it in MinGW's start_command(), too.
> 
> Thanks for reading my mind ;-)  This was the alternative justification I 
> had in mind after reading my patch again.

Well, given that the justification you gave had the obvious flaw -- which 
you even pointed out -- that non-builtins are _still_ affected, i.e. that 
you leave that bug unfixed (but your description purports that you want to 
fix that bug), it would have been wiser to give the alternative 
justification, which makes the commit obviously correct.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

On Tue, 29 Jul 2008, Steffen Prohaska wrote:

> On Jul 29, 2008, at 7:24 AM, Shawn O. Pearce wrote:
> 
> > Steffen Prohaska <prohaska@zib.de> 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.
> 
> 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

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

[Steffen Prohaska]

On Jul 29, 2008, at 1:13 PM, Johannes Schindelin wrote:

> On Tue, 29 Jul 2008, Steffen Prohaska wrote:
>
>> On Jul 29, 2008, at 7:42 AM, Junio C Hamano wrote:
>>
>>>  We prefer running the dashless form, and POSIX side already does  
>>> so;
>>>  we should use it in MinGW's start_command(), too.
>>
>> Thanks for reading my mind ;-)  This was the alternative  
>> justification I
>> had in mind after reading my patch again.
>
> Well, given that the justification you gave had the obvious flaw --  
> which
> you even pointed out -- that non-builtins are _still_ affected, i.e.  
> that
> you leave that bug unfixed (but your description purports that you  
> want to
> fix that bug), it would have been wiser to give the alternative
> justification, which makes the commit obviously correct.

We still need to fix the problem with system_path(), because currently
we cannot release Git-1.6.0 on Windows.  That is why I pointed out the
real problem we are facing.  The (good) side-effect is that the
MSYS-codepath is now prepared for 1.7.

	Steffen