Re: [PATCH 2/2] git-sh-setup: work around Cygwin path handling gotchas

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> I guess you won't be shocked to hear that I don't think this patch is
>> necessary. :-P
> 
> That is more or less irrelevant, not in the sense that what you say is
> irrelevant, but in the sense that something can be worked around in a
> different way alone is not a good reason to reject a patch, if its benefit
> outweigh its costs.

I completely agree.

In case it was not clear, I was not suggesting that the patch be rejected on
the basis that the problem could be worked around in a different way.

> If I speculated in the other message is correct (in short, "In Cygwin
> world, Git is compiled to use POSIX paths and would not work with Windows
> paths."), I think this "problem" is fundamentally un"fix"able.

Yes, Cygwin is essentially just another POSIX target as far as Git is
concerned. Cygwin tries hard to provide a POSIX-like environment, but it is
not possible for it to completely hide the fact that the base OS is not
actually a POSIX system.
 
> And from Cygwin Git, your programs (like $EDITOR and hooks) will get POSIX
> paths.  It is your programs' responsibility to turn them into Windows
> paths if/as necessary.

I would say that this is the only sensible way to proceed.

However, you could imagine adding code to accommodate external windows
programs. If we limit ourselves to the text editor, for example, I could
imagine something like the diff attached below to fix up the C based git
programs. (You would need to make similar changes to the shell and perl
scripts which launch the text editor).

I would not like to see a patch based on this (or any others like it) applied,
since it is going in the wrong direction. (Why do people use Cygwin git rather
than MinGW git and vice versa). Of course, it is not my decision to make ... :-P

>> Anyway, I applied this patch tonight to give it a go. The very first test
>> I tried failed. I've attached the log of the failing test below.
>> Note that it is attempting to use "ssh" to a "host" that ends in ".../C:".
> 
> Of course.  That is one typical symptom that suggests my speculation was
> correct.

Heh, I was surprised that it did as well as it did; after all, it passed 44
out of the 45 tests run (from t7400-submodule-basic.sh). ;-)

> So "I don't think this patch is necessary" is irrelevant, but "This patch
> is harmful; Git on Cygwin is never supposed to use Windows paths" is very
> relevant ;-)

I agree.

ATB,
Ramsay Jones

-- >8 --
diff --git a/editor.c b/editor.c
index d834003..cf36e62 100644
--- a/editor.c
+++ b/editor.c
@@ -1,6 +1,9 @@
 #include "cache.h"
 #include "strbuf.h"
 #include "run-command.h"
+#ifdef __CYGWIN__
+# include <sys/cygwin.h>
+#endif
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -37,6 +40,12 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
+#ifdef __CYGWIN__
+		char win32_path[1024];
+
+		cygwin_conv_to_full_win32_path(path, win32_path);
+		args[1] = win32_path;
+#endif
 
 		if (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))
 			return error("There was a problem with the editor '%s'.",
-- 8< --