[PATCH] mergetool: use more conservative temporary filenames

Discussion:

David Aguilar

2014-10-08 08:56:02 UTC

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 <***@ensoftcorp.com>
Signed-off-by: David Aguilar <***@gmail.com>
---
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"

base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}')
local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')

--
2.1.2.337.gd0cf3c1

Junio C Hamano

2014-10-09 18:36:00 UTC

Permalink

Post by David Aguilar
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
foo/bar_BASE_1234.ext
---
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 "./" ;-)

David Aguilar

2014-10-10 08:10:36 UTC

Permalink

Post by Junio C Hamano

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
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 ;-)

Post by Junio C Hamano
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

Charles Bailey

2014-10-10 09:07:20 UTC

Permalink

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?

[Apologies for this email client's long lines.]--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Aguilar

2014-10-10 18:23:13 UTC

Permalink

Post by Charles Bailey
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?

Hiding it behind mergetool.usetmpdir seems like a good idea.

That could be a good follow-up patch.
I don't use eclipse so I wasn't aware that this is an issue.

Thanks,

--
David