Re: [PATCH] mergetool: use more conservative temporary filenames
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:41
David Aguilar [off-list ref] writes:
quoted hunk
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"
But I do not have very strong opinion either way. I just didn't
want to have to think about the leading "./" ;-)