{"thread":{"id":"37704","subject":"[PATCH v2] mergetool: use more conservative temporary filenames","startedAt":"2014-10-10T08:19:47Z","lastAt":"2014-10-13T19:30:57Z","messageCount":6,"participants":["David Aguilar","Jakub Narębski","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"250475","messageId":"1412929187-57936-1-git-send-email-davvid@gmail.com","threadId":"37704","inReplyTo":null,"subject":"[PATCH v2] mergetool: use more conservative temporary filenames","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-10-10T08:19:47Z","receivedAt":"2014-10-10T08:19:47Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Avoid filenames with multiple dots so that overly-picky tools do\nnot misinterpret their extension.\n\nPreviously, foo/bar.ext in the worktree would result in e.g.\n\n\t./foo/bar.ext.BASE.1234.ext\n\nThis can be improved by having only a single .ext and using\nunderscore instead of dot so that the extension cannot be\nmisinterpreted. The resulting path becomes:\n\n\t./foo/bar_BASE_1234.ext\n\nSuggested-by: Sergio Ferrero \nHelped-by: Junio C Hamano \nSigned-off-by: David Aguilar \n---\nChanges since v1\n\nThe commit message changed to say \"./foo\" instead of \"foo\".\n\nThe patch now uses Junio's suggestion to minimize variables,\nand preserves the original leading ./ just in case there are\ntools that rely on having ./ in front of relative paths.\n\n git-mergetool.sh | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 9a046b7..96a61ba 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -228,11 +228,17 @@ merge_file () {\n \t\treturn 1\n \tfi\n \n-\text=\"$$$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\"\n-\tBACKUP=\"./$MERGED.BACKUP.$ext\"\n-\tLOCAL=\"./$MERGED.LOCAL.$ext\"\n-\tREMOTE=\"./$MERGED.REMOTE.$ext\"\n-\tBASE=\"./$MERGED.BASE.$ext\"\n+\tif BASE=$(expr \"$MERGED\" : '\\(.*\\)\\.[^/]*$')\n+\tthen\n+\t\text=$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\n+\telse\n+\t\tBASE=$MERGED\n+\t\text=\n+\tfi\n+\tBACKUP=\"./${BASE}_BACKUP_$$$ext\"\n+\tLOCAL=\"./${BASE}_LOCAL_$$$ext\"\n+\tREMOTE=\"./${BASE}_REMOTE_$$$ext\"\n+\tBASE=\"./${BASE}_BASE_$$$ext\"\n \n \tbase_mode=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==1) print $1;}')\n \tlocal_mode=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==2) print $1;}')\n-- \n2.1.2.375.gd89e6a9\n"},{"id":"250481","messageId":"5437C0CC.7030102@gmail.com","threadId":"37704","inReplyTo":"1412929187-57936-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2] mergetool: use more conservative temporary filenames","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2014-10-10T11:19:40Z","receivedAt":"2014-10-10T11:19:40Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"David Aguilar wrote:\n> Avoid filenames with multiple dots so that overly-picky tools do\n> not misinterpret their extension.\n>\n> Previously, foo/bar.ext in the worktree would result in e.g.\n>\n> \t./foo/bar.ext.BASE.1234.ext\n>\n> This can be improved by having only a single .ext and using\n> underscore instead of dot so that the extension cannot be\n> misinterpreted. The resulting path becomes:\n>\n> \t./foo/bar_BASE_1234.ext\n>\n> Suggested-by: Sergio Ferrero \n> Helped-by: Junio C Hamano \n> Signed-off-by: David Aguilar \n> ---\n\n> +\tif BASE=$(expr \"$MERGED\" : '\\(.*\\)\\.[^/]*$')\n> +\tthen\n> +\t\text=$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\n> +\telse\n> +\t\tBASE=$MERGED\n> +\t\text=\n> +\tfi\n\nWhy use expr and not POSIX shell parameter substitution?\n\nBASE=${MERGED%.*}\next=.${MERGED##*.}\n\nOr something like that...\n\n-- \nJakub Narębski\n"},{"id":"250488","messageId":"20141010184834.GB78117@gmail.com","threadId":"37704","inReplyTo":"5437C0CC.7030102@gmail.com","subject":"Re: [PATCH v2] mergetool: use more conservative temporary filenames","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-10-10T18:48:36Z","receivedAt":"2014-10-10T18:48:36Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Oct 10, 2014 at 01:19:40PM +0200, Jakub Narębski wrote:\n> David Aguilar wrote:\n> >Avoid filenames with multiple dots so that overly-picky tools do\n> >not misinterpret their extension.\n> >\n> >Previously, foo/bar.ext in the worktree would result in e.g.\n> >\n> >\t./foo/bar.ext.BASE.1234.ext\n> >\n> >This can be improved by having only a single .ext and using\n> >underscore instead of dot so that the extension cannot be\n> >misinterpreted. The resulting path becomes:\n> >\n> >\t./foo/bar_BASE_1234.ext\n> >\n> >Suggested-by: Sergio Ferrero \n> >Helped-by: Junio C Hamano \n> >Signed-off-by: David Aguilar \n> >---\n> \n> >+\tif BASE=$(expr \"$MERGED\" : '\\(.*\\)\\.[^/]*$')\n> >+\tthen\n> >+\t\text=$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\n> >+\telse\n> >+\t\tBASE=$MERGED\n> >+\t\text=\n> >+\tfi\n> \n> Why use expr and not POSIX shell parameter substitution?\n> \n> BASE=${MERGED%.*}\n> ext=.${MERGED##*.}\n> \n> Or something like that...\n\nThanks for the sug.\n\nMy POSIX shell parameter expansion-fu is not super advanced, but\nif you can help me rework it I'd be happy to reroll.\n\nIt does seem simple and robust with expr, though. Extending the\nparameter expansion approach to work in all cases may end up\nwith more complexity than with the expr method, it seems.\n\nHere are the use cases:\n\n\t$ MERGED=foo.bar.baz && echo ${MERGED%.*} ${MERGED##*.}\n\tfoo.bar baz\n\nGood.\n\n\t$ MERGED=foo && echo ${MERGED%.*} ${MERGED##*.}\n\tfoo foo\n\nBad.\nThere's no extension and the substitution doesn't handle it.\n\n\t$ MERGED=foo.bar/baz && echo ${MERGED%.*} ${MERGED##*.}\n\tfoo bar/baz\n\nBad.\nThere's no extension but the substitution thinks the parent directory's\nextension-less name is the basename, and thinks that bar/baz is the extension.\n\nI am curious to know whether there's a nice and elegant way to do it\nwith shell expansions. Let me know what you think.\n\ncheers,\n-- \nDavid\n"},{"id":"250490","messageId":"54383975.9080803@kdbg.org","threadId":"37704","inReplyTo":"20141010184834.GB78117@gmail.com","subject":"Re: [PATCH v2] mergetool: use more conservative temporary filenames","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-10-10T19:54:29Z","receivedAt":"2014-10-10T19:54:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.10.2014 um 20:48 schrieb David Aguilar:\n> On Fri, Oct 10, 2014 at 01:19:40PM +0200, Jakub Narębski wrote:\n>> David Aguilar wrote:\n>>> Avoid filenames with multiple dots so that overly-picky tools do\n>>> not misinterpret their extension.\n>>>\n>>> Previously, foo/bar.ext in the worktree would result in e.g.\n>>>\n>>> \t./foo/bar.ext.BASE.1234.ext\n>>>\n>>> This can be improved by having only a single .ext and using\n>>> underscore instead of dot so that the extension cannot be\n>>> misinterpreted. The resulting path becomes:\n>>>\n>>> \t./foo/bar_BASE_1234.ext\n>>>\n>>> Suggested-by: Sergio Ferrero \n>>> Helped-by: Junio C Hamano \n>>> Signed-off-by: David Aguilar \n>>> ---\n>>\n>>> +\tif BASE=$(expr \"$MERGED\" : '\\(.*\\)\\.[^/]*$')\n>>> +\tthen\n>>> +\t\text=$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\n>>> +\telse\n>>> +\t\tBASE=$MERGED\n>>> +\t\text=\n>>> +\tfi\n>>\n>> Why use expr and not POSIX shell parameter substitution?\n>>\n>> BASE=${MERGED%.*}\n>> ext=.${MERGED##*.}\n>>\n>> Or something like that...\n> \n> Thanks for the sug.\n> \n> My POSIX shell parameter expansion-fu is not super advanced, but\n> if you can help me rework it I'd be happy to reroll.\n> \n> It does seem simple and robust with expr, though. Extending the\n> parameter expansion approach to work in all cases may end up\n> with more complexity than with the expr method, it seems.\n> \n> Here are the use cases:\n> \n> \t$ MERGED=foo.bar.baz && echo ${MERGED%.*} ${MERGED##*.}\n> \tfoo.bar baz\n> \n> Good.\n> \n> \t$ MERGED=foo && echo ${MERGED%.*} ${MERGED##*.}\n> \tfoo foo\n> \n> Bad.\n> There's no extension and the substitution doesn't handle it.\n> \n> \t$ MERGED=foo.bar/baz && echo ${MERGED%.*} ${MERGED##*.}\n> \tfoo bar/baz\n> \n> Bad.\n> There's no extension but the substitution thinks the parent directory's\n> extension-less name is the basename, and thinks that bar/baz is the extension.\n> \n> I am curious to know whether there's a nice and elegant way to do it\n> with shell expansions. Let me know what you think.\n\nIt's not exactly elegant to do it:\n\nFirst, you extract the last path part:\n\n\tfile=${MERGED##*/}\n\nThen the directory including the trailing slash:\n\n\tdir=${MERGED%\"$file\"}\n\nThen the basename without an extension:\n\n\tbase=${file%.*}\n\nFinally the extension including the dot:\n\n\text=${file#\"$base\"}\n\nBeware of empty $base and $dir (e.g., for files named .gitignore or so)\n\n-- Hannes\n"},{"id":"250491","messageId":"xmqq38av62x0.fsf@gitster.dls.corp.google.com","threadId":"37704","inReplyTo":"5437C0CC.7030102@gmail.com","subject":"Re: [PATCH v2] mergetool: use more conservative temporary filenames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-10T21:16:43Z","receivedAt":"2014-10-10T21:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski writes:\n\n> Why use expr and not POSIX shell parameter substitution?\n>\n> BASE=${MERGED%.*}\n> ext=.${MERGED##*.}\n>\n> Or something like that...\n\nBecause they are insufficient. See David's illustrations for how.\n\nParameter expansion (e.g. ${parameter%word}) is fine for the\nsimplest cases (e.g. you know there is .c suffix to the $string and\nwant to strip it out) but not sufficient for other cases (e.g. you\nmay not even know if there is any suffix). You can deconstruct your\nregexps manually and use conditional if/else/fi if you are unable to\nuse expr, but I do not see how it buys us anything.\n"},{"id":"250570","messageId":"xmqqfver3gy6.fsf@gitster.dls.corp.google.com","threadId":"37704","inReplyTo":"1412929187-57936-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2] mergetool: use more conservative temporary filenames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-13T19:30:57Z","receivedAt":"2014-10-13T19:30:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Avoid filenames with multiple dots so that overly-picky tools do\n> not misinterpret their extension.\n>\n> Previously, foo/bar.ext in the worktree would result in e.g.\n>\n> \t./foo/bar.ext.BASE.1234.ext\n>\n> This can be improved by having only a single .ext and using\n> underscore instead of dot so that the extension cannot be\n> misinterpreted. The resulting path becomes:\n>\n> \t./foo/bar_BASE_1234.ext\n>\n> Suggested-by: Sergio Ferrero \n> Helped-by: Junio C Hamano \n> Signed-off-by: David Aguilar \n> ---\n> Changes since v1\n>\n> The commit message changed to say \"./foo\" instead of \"foo\".\n>\n> The patch now uses Junio's suggestion to minimize variables,\n> and preserves the original leading ./ just in case there are\n> tools that rely on having ./ in front of relative paths.\n\n;-)\n\nPerhaps together with the \"allow temporary directory\" patch, we\nwould want to have a few tests for these changes?\n\n>\n> git-mergetool.sh | 16 +++++++++++-----\n> 1 file changed, 11 insertions(+), 5 deletions(-)\n>\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index 9a046b7..96a61ba 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -228,11 +228,17 @@ merge_file () {\n> \t\treturn 1\n> \tfi\n> \n> -\text=\"$$$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\"\n> -\tBACKUP=\"./$MERGED.BACKUP.$ext\"\n> -\tLOCAL=\"./$MERGED.LOCAL.$ext\"\n> -\tREMOTE=\"./$MERGED.REMOTE.$ext\"\n> -\tBASE=\"./$MERGED.BASE.$ext\"\n> +\tif BASE=$(expr \"$MERGED\" : '\\(.*\\)\\.[^/]*$')\n> +\tthen\n> +\t\text=$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\n> +\telse\n> +\t\tBASE=$MERGED\n> +\t\text=\n> +\tfi\n> +\tBACKUP=\"./${BASE}_BACKUP_$$$ext\"\n> +\tLOCAL=\"./${BASE}_LOCAL_$$$ext\"\n> +\tREMOTE=\"./${BASE}_REMOTE_$$$ext\"\n> +\tBASE=\"./${BASE}_BASE_$$$ext\"\n> \n> \tbase_mode=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==1) print $1;}')\n> \tlocal_mode=$(git ls-files -u -- \"$MERGED\" | awk '{if ($3==2) print $1;}')\n"}]}