Ramsay Jones [off-list ref] 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.
The more important message for you to respond to is this one:
Subject: Re: Git commit path vs rebase path
To: Ramsay Jones [off-list ref]
Cc: Johannes Sixt [off-list ref],
Steven Penny [off-list ref],
Junio C Hamano [off-list ref], git@vger.kernel.org
Date: Thu, 17 May 2012 12:19:33 -0700
Message-ID: [off-list ref]
My concerns about this patch include:
- the additional fork+exec overhead associated with calling cygpath.
I'm not actually claiming there is any substantial increase; I
haven't tried it, so I don't know how "hot" the pwd() function is.
- this is a "big hammer" which will affect much more code that is
required to fix this problem.
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.
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.
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.
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 ;-)
On Thu, May 17, 2012 at 9:34 PM, Junio C Hamano wrote:
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 think this is correct. These program should be accepting Cygwin paths and are
not for obvious reason, they exist outside the the Cygwin world.
For now I will do something like this
editor(){
cygpath -m "$1" | xargs notepad
}
export -f editor
git config core.editor editor
Junio C Hamano wrote:
Ramsay Jones [off-list ref] writes:
quoted
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
quoted
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< --