Re: [PATCH] editor: use canonicalized absolute path

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 29, 2013, 14:56 UTC
Message-ID: <7v8v0p2z45.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1375030782-13339-1-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 13 quoted lines

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

Show 6 quoted lines

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

Show 10 quoted lines

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