{"thread":{"id":"34554","subject":"[PATCH] editor: use canonicalized absolute path","startedAt":"2013-07-28T16:59:42Z","lastAt":"2013-07-30T13:48:49Z","messageCount":10,"participants":["Ramkumar Ramachandra","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224204","messageId":"1375030782-13339-1-git-send-email-artagnon@gmail.com","threadId":"34554","inReplyTo":null,"subject":"[PATCH] editor: use canonicalized absolute path","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-28T16:59:42Z","receivedAt":"2013-07-28T16:59:42Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"By improving the relative_path() algorithm, e02ca72 (path.c: refactor\nrelative_path(), not only strip prefix, 2013-06-25) uncovered a latent\nbug. While most editor applications like cat and vim handle\nnon-canonicalized relative paths fine, emacs does not. This is due to a\nlong-standing bug in emacs, where it refuses to resolve symlinks in the\nsupplied path:\n\n #!/bin/sh\n mkdir z z/a z/b\n echo moodle >z/a/file\n ln -s z/b\n cd b\n emacs ../a/file # fail: opens /tmp/a/file\n\nEven if emacs were to be patched to fix this bug now, we still need to\nsupport users running older versions.\n\nCo-authored-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Ramkumar Ramachandra \n---\n Urgent candidate for maint. I wrote to emacs-devel, but nobody seems\n to be interested; the sources are horrendously unmaintainable, and\n the project should die soon.\n\n editor.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/editor.c b/editor.c\nindex 27bdecd..0abbd8d 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n \n \tif (strcmp(editor, \":\")) {\n-\t\tconst char *args[] = { editor, path, NULL };\n+\t\tconst char *args[] = { editor, real_path(path), NULL };\n \t\tstruct child_process p;\n \t\tint ret, sig;\n \n-- \n1.8.4.rc0.2.g7ba4592\n"},{"id":"224216","messageId":"CACsJy8CQiESBsZvRB-Gs17Jg4Lvu-gJLUckV0njeTj+EwnTrig@mail.gmail.com","threadId":"34554","inReplyTo":"1375030782-13339-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] editor: use canonicalized absolute path","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-07-29T10:03:49Z","receivedAt":"2013-07-29T10:03:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Jul 28, 2013 at 11:59 PM, Ramkumar Ramachandra\n wrote:\n> By improving the relative_path() algorithm, e02ca72 (path.c: refactor\n> relative_path(), not only strip prefix, 2013-06-25) uncovered a latent\n> bug. While most editor applications like cat and vim handle\n> non-canonicalized relative paths fine, emacs does not. This is due to a\n> long-standing bug in emacs, where it refuses to resolve symlinks in the\n> supplied path:\n>\n> #!/bin/sh\n> mkdir z z/a z/b\n> echo moodle >z/a/file\n> ln -s z/b\n> cd b\n> emacs ../a/file # fail: opens /tmp/a/file\n>\n> Even if emacs were to be patched to fix this bug now, we still need to\n> support users running older versions.\n\nWe used to have workaround for external programs, e.g. 35ce862 (pager:\nWork around window resizing bug in 'less' - 2007-01-24), so I don't\nthink this is a problem.\n\n> Co-authored-by: Nguyễn Thái Ngọc Duy \n> Signed-off-by: Ramkumar Ramachandra \n> ---\n> Urgent candidate for maint. I wrote to emacs-devel, but nobody seems\n> to be interested; the sources are horrendously unmaintainable, and\n> the project should die soon.\n\nHey, I don't want to throw away years of training my fingers to use\nemacs. It can't die until there is a viable fork :)\n\n> diff --git a/editor.c b/editor.c\n> index 27bdecd..0abbd8d 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n> return error(\"Terminal is dumb, but EDITOR unset\");\n>\n> if (strcmp(editor, \":\")) {\n> - const char *args[] = { editor, path, NULL };\n> + const char *args[] = { editor, real_path(path), NULL };\n> struct child_process p;\n> int ret, sig;\n\nreal_path() returns a static buffer, which could be overwritten by the\nnext real_path() call. I checked and I think that won't happen. So the\npatch looks good.\n-- \nDuy\n"},{"id":"224220","messageId":"7v8v0p2z45.fsf@alter.siamese.dyndns.org","threadId":"34554","inReplyTo":"1375030782-13339-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] editor: use canonicalized absolute path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-29T14:56:58Z","receivedAt":"2013-07-29T14:56:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> By improving the relative_path() algorithm, e02ca72 (path.c: refactor\n> relative_path(), not only strip prefix, 2013-06-25) uncovered a latent\n> bug. While most editor applications like cat and vim handle\n> non-canonicalized relative paths fine, emacs does not. This is due to a\n> long-standing bug in emacs, where it refuses to resolve symlinks in the\n> supplied path:\n>\n> #!/bin/sh\n> mkdir z z/a z/b\n> echo moodle >z/a/file\n> ln -s z/b\n> cd b\n> emacs ../a/file # fail: opens /tmp/a/file\n\nSomewhat puzzling. Perhaps you want to add \"cd /tmp\" at the very\nbeginning of the script to clarify, and s/opens/attempts to open/\nto avoid confusing a little panda brain.\n\n>\n> Even if emacs were to be patched to fix this bug now, we still need to\n> support users running older versions.\n>\n> Co-authored-by: Nguyễn Thái Ngọc Duy \n> Signed-off-by: Ramkumar Ramachandra \n\nThat's a bit strange---the patch text looks like the \"how about\nthis\" patch Duy posted earlier. Shouldn't it be From: Duy with\nS-o-b: by two of you instead?\n\n> diff --git a/editor.c b/editor.c\n> index 27bdecd..0abbd8d 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n> \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n> \n> \tif (strcmp(editor, \":\")) {\n> -\t\tconst char *args[] = { editor, path, NULL };\n> +\t\tconst char *args[] = { editor, real_path(path), NULL };\n\nWhile I am not fundamentally opposed to add a workaround for bugs in\na popular tool many people use, I am a bit uneasy about this change.\n\nFor editors that are not broken, this could be an annoying\nregression, isn't it? When the user asks \"What is the path of the\nfile I am editing?\" to the editor (i.e. an equivalent of \\C-x\\C-b),\nthe updated code will start spewing a long full-path from the root\ndirectory, while we used to give a relative path that is short,\nsweet and more in line with the context of user's work.\n\nCompared to not being able to edit, it may be a small price to pay\nfor those who do need to suffer the broken editor, but the patch\nmakes those who do not need this workaround to pay the price.\n"},{"id":"224232","messageId":"CALkWK0=dGmMkgvaNV27E3Pb3+SWSZ2pNMOr-wsEcUR_wgLjMFA@mail.gmail.com","threadId":"34554","inReplyTo":"7v8v0p2z45.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] editor: use canonicalized absolute path","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-29T16:07:15Z","receivedAt":"2013-07-29T16:07:15Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> That's a bit strange---the patch text looks like the \"how about\n> this\" patch Duy posted earlier. Shouldn't it be From: Duy with\n> S-o-b: by two of you instead?\n\nFeel free to amend as you see fit, as always.\n\n> For editors that are not broken, this could be an annoying\n> regression, isn't it? When the user asks \"What is the path of the\n> file I am editing?\" to the editor (i.e. an equivalent of \\C-x\\C-b),\n> the updated code will start spewing a long full-path from the root\n> directory, while we used to give a relative path that is short,\n> sweet and more in line with the context of user's work.\n\nDoes it matter for COMMIT_EDITMSG or other files in $GITDIR? If\nyou're concerned about it, we can change the logic to: real_path() if\npath-is-relative to avoid mangling the path in the non-submodules\ncase; in the submodules case, it will use an absolute-path, as before.\n"},{"id":"224238","messageId":"7vwqo91dgw.fsf@alter.siamese.dyndns.org","threadId":"34554","inReplyTo":"CALkWK0=dGmMkgvaNV27E3Pb3+SWSZ2pNMOr-wsEcUR_wgLjMFA@mail.gmail.com","subject":"Re: [PATCH] editor: use canonicalized absolute path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-29T17:29:51Z","receivedAt":"2013-07-29T17:29:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>> That's a bit strange---the patch text looks like the \"how about\n>> this\" patch Duy posted earlier. Shouldn't it be From: Duy with\n>> S-o-b: by two of you instead?\n>\n> Feel free to amend as you see fit, as always.\n\nI was asking what is \"correct\", without which I cannot \"feel free\"\nto do anything, and your answer is not helping.\n"},{"id":"224241","messageId":"CALkWK0=ZekB9uVtzM_z4wbfZ0JWawaob_PspM9UHbVJKyFGBfw@mail.gmail.com","threadId":"34554","inReplyTo":"7vwqo91dgw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] editor: use canonicalized absolute path","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-29T17:48:01Z","receivedAt":"2013-07-29T17:48:01Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Ramkumar Ramachandra writes:\n>> Junio C Hamano wrote:\n>>> That's a bit strange---the patch text looks like the \"how about\n>>> this\" patch Duy posted earlier. Shouldn't it be From: Duy with\n>>> S-o-b: by two of you instead?\n>>\n>> Feel free to amend as you see fit, as always.\n>\n> I was asking what is \"correct\", without which I cannot \"feel free\"\n> to do anything, and your answer is not helping.\n\nI don't have a strong opinion either way. Fwiw, I posted the original\nwith me as the author because I discovered it, dug through the emacs\nsources for hours to find the exact bug, and contacted emacs-devel\nabout it. But no, I do not think there is anything \"correct\" about it.\n"},{"id":"224257","messageId":"7vr4ehyx2j.fsf@alter.siamese.dyndns.org","threadId":"34554","inReplyTo":"CALkWK0=ZekB9uVtzM_z4wbfZ0JWawaob_PspM9UHbVJKyFGBfw@mail.gmail.com","subject":"Re: [PATCH] editor: use canonicalized absolute path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-29T19:40:04Z","receivedAt":"2013-07-29T19:40:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>> Ramkumar Ramachandra writes:\n>>> Junio C Hamano wrote:\n>>>> That's a bit strange---the patch text looks like the \"how about\n>>>> this\" patch Duy posted earlier. Shouldn't it be From: Duy with\n>>>> S-o-b: by two of you instead?\n>>>\n>>> Feel free to amend as you see fit, as always.\n>>\n>> I was asking what is \"correct\", without which I cannot \"feel free\"\n>> to do anything, and your answer is not helping.\n>\n> I don't have a strong opinion either way.\n\nThere is no opinion involved. Maybe the word \"correct\" had a wrong\nconnotation, but what I needed to find out was a true provenance of\nthe change, as that is what the S-o-b chain records. I did not\nknow, and I needed to find out, if the patch in question was what\nyou came up independently without looking at Duy's patch (which is\nvery understandable as you two were going back and forth digging the\nissue on the list), or you submitted his patch after tidying it up.\n\nIt appears from your description below that it was an independent\nwork, so that is what I'll queue.\n\nThanks.\n\n> Fwiw, I posted the original\n> with me as the author because I discovered it, dug through the emacs\n> sources for hours to find the exact bug, and contacted emacs-devel\n> about it.\n"},{"id":"224271","messageId":"CACsJy8DxSv7zP+EL0yZnkTztEm7taS0VQxcQ6XPQHMvVMLtVqg@mail.gmail.com","threadId":"34554","inReplyTo":"7v8v0p2z45.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] editor: use canonicalized absolute path","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-07-30T00:27:59Z","receivedAt":"2013-07-30T00:27:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jul 29, 2013 at 9:56 PM, Junio C Hamano wrote:\n>> Co-authored-by: Nguyễn Thái Ngọc Duy \n>> Signed-off-by: Ramkumar Ramachandra \n>\n> That's a bit strange---the patch text looks like the \"how about\n> this\" patch Duy posted earlier. Shouldn't it be From: Duy with\n> S-o-b: by two of you instead?\n\nThe idea is the same, but my patch is a bit different (use of realpath\ninstead of real_path, I didn't remember git has real_path). I'm fine\nwith Ram being the author.\n\n>> diff --git a/editor.c b/editor.c\n>> index 27bdecd..0abbd8d 100644\n>> --- a/editor.c\n>> +++ b/editor.c\n>> @@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n>> return error(\"Terminal is dumb, but EDITOR unset\");\n>>\n>> if (strcmp(editor, \":\")) {\n>> - const char *args[] = { editor, path, NULL };\n>> + const char *args[] = { editor, real_path(path), NULL };\n>\n> While I am not fundamentally opposed to add a workaround for bugs in\n> a popular tool many people use, I am a bit uneasy about this change.\n>\n> For editors that are not broken, this could be an annoying\n> regression, isn't it? When the user asks \"What is the path of the\n> file I am editing?\" to the editor (i.e. an equivalent of \\C-x\\C-b),\n> the updated code will start spewing a long full-path from the root\n> directory, while we used to give a relative path that is short,\n> sweet and more in line with the context of user's work.\n>\n> Compared to not being able to edit, it may be a small price to pay\n> for those who do need to suffer the broken editor, but the patch\n> makes those who do not need this workaround to pay the price.\n\nDoes looking at the edited file's path happen often? I have never done\nthat before. I ask because in order to avoid the price for those\nusers, the code could get a little bit more complicated (detecting if\nthe given relative path traverse backward outside a symlink..). To me\nit seems like a good trade off in favor of simpler code.\n-- \nDuy\n"},{"id":"224292","messageId":"20130730102138.GA30476@lanh","threadId":"34554","inReplyTo":"7v8v0p2z45.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] editor: use canonicalized absolute path","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-07-30T10:21:38Z","receivedAt":"2013-07-30T10:21:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jul 29, 2013 at 07:56:58AM -0700, Junio C Hamano wrote:\n> > diff --git a/editor.c b/editor.c\n> > index 27bdecd..0abbd8d 100644\n> > --- a/editor.c\n> > +++ b/editor.c\n> > @@ -37,7 +37,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n> > \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n> > \n> > \tif (strcmp(editor, \":\")) {\n> > -\t\tconst char *args[] = { editor, path, NULL };\n> > +\t\tconst char *args[] = { editor, real_path(path), NULL };\n> \n> While I am not fundamentally opposed to add a workaround for bugs in\n> a popular tool many people use, I am a bit uneasy about this change.\n> \n> For editors that are not broken, this could be an annoying\n> regression, isn't it? When the user asks \"What is the path of the\n> file I am editing?\" to the editor (i.e. an equivalent of \\C-x\\C-b),\n> the updated code will start spewing a long full-path from the root\n> directory, while we used to give a relative path that is short,\n> sweet and more in line with the context of user's work.\n> \n> Compared to not being able to edit, it may be a small price to pay\n> for those who do need to suffer the broken editor, but the patch\n> makes those who do not need this workaround to pay the price.\n> \n\nHow about something like this? For standard setups, even if you have\nsymlink in cwd, it won't kick in because the given path is usually\n.git/something (i.e. no \"..\" component)\n\n-- 8< --\ndiff --git a/editor.c b/editor.c\nindex 27bdecd..02bf42c 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -37,10 +37,17 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n \n \tif (strcmp(editor, \":\")) {\n+\t\tchar cwd[PATH_MAX];\n \t\tconst char *args[] = { editor, path, NULL };\n \t\tstruct child_process p;\n \t\tint ret, sig;\n \n+\t\t/* emacs workaround */\n+\t\tif (getcwd(cwd, PATH_MAX) &&\n+\t\t strcmp(real_path(cwd), cwd) &&\n+\t\t strstr(path, \"../\"))\n+\t\t\targs[1] = real_path(path);\n+\n \t\tmemset(&p, 0, sizeof(p));\n \t\tp.argv = args;\n \t\tp.env = env;\n-- 8<--\n"},{"id":"224293","messageId":"7vwqo8xiny.fsf@alter.siamese.dyndns.org","threadId":"34554","inReplyTo":"CACsJy8DxSv7zP+EL0yZnkTztEm7taS0VQxcQ6XPQHMvVMLtVqg@mail.gmail.com","subject":"Re: [PATCH] editor: use canonicalized absolute path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-30T13:48:49Z","receivedAt":"2013-07-30T13:48:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> The idea is the same, but my patch is a bit different (use of realpath\n> instead of real_path, I didn't remember git has real_path). I'm fine\n> with Ram being the author.\n\nThanks, both of you, for clarification.\n\n>> Compared to not being able to edit, it may be a small price to pay\n>> for those who do need to suffer the broken editor, but the patch\n>> makes those who do not need this workaround to pay the price.\n>\n> Does looking at the edited file's path happen often? I have never done\n> that before. I ask because in order to avoid the price for those\n> users, the code could get a little bit more complicated (detecting if\n> the given relative path traverse backward outside a symlink..). To me\n> it seems like a good trade off in favor of simpler code.\n\nYeah, I was being my usual cautious self, as somebody has to play\nthat role. I think the code as-is would be an OK trade off.\n"}]}