Re: [PATCH] mergetool: use more conservative temporary filenames

On Thu, Oct 09, 2014 at 11:36:00AM -0700, Junio C Hamano wrote:

David Aguilar [off-list ref] writes:

quoted

Avoid filenames with multiple dots so that overly-picky tools do
not misinterpret their extension.

Previously, foo/bar.ext in the worktree would result in e.g.

	foo/bar.ext.BASE.1234.ext

This can be improved by having only a single .ext and using
underscore instead of dot so that the extension cannot be
misinterpreted.  The resulting path becomes:

	foo/bar_BASE_1234.ext

Suggested-by: Sergio Ferrero <redacted>
Signed-off-by: David Aguilar <redacted>
---
 git-mergetool.sh | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 9a046b7..1f33051 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh

@@ -228,11 +228,15 @@ merge_file () {
 		return 1
 	fi
 
-	ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')"
-	BACKUP="./$MERGED.BACKUP.$ext"
-	LOCAL="./$MERGED.LOCAL.$ext"
-	REMOTE="./$MERGED.REMOTE.$ext"
-	BASE="./$MERGED.BASE.$ext"
+	ext=$(expr "$MERGED" : '.*\(\.[^/]*\)$')
+	base=$(basename "$MERGED" "$ext")
+	dir=$(dirname "$MERGED")
+	suffix="$$""$ext"
+
+	BACKUP="$dir/$base"_BACKUP_"$suffix"
+	BASE="$dir/$base"_BASE_"$suffix"
+	LOCAL="$dir/$base"_LOCAL_"$suffix"
+	REMOTE="$dir/$base"_REMOTE_"$suffix"

We used to feed "./foo/bar.ext.BASE.1234.ext"; with this patch we
feed "foo/bar_BASE_1234.ext".  

It does make this particular example look prettier, but is the
droppage of "./" intentional and is free of unintended ill side
effects?

We avoid "local" and bash-isms, so I'd prefer to see us not to
introduce new temporary variables unnecessarily.  I think we can at
least do without basename/dirname in this case, perhaps like so:

	if BASE=$(expr "$MERGED" : '\(.*\)\.[^/]*$')
        then
        	ext=$(expr "$MERGED" : '.*\(\.[^/]*\)$')
	else
        	ext= BASE=$MERGED
	fi
        BACKUP="${BASE}_BACKUP_$$$ext"
        LOCAL="${BASE}_LOCAL_$$$ext"
        REMOTE="${BASE}_REMOTE_$$$ext"
        BASE="${BASE}_BASE_$$$ext"

Clever ;-)

But I do not have very strong opinion either way.  I just didn't
want to have to think about the leading "./" ;-)

When I first wrote this I thought, "$(dirname foo) == '.', so it should
be okay", but it slipped my mind that $(dirname foo/bar) != "./foo" --
I like this new version better.

The leading ./ shoudln't make a difference but I also don't want
to have to think about it either.  I'll have a v2 patch shortly.
-- 
David