[PATCH] editor: use canonicalized absolute path

[PATCH] editor: use canonicalized absolute path

[Ramkumar Ramachandra]

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 <pclouds@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 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

Re: [PATCH] editor: use canonicalized absolute path

[Duy Nguyen]

On Sun, Jul 28, 2013 at 11:59 PM, Ramkumar Ramachandra
<artagnon@gmail.com> 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 <pclouds@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  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

Re: [PATCH] editor: use canonicalized absolute path

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> 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 <pclouds@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

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.

Re: [PATCH] editor: use canonicalized absolute path

[Ramkumar Ramachandra]

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.

Re: [PATCH] editor: use canonicalized absolute path

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> 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.

Re: [PATCH] editor: use canonicalized absolute path

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> 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.

Re: [PATCH] editor: use canonicalized absolute path

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Ramkumar Ramachandra <artagnon@gmail.com> 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.

Re: [PATCH] editor: use canonicalized absolute path

[Duy Nguyen]

On Mon, Jul 29, 2013 at 9:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Co-authored-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>
> 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

Re: [PATCH] editor: use canonicalized absolute path

[Duy Nguyen]

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

Re: [PATCH] editor: use canonicalized absolute path

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> 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.