{"thread":{"id":"37688","subject":"[PATCH] mergetool: use more conservative temporary filenames","startedAt":"2014-10-08T08:56:02Z","lastAt":"2014-10-10T18:23:13Z","messageCount":5,"participants":["David Aguilar","Junio C Hamano","Charles Bailey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"250344","messageId":"1412758562-25402-1-git-send-email-davvid@gmail.com","threadId":"37688","inReplyTo":null,"subject":"[PATCH] mergetool: use more conservative temporary filenames","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-10-08T08:56:02Z","receivedAt":"2014-10-08T08:56:02Z","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\tfoo/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\tfoo/bar_BASE_1234.ext\n\nSuggested-by: Sergio Ferrero <sferrero@ensoftcorp.com>\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool.sh | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 9a046b7..1f33051 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -228,11 +228,15 @@ 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+\text=$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\n+\tbase=$(basename \"$MERGED\" \"$ext\")\n+\tdir=$(dirname \"$MERGED\")\n+\tsuffix=\"$$\"\"$ext\"\n+\n+\tBACKUP=\"$dir/$base\"_BACKUP_\"$suffix\"\n+\tBASE=\"$dir/$base\"_BASE_\"$suffix\"\n+\tLOCAL=\"$dir/$base\"_LOCAL_\"$suffix\"\n+\tREMOTE=\"$dir/$base\"_REMOTE_\"$suffix\"\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.337.gd0cf3c1\n"},{"id":"250434","messageId":"xmqq8ukp6qgf.fsf@gitster.dls.corp.google.com","threadId":"37688","inReplyTo":"1412758562-25402-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] mergetool: use more conservative temporary filenames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-09T18:36:00Z","receivedAt":"2014-10-09T18:36:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> 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> \tfoo/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> \tfoo/bar_BASE_1234.ext\n>\n> Suggested-by: Sergio Ferrero <sferrero@ensoftcorp.com>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  git-mergetool.sh | 14 +++++++++-----\n>  1 file changed, 9 insertions(+), 5 deletions(-)\n>\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index 9a046b7..1f33051 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -228,11 +228,15 @@ 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> +\text=$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\n> +\tbase=$(basename \"$MERGED\" \"$ext\")\n> +\tdir=$(dirname \"$MERGED\")\n> +\tsuffix=\"$$\"\"$ext\"\n> +\n> +\tBACKUP=\"$dir/$base\"_BACKUP_\"$suffix\"\n> +\tBASE=\"$dir/$base\"_BASE_\"$suffix\"\n> +\tLOCAL=\"$dir/$base\"_LOCAL_\"$suffix\"\n> +\tREMOTE=\"$dir/$base\"_REMOTE_\"$suffix\"\n\nWe used to feed \"./foo/bar.ext.BASE.1234.ext\"; with this patch we\nfeed \"foo/bar_BASE_1234.ext\".  \n\nIt does make this particular example look prettier, but is the\ndroppage of \"./\" intentional and is free of unintended ill side\neffects?\n\nWe avoid \"local\" and bash-isms, so I'd prefer to see us not to\nintroduce new temporary variables unnecessarily.  I think we can at\nleast do without basename/dirname in this case, perhaps like so:\n\n\tif BASE=$(expr \"$MERGED\" : '\\(.*\\)\\.[^/]*$')\n        then\n        \text=$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\n\telse\n        \text= BASE=$MERGED\n\tfi\n        BACKUP=\"${BASE}_BACKUP_$$$ext\"\n        LOCAL=\"${BASE}_LOCAL_$$$ext\"\n        REMOTE=\"${BASE}_REMOTE_$$$ext\"\n        BASE=\"${BASE}_BASE_$$$ext\"\n        \nBut I do not have very strong opinion either way.  I just didn't\nwant to have to think about the leading \"./\" ;-)\n"},{"id":"250473","messageId":"20141010081034.GA44749@gmail.com","threadId":"37688","inReplyTo":"xmqq8ukp6qgf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] mergetool: use more conservative temporary filenames","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-10-10T08:10:36Z","receivedAt":"2014-10-10T08:10:36Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Oct 09, 2014 at 11:36:00AM -0700, Junio C Hamano wrote:\n> David Aguilar <davvid@gmail.com> 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> > \tfoo/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> > \tfoo/bar_BASE_1234.ext\n> >\n> > Suggested-by: Sergio Ferrero <sferrero@ensoftcorp.com>\n> > Signed-off-by: David Aguilar <davvid@gmail.com>\n> > ---\n> >  git-mergetool.sh | 14 +++++++++-----\n> >  1 file changed, 9 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/git-mergetool.sh b/git-mergetool.sh\n> > index 9a046b7..1f33051 100755\n> > --- a/git-mergetool.sh\n> > +++ b/git-mergetool.sh\n> > @@ -228,11 +228,15 @@ 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> > +\text=$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\n> > +\tbase=$(basename \"$MERGED\" \"$ext\")\n> > +\tdir=$(dirname \"$MERGED\")\n> > +\tsuffix=\"$$\"\"$ext\"\n> > +\n> > +\tBACKUP=\"$dir/$base\"_BACKUP_\"$suffix\"\n> > +\tBASE=\"$dir/$base\"_BASE_\"$suffix\"\n> > +\tLOCAL=\"$dir/$base\"_LOCAL_\"$suffix\"\n> > +\tREMOTE=\"$dir/$base\"_REMOTE_\"$suffix\"\n> \n> We used to feed \"./foo/bar.ext.BASE.1234.ext\"; with this patch we\n> feed \"foo/bar_BASE_1234.ext\".  \n> \n> It does make this particular example look prettier, but is the\n> droppage of \"./\" intentional and is free of unintended ill side\n> effects?\n> \n> We avoid \"local\" and bash-isms, so I'd prefer to see us not to\n> introduce new temporary variables unnecessarily.  I think we can at\n> least do without basename/dirname in this case, perhaps like so:\n> \n> \tif BASE=$(expr \"$MERGED\" : '\\(.*\\)\\.[^/]*$')\n>         then\n>         \text=$(expr \"$MERGED\" : '.*\\(\\.[^/]*\\)$')\n> \telse\n>         \text= BASE=$MERGED\n> \tfi\n>         BACKUP=\"${BASE}_BACKUP_$$$ext\"\n>         LOCAL=\"${BASE}_LOCAL_$$$ext\"\n>         REMOTE=\"${BASE}_REMOTE_$$$ext\"\n>         BASE=\"${BASE}_BASE_$$$ext\"\n\nClever ;-)\n\n> But I do not have very strong opinion either way.  I just didn't\n> want to have to think about the leading \"./\" ;-)\n\nWhen I first wrote this I thought, \"$(dirname foo) == '.', so it should\nbe okay\", but it slipped my mind that $(dirname foo/bar) != \"./foo\" --\nI like this new version better.\n\nThe leading ./ shoudln't make a difference but I also don't want\nto have to think about it either.  I'll have a v2 patch shortly.\n-- \nDavid\n"},{"id":"250478","messageId":"2DA99335-66B9-438E-A445-EB54B81EBE7A@hashpling.org","threadId":"37688","inReplyTo":"20141010081034.GA44749@gmail.com","subject":"Re: [PATCH] mergetool: use more conservative temporary filenames","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2014-10-10T09:07:20Z","receivedAt":"2014-10-10T09:07:20Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"While you have the lid of this section of code, should we consider (optionally?) using a tmpdir to alleviate the eclipse issue where it wants temporary merge files to be the canonical locations for definitions of things that it finds when scanning source files in the project tree?\n\n[Apologies for this email client's long lines.]"},{"id":"250487","messageId":"20141010182312.GA78117@gmail.com","threadId":"37688","inReplyTo":"2DA99335-66B9-438E-A445-EB54B81EBE7A@hashpling.org","subject":"Re: [PATCH] mergetool: use more conservative temporary filenames","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-10-10T18:23:13Z","receivedAt":"2014-10-10T18:23:13Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Oct 10, 2014 at 10:07:20AM +0100, Charles Bailey wrote:\n> While you have the lid of this section of code, should we\n> consider (optionally?) using a tmpdir to alleviate the eclipse\n> issue where it wants temporary merge files to be the canonical\n> locations for definitions of things that it finds when\n> scanning source files in the project tree?\n\nHiding it behind mergetool.usetmpdir seems like a good idea.\n\nThat could be a good follow-up patch.\nI don't use eclipse so I wasn't aware that this is an issue.\n\nThanks,\n-- \nDavid\n"}]}