{"thread":{"id":"11492","subject":"[PATCH] Allow git-mergetool to handle paths with a leading space","startedAt":"2008-01-06T09:25:18Z","lastAt":"2008-01-08T01:19:33Z","messageCount":7,"participants":["Rogan Dawes","Junio C Hamano","Theodore Tso"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"64581","messageId":"47809E7E.2090708@dawes.za.net","threadId":"11492","inReplyTo":null,"subject":"[PATCH] Allow git-mergetool to handle paths with a leading space","fromName":"Rogan Dawes","fromEmail":"rogan@dawes.za.net","sentAt":"2008-01-06T09:25:18Z","receivedAt":"2008-01-06T09:25:18Z","isPatch":true,"sender":{"key":"rogan@dawes.za.net","avatar":null},"body":"Signed-off-by: Rogan Dawes <rogan@dawes.za.net>\n\n---\nI am working on a project which has the root directory constructed with \na leading space. i.e. ./ dir/. \"read\" skips the leading space char, and \nends up with an incorrect filename, which can then not be found. Setting \nIFS=\\n solves this problem.\n\nThanks to Mikachu and Tv on #git for assistance and suggestions.\n\nUnfortunately, Thunderbird is not very easy to convince not to mangle \npatches, so I am attaching the patch as well. Sorry.\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 2f31fa2..8521ca3 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -394,7 +394,7 @@ if test $# -eq 0 ; then\n                 exit 0\n         fi\n         echo Merging the files: $files\n-       git ls-files -u | sed -e 's/^[^ ]*      //' | sort -u | while read i\n+       git ls-files -u | sed -e 's/^[^ ]*      //' | sort -u | while \nIFS=\\n read i\n         do\n                 printf \"\\n\"\n                 merge_file \"$i\" < /dev/tty > /dev/tty\n\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 2f31fa2..8521ca3 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -394,7 +394,7 @@ if test $# -eq 0 ; then\n \t\texit 0\n \tfi\n \techo Merging the files: $files\n-\tgit ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u | while read i\n+\tgit ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u | while IFS=\\n read i\n \tdo\n \t\tprintf \"\\n\"\n \t\tmerge_file \"$i\" < /dev/tty > /dev/tty\n"},{"id":"64582","messageId":"7vodbz5ka9.fsf@gitster.siamese.dyndns.org","threadId":"11492","inReplyTo":"47809E7E.2090708@dawes.za.net","subject":"Re: [PATCH] Allow git-mergetool to handle paths with a leading space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-06T10:18:54Z","receivedAt":"2008-01-06T10:18:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rogan Dawes <rogan@dawes.za.net> writes:\n\n> Signed-off-by: Rogan Dawes <rogan@dawes.za.net>\n>\n> ---\n> I am working on a project which has the root directory constructed\n> with a leading space. i.e. ./ dir/. \"read\" skips the leading space\n> char, and ends up with an incorrect filename, which can then not be\n> found. Setting IFS=\\n solves this problem.\n\nDoes the project have a file that has letter 'n' (en) in its name?\nHave you tested your patch while having a conflict in that file?\n"},{"id":"64585","messageId":"4780B2BD.6020109@dawes.za.net","threadId":"11492","inReplyTo":"7vodbz5ka9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Allow git-mergetool to handle paths with a leading space","fromName":"Rogan Dawes","fromEmail":"rogan@dawes.za.net","sentAt":"2008-01-06T10:51:41Z","receivedAt":"2008-01-06T10:51:41Z","isPatch":true,"sender":{"key":"rogan@dawes.za.net","avatar":null},"body":"Junio C Hamano wrote:\n> Rogan Dawes <rogan@dawes.za.net> writes:\n> \n>> Signed-off-by: Rogan Dawes <rogan@dawes.za.net>\n>>\n>> ---\n>> I am working on a project which has the root directory constructed\n>> with a leading space. i.e. ./ dir/. \"read\" skips the leading space\n>> char, and ends up with an incorrect filename, which can then not be\n>> found. Setting IFS=\\n solves this problem.\n> \n> Does the project have a file that has letter 'n' (en) in its name?\n> Have you tested your patch while having a conflict in that file?\n> \n\nYes, it works correctly.\n\n$ git mergetool\nmerge tool candidates: kdiff3 tkdiff xxdiff meld gvimdiff vimdiff \nopendiff emerge vimdiff\nMerging the files: \nwebgoat/main/project/JavaSource/org/owasp/webgoat/lessons/BackDoors.java \nwebgoat/main/project/JavaSource/org/owasp/webgoat/lessons/BlindSqlInjection.java \nwebgoat/main/project/JavaSource/org/owasp/webgoat/lessons/Challenge2Screen.java\n\nNormal merge conflict for ' \nwebgoat/main/project/JavaSource/org/owasp/webgoat/lessons/BackDoors.java':\n   {local}: modified\n   {remote}: modified\nHit return to start merge resolution tool (kdiff3):\nmerge of \nwebgoat/main/project/JavaSource/org/owasp/webgoat/lessons/BackDoors.java \nfailed\n\n$\n\nMy copy and paste does not show the spaces properly, since everything \ngets wrapped. but the \"Normal merge conflict for ' webgoat/\" line would \nhave taken the ' to the next line if the space was not there. :-)\n\nNote that the lines after \"Merging the files\" don't include the spaces, \nbut that is just cosmetic, IMO.\n\nRogan\n"},{"id":"64590","messageId":"7v3atb5g5o.fsf@gitster.siamese.dyndns.org","threadId":"11492","inReplyTo":"4780B2BD.6020109@dawes.za.net","subject":"Re: [PATCH] Allow git-mergetool to handle paths with a leading space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-06T11:48:03Z","receivedAt":"2008-01-06T11:48:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rogan Dawes <rogan@dawes.za.net> writes:\n\n>>> I am working on a project which has the root directory constructed\n>>> with a leading space. i.e. ./ dir/. \"read\" skips the leading space\n>>> char, and ends up with an incorrect filename, which can then not be\n>>> found. Setting IFS=\\n solves this problem.\n>>\n>> Does the project have a file that has letter 'n' (en) in its name?\n>> Have you tested your patch while having a conflict in that file?\n>\n> Yes, it works correctly.\n\nI am curious and puzzled...\n\n        $ echo 'ann1' | (IFS=\\n read i; echo \"<$i>\")\n        <ann1>\n        $ echo 'ann1' | (IFS=\\n read i j; echo \"<$i>\")\n\t<a>\n        $ echo 'n1' | (IFS=\\n read i j; echo \"<$i>\")\n\t<>\n\nOk, \"\\n\" is a funny way to say IFS does not matter as long as it\nis set to a non whitespace letter.\n\nIt is VERY misleading as it looks as if the issue is fixed by\nsetting IFS to a single LF alone (excluding SP and HT from the\nusual set), but that is not the patch is doing.  It is setting\nit to a single 'n'.\n\nI think you still will lose backslash by using read, but I guess\nyou would not care about that too much.\n\nIf you really cared, you would do something like this, but you\nwould also need similar surgery in merge_file function itself\nthat parses text form of ls-files output that tries to verify\nand extract paths, which I did not bother to touch in this\ndemonstration patch.\n\n git-mergetool.sh |   33 ++++++++++++++++++++++-----------\n 1 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 2f31fa2..b7c5098 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -388,17 +388,28 @@ fi\n \n \n if test $# -eq 0 ; then\n-\tfiles=`git ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u`\n-\tif test -z \"$files\" ; then\n-\t\techo \"No files need merging\"\n-\t\texit 0\n-\tfi\n-\techo Merging the files: $files\n-\tgit ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u | while read i\n-\tdo\n-\t\tprintf \"\\n\"\n-\t\tmerge_file \"$i\" < /dev/tty > /dev/tty\n-\tdone\n+\tdoit=$(perl -e '\n+\t\t$/ = \"\\0\";\n+\t\tmy (%seen, @file);\n+\t\tmy $ls_files;\n+\t\topen($ls_files, \"-|\", qw(git ls-files -u -z));\n+\t\twhile (<$ls_files>) {\n+\t\t\tchomp;\n+\t\t\ts/^[^\t]*\t//;\n+\t\t\t$seen{$_}++;\n+\t\t}\n+\t\t@file = sort keys %seen;\n+\t\tif (!@file) {\n+\t\t\tprint \"echo No files need merging\\n\";\n+\t\t\tprint \"exit 0\\n\";\n+\t\t}\n+\t\tfor (@file) {\n+\t\t\ts|'\\''|'\\''\\\\'\\'''\\''|g;\n+\t\t\tprint \"echo\\n\";\n+\t\t\tprint \"merge_file '\\''$_'\\'' </dev/tty >/dev/tty\\n\";\n+\t\t}\n+\t')\n+\teval \"$doit\"\n else\n \twhile test $# -gt 0; do\n \t\tprintf \"\\n\"\n"},{"id":"64628","messageId":"4781D6C2.9060305@dawes.za.net","threadId":"11492","inReplyTo":"7v3atb5g5o.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Allow git-mergetool to handle paths with a leading space","fromName":"Rogan Dawes","fromEmail":"rogan@dawes.za.net","sentAt":"2008-01-07T07:37:38Z","receivedAt":"2008-01-07T07:37:38Z","isPatch":true,"sender":{"key":"rogan@dawes.za.net","avatar":null},"body":"Signed-off-by: Rogan Dawes <rogan@dawes.za.net>\n---\nJunio C Hamano wrote:\n> Rogan Dawes <rogan@dawes.za.net> writes:\n> \n>>>> I am working on a project which has the root directory constructed\n>>>> with a leading space. i.e. ./ dir/. \"read\" skips the leading space\n>>>> char, and ends up with an incorrect filename, which can then not be\n>>>> found. Setting IFS=\\n solves this problem.\n>>> Does the project have a file that has letter 'n' (en) in its name?\n>>> Have you tested your patch while having a conflict in that file?\n>> Yes, it works correctly.\n> \n> I am curious and puzzled...\n> \n>         $ echo 'ann1' | (IFS=\\n read i; echo \"<$i>\")\n>         <ann1>\n>         $ echo 'ann1' | (IFS=\\n read i j; echo \"<$i>\")\n> \t<a>\n>         $ echo 'n1' | (IFS=\\n read i j; echo \"<$i>\")\n> \t<>\n> \n> Ok, \"\\n\" is a funny way to say IFS does not matter as long as it\n> is set to a non whitespace letter.\n> \n> It is VERY misleading as it looks as if the issue is fixed by\n> setting IFS to a single LF alone (excluding SP and HT from the\n> usual set), but that is not the patch is doing.  It is setting\n> it to a single 'n'.\n\nYes, you are right. Maybe setting IFS to the empty string is better?\n\n$ printf \" ann 1\\n ann 2\\n\" | while IFS=\"\" read i j k ; do echo \"<$i> \n<$j> <$k>\"; done\n< ann 1> <> <>\n< ann 2> <> <>\n$\n\nAdmittedly, there are still problems with my version, as you say, \nbackslashes and newlines will not be handled correctly. My Perl-fu is \nweak, however, and my revised solution (IFS=\"\") works for me :-)\n\nRogan\n\nP.S. Quoting \"$files\" stops the spaces from being eaten in the preceding \necho line.\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 2f31fa2..facfbc8 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -393,8 +393,8 @@ if test $# -eq 0 ; then\n                 echo \"No files need merging\"\n                 exit 0\n         fi\n-       echo Merging the files: $files\n-       git ls-files -u | sed -e 's/^[^ ]*      //' | sort -u | while read i\n+       echo Merging the files: \"$files\"\n+       git ls-files -u | sed -e 's/^[^ ]*      //' | sort -u | while \nIFS=\"\" read i\n         do\n                 printf \"\\n\"\n                 merge_file \"$i\" < /dev/tty > /dev/tty\n\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 2f31fa2..facfbc8 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -393,8 +393,8 @@ if test $# -eq 0 ; then\n \t\techo \"No files need merging\"\n \t\texit 0\n \tfi\n-\techo Merging the files: $files\n-\tgit ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u | while read i\n+\techo Merging the files: \"$files\"\n+\tgit ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u | while IFS=\"\" read i\n \tdo\n \t\tprintf \"\\n\"\n \t\tmerge_file \"$i\" < /dev/tty > /dev/tty\n"},{"id":"64633","messageId":"7v8x320zom.fsf@gitster.siamese.dyndns.org","threadId":"11492","inReplyTo":"4781D6C2.9060305@dawes.za.net","subject":"Re: [PATCH] Allow git-mergetool to handle paths with a leading space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-07T09:09:45Z","receivedAt":"2008-01-07T09:09:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rogan Dawes <rogan@dawes.za.net> writes:\n\n> Yes, you are right. Maybe setting IFS to the empty string is better?\n\nYes.  POSIX XCU \"2.6.5 Field Splitting\" is quite clear about\nthis.  Empty IFS tells read not to split the fields, and such an\nIFS does not have IFS white space in it so the SP at the\nbeginning of the input will not be stripped.\n\nMy limited test indicates that bash, dash and ksh do behave as\nspecified, so I think it is a reasonably safe thing to do.\nAlthough I usually would say \"I do not care about Solaris\ndefault /bin/sh\", I know even that shell at least gets this\nright.\n\nI think the patch is Ok as long as the user does not care any\ncharacter outside the portable filename character set (POSIX XBD\n3.276) other than SP.  Because the rest of the mergetool is\nwritten in such a way that it does not handle many characters\noutside the portable filename character set well anyway, I think\nthe filename limitation that still remains after your patch may\nbe acceptable.\n\nI did not however apply your patch to my tree tonight, as Ted as\nthe original and the primary author of the script may have\nbetter ideas, and/or future directions for the patch.  He can\nsimply just Ack your patch if he chooses to.\n\nI'd however write that overly long pipeline indented a bit more\nsanely, like this, though:\n\n\tgit ls-files -u |\n        sed -e 's/^[^\t]*\t//' |\n        sort -u |\n        while IFS=\"\" read i\n\tdo\n        \tprintf \"\\n\"\n                merge_file \"$i\" </dev/tty >/dev/tty\n                ...\n\nIn the longer term, I'd probably suggest redoing the entire\ncommand in a NUL safe scripting language (or even C, especially\nif pressed by mingw or msys folks) to eliminate the issue\naltogether, though.  But that would definitely be a post 1.5.4\nitem.\n\n\n[Reference]\n\nhttp://www.opengroup.org/onlinepubs/000095399/toc.htm\n"},{"id":"64692","messageId":"20080108011933.GE27800@mit.edu","threadId":"11492","inReplyTo":"7v8x320zom.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Allow git-mergetool to handle paths with a leading space","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2008-01-08T01:19:33Z","receivedAt":"2008-01-08T01:19:33Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Mon, Jan 07, 2008 at 01:09:45AM -0800, Junio C Hamano wrote:\n> Rogan Dawes <rogan@dawes.za.net> writes:\n> \n> I did not however apply your patch to my tree tonight, as Ted as\n> the original and the primary author of the script may have\n> better ideas, and/or future directions for the patch.  He can\n> simply just Ack your patch if he chooses to.\n\nAcked-by: \"Theodore Ts'o\" <tytso@mit.edu>\n\n> In the longer term, I'd probably suggest redoing the entire\n> command in a NUL safe scripting language (or even C, especially\n> if pressed by mingw or msys folks) to eliminate the issue\n> altogether, though.  But that would definitely be a post 1.5.4\n> item.\n\nYeah, we eventually need to redo \"git mergetool\" in C.  One of the\nreasons for that is at the moment it can't take advantage of the\nrename detection machinery, so in the case where there is a merge\nconflict involving a file that has been renamed in one of the two\nbranches, \"git mergetool\" doesn't do the right thing.\n\nEven if it would be possible to expose the rename machinery in a form\nthat would be convenient for a shell script, it's past time to rewrite\nit in C, I think....\n\n\t\t\t\t\t\t- Ted\n"}]}