Re: [PATCH] editor: use canonicalized absolute path
Duy Nguyen writes: > The idea is the same, but my patch is a bit different (use of realpath > instead of real_path, I didn't remember git has real_path). I'm fine > with Ram being the author. Thanks, both of you, for clarification. >> Compared to not being able to edit, it may be a small price to pay >> for those who do need to suffer the broken editor, but the patch >> makes those who do not need this workaround to pay the price. > > Does looking at the edited file's path happen often? I have never done > that before. I ask because in order to avoid the price for those > users, the code could get a little bit more complicated (detecting if > the given relative path traverse backward outside a symlink..). To me > it seems like a good trade off in favor of simpler code. Yeah, I was being my usual cautious self, as somebody has to play that role. I think the code as-is would be an OK trade off. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] editor: use canonicalized absolute path
On Mon, Jul 29, 2013 at 07:56:58AM -0700, Junio C Hamano wrote:
> > diff --git a/editor.c b/editor.c
> > index 27bdecd..0abbd8d 100644
> > --- a/editor.c
> > +++ b/editor.c
> > @@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf
> > *buffer, const char *const *en
> > return error("Terminal is dumb, but EDITOR unset");
> >
> > if (strcmp(editor, ":")) {
> > - const char *args[] = { editor, path, NULL };
> > + const char *args[] = { editor, real_path(path), NULL };
>
> While I am not fundamentally opposed to add a workaround for bugs in
> a popular tool many people use, I am a bit uneasy about this change.
>
> For editors that are not broken, this could be an annoying
> regression, isn't it? When the user asks "What is the path of the
> file I am editing?" to the editor (i.e. an equivalent of \C-x\C-b),
> the updated code will start spewing a long full-path from the root
> directory, while we used to give a relative path that is short,
> sweet and more in line with the context of user's work.
>
> Compared to not being able to edit, it may be a small price to pay
> for those who do need to suffer the broken editor, but the patch
> makes those who do not need this workaround to pay the price.
>
How about something like this? For standard setups, even if you have
symlink in cwd, it won't kick in because the given path is usually
.git/something (i.e. no ".." component)
-- 8< --
diff --git a/editor.c b/editor.c
index 27bdecd..02bf42c 100644
--- a/editor.c
+++ b/editor.c
@@ -37,10 +37,17 @@ int launch_editor(const char *path, struct strbuf *buffer,
const char *const *en
return error("Terminal is dumb, but EDITOR unset");
if (strcmp(editor, ":")) {
+ char cwd[PATH_MAX];
const char *args[] = { editor, path, NULL };
struct child_process p;
int ret, sig;
+ /* emacs workaround */
+ if (getcwd(cwd, PATH_MAX) &&
+ strcmp(real_path(cwd), cwd) &&
+ strstr(path, "../"))
+ args[1] = real_path(path);
+
memset(&p, 0, sizeof(p));
p.argv = args;
p.env = env;
-- 8<--
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] editor: use canonicalized absolute path
On Mon, Jul 29, 2013 at 9:56 PM, Junio C Hamano wrote:
>> Co-authored-by: Nguyễn Thái Ngọc Duy
>> Signed-off-by: Ramkumar Ramachandra
>
> That's a bit strange---the patch text looks like the "how about
> this" patch Duy posted earlier. Shouldn't it be From: Duy with
> S-o-b: by two of you instead?
The idea is the same, but my patch is a bit different (use of realpath
instead of real_path, I didn't remember git has real_path). I'm fine
with Ram being the author.
>> diff --git a/editor.c b/editor.c
>> index 27bdecd..0abbd8d 100644
>> --- a/editor.c
>> +++ b/editor.c
>> @@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf *buffer,
>> const char *const *en
>> return error("Terminal is dumb, but EDITOR unset");
>>
>> if (strcmp(editor, ":")) {
>> - const char *args[] = { editor, path, NULL };
>> + const char *args[] = { editor, real_path(path), NULL };
>
> While I am not fundamentally opposed to add a workaround for bugs in
> a popular tool many people use, I am a bit uneasy about this change.
>
> For editors that are not broken, this could be an annoying
> regression, isn't it? When the user asks "What is the path of the
> file I am editing?" to the editor (i.e. an equivalent of \C-x\C-b),
> the updated code will start spewing a long full-path from the root
> directory, while we used to give a relative path that is short,
> sweet and more in line with the context of user's work.
>
> Compared to not being able to edit, it may be a small price to pay
> for those who do need to suffer the broken editor, but the patch
> makes those who do not need this workaround to pay the price.
Does looking at the edited file's path happen often? I have never done
that before. I ask because in order to avoid the price for those
users, the code could get a little bit more complicated (detecting if
the given relative path traverse backward outside a symlink..). To me
it seems like a good trade off in favor of simpler code.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] editor: use canonicalized absolute path
Ramkumar Ramachandra writes: > Junio C Hamano wrote: >> Ramkumar Ramachandra writes: >>> Junio C Hamano wrote: That's a bit strange---the patch text looks like the "how about this" patch Duy posted earlier. Shouldn't it be From: Duy with S-o-b: by two of you instead? >>> >>> Feel free to amend as you see fit, as always. >> >> I was asking what is "correct", without which I cannot "feel free" >> to do anything, and your answer is not helping. > > I don't have a strong opinion either way. There is no opinion involved. Maybe the word "correct" had a wrong connotation, but what I needed to find out was a true provenance of the change, as that is what the S-o-b chain records. I did not know, and I needed to find out, if the patch in question was what you came up independently without looking at Duy's patch (which is very understandable as you two were going back and forth digging the issue on the list), or you submitted his patch after tidying it up. It appears from your description below that it was an independent work, so that is what I'll queue. Thanks. > Fwiw, I posted the original > with me as the author because I discovered it, dug through the emacs > sources for hours to find the exact bug, and contacted emacs-devel > about it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] editor: use canonicalized absolute path
Junio C Hamano wrote: > Ramkumar Ramachandra writes: >> Junio C Hamano wrote: >>> That's a bit strange---the patch text looks like the "how about >>> this" patch Duy posted earlier. Shouldn't it be From: Duy with >>> S-o-b: by two of you instead? >> >> Feel free to amend as you see fit, as always. > > I was asking what is "correct", without which I cannot "feel free" > to do anything, and your answer is not helping. I don't have a strong opinion either way. Fwiw, I posted the original with me as the author because I discovered it, dug through the emacs sources for hours to find the exact bug, and contacted emacs-devel about it. But no, I do not think there is anything "correct" about it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] editor: use canonicalized absolute path
Ramkumar Ramachandra writes: > Junio C Hamano wrote: >> That's a bit strange---the patch text looks like the "how about >> this" patch Duy posted earlier. Shouldn't it be From: Duy with >> S-o-b: by two of you instead? > > Feel free to amend as you see fit, as always. I was asking what is "correct", without which I cannot "feel free" to do anything, and your answer is not helping. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] editor: use canonicalized absolute path
Junio C Hamano wrote: > That's a bit strange---the patch text looks like the "how about > this" patch Duy posted earlier. Shouldn't it be From: Duy with > S-o-b: by two of you instead? Feel free to amend as you see fit, as always. > For editors that are not broken, this could be an annoying > regression, isn't it? When the user asks "What is the path of the > file I am editing?" to the editor (i.e. an equivalent of \C-x\C-b), > the updated code will start spewing a long full-path from the root > directory, while we used to give a relative path that is short, > sweet and more in line with the context of user's work. Does it matter for COMMIT_EDITMSG or other files in $GITDIR? If you're concerned about it, we can change the logic to: real_path() if path-is-relative to avoid mangling the path in the non-submodules case; in the submodules case, it will use an absolute-path, as before. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] editor: use canonicalized absolute path
Ramkumar Ramachandra writes:
> By improving the relative_path() algorithm, e02ca72 (path.c: refactor
> relative_path(), not only strip prefix, 2013-06-25) uncovered a latent
> bug. While most editor applications like cat and vim handle
> non-canonicalized relative paths fine, emacs does not. This is due to a
> long-standing bug in emacs, where it refuses to resolve symlinks in the
> supplied path:
>
> #!/bin/sh
> mkdir z z/a z/b
> echo moodle >z/a/file
> ln -s z/b
> cd b
> emacs ../a/file # fail: opens /tmp/a/file
Somewhat puzzling. Perhaps you want to add "cd /tmp" at the very
beginning of the script to clarify, and s/opens/attempts to open/
to avoid confusing a little panda brain.
>
> Even if emacs were to be patched to fix this bug now, we still need to
> support users running older versions.
>
> Co-authored-by: Nguyễn Thái Ngọc Duy
> Signed-off-by: Ramkumar Ramachandra
That's a bit strange---the patch text looks like the "how about
this" patch Duy posted earlier. Shouldn't it be From: Duy with
S-o-b: by two of you instead?
> diff --git a/editor.c b/editor.c
> index 27bdecd..0abbd8d 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf *buffer,
> const char *const *en
> return error("Terminal is dumb, but EDITOR unset");
>
> if (strcmp(editor, ":")) {
> - const char *args[] = { editor, path, NULL };
> + const char *args[] = { editor, real_path(path), NULL };
While I am not fundamentally opposed to add a workaround for bugs in
a popular tool many people use, I am a bit uneasy about this change.
For editors that are not broken, this could be an annoying
regression, isn't it? When the user asks "What is the path of the
file I am editing?" to the editor (i.e. an equivalent of \C-x\C-b),
the updated code will start spewing a long full-path from the root
directory, while we used to give a relative path that is short,
sweet and more in line with the context of user's work.
Compared to not being able to edit, it may be a small price to pay
for those who do need to suffer the broken editor, but the patch
makes those who do not need this workaround to pay the price.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] editor: use canonicalized absolute path
On Sun, Jul 28, 2013 at 11:59 PM, Ramkumar Ramachandra
wrote:
> By improving the relative_path() algorithm, e02ca72 (path.c: refactor
> relative_path(), not only strip prefix, 2013-06-25) uncovered a latent
> bug. While most editor applications like cat and vim handle
> non-canonicalized relative paths fine, emacs does not. This is due to a
> long-standing bug in emacs, where it refuses to resolve symlinks in the
> supplied path:
>
> #!/bin/sh
> mkdir z z/a z/b
> echo moodle >z/a/file
> ln -s z/b
> cd b
> emacs ../a/file # fail: opens /tmp/a/file
>
> Even if emacs were to be patched to fix this bug now, we still need to
> support users running older versions.
We used to have workaround for external programs, e.g. 35ce862 (pager:
Work around window resizing bug in 'less' - 2007-01-24), so I don't
think this is a problem.
> Co-authored-by: Nguyễn Thái Ngọc Duy
> Signed-off-by: Ramkumar Ramachandra
> ---
> Urgent candidate for maint. I wrote to emacs-devel, but nobody seems
> to be interested; the sources are horrendously unmaintainable, and
> the project should die soon.
Hey, I don't want to throw away years of training my fingers to use
emacs. It can't die until there is a viable fork :)
> diff --git a/editor.c b/editor.c
> index 27bdecd..0abbd8d 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf *buffer,
> const char *const *en
> return error("Terminal is dumb, but EDITOR unset");
>
> if (strcmp(editor, ":")) {
> - const char *args[] = { editor, path, NULL };
> + const char *args[] = { editor, real_path(path), NULL };
> struct child_process p;
> int ret, sig;
real_path() returns a static buffer, which could be overwritten by the
next real_path() call. I checked and I think that won't happen. So the
patch looks good.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] editor: use canonicalized absolute path
By improving the relative_path() algorithm, e02ca72 (path.c: refactor
relative_path(), not only strip prefix, 2013-06-25) uncovered a latent
bug. While most editor applications like cat and vim handle
non-canonicalized relative paths fine, emacs does not. This is due to a
long-standing bug in emacs, where it refuses to resolve symlinks in the
supplied path:
#!/bin/sh
mkdir z z/a z/b
echo moodle >z/a/file
ln -s z/b
cd b
emacs ../a/file # fail: opens /tmp/a/file
Even if emacs were to be patched to fix this bug now, we still need to
support users running older versions.
Co-authored-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Ramkumar Ramachandra
---
Urgent candidate for maint. I wrote to emacs-devel, but nobody seems
to be interested; the sources are horrendously unmaintainable, and
the project should die soon.
editor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/editor.c b/editor.c
index 27bdecd..0abbd8d 100644
--- a/editor.c
+++ b/editor.c
@@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf *buffer,
const char *const *en
return error("Terminal is dumb, but EDITOR unset");
if (strcmp(editor, ":")) {
- const char *args[] = { editor, path, NULL };
+ const char *args[] = { editor, real_path(path), NULL };
struct child_process p;
int ret, sig;
--
1.8.4.rc0.2.g7ba4592
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
