[PATCH v2] mergetool: use more conservative temporary filenames

[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
misinterpreted. The resulting path becomes:

./foo/bar_BASE_1234.ext

Suggested-by: Sergio Ferrero <***@ensoftcorp.com>
Helped-by: Junio C Hamano <***@pobox.com>
Signed-off-by: David Aguilar <***@gmail.com>
---
Changes since v1

The commit message changed to say "./foo" instead of "foo".

The patch now uses Junio's suggestion to minimize variables,
and preserves the original leading ./ just in case there are
tools that rely on having ./ in front of relative paths.

git-mergetool.sh | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 9a046b7..96a61ba 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -228,11 +228,17 @@ merge_file () {
return 1
fi

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

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

[Jakub Narębski]

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
---
+ if BASE=3D$(expr "$MERGED" : '\(.*\)\.[^/]*$')
+ then
+ ext=3D$(expr "$MERGED" : '.*\(\.[^/]*\)$')
+ else
+ BASE=3D$MERGED
+ ext=3D
+ fi

Why use expr and not POSIX shell parameter substitution?

BASE=3D${MERGED%.*}
ext=3D.${MERGED##*.}

Or something like that...

--=20
Jakub Nar=C4=99bski

[David Aguilar]

Post by Jakub Narębski

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

=20

Post by David Aguilar
+ if BASE=3D$(expr "$MERGED" : '\(.*\)\.[^/]*$')
+ then
+ ext=3D$(expr "$MERGED" : '.*\(\.[^/]*\)$')
+ else
+ BASE=3D$MERGED
+ ext=3D
+ fi

=20
Why use expr and not POSIX shell parameter substitution?
=20
BASE=3D${MERGED%.*}
ext=3D.${MERGED##*.}
=20
Or something like that...

Thanks for the sug.

My POSIX shell parameter expansion-fu is not super advanced, but
if you can help me rework it I'd be happy to reroll.

It does seem simple and robust with expr, though. Extending the
parameter expansion approach to work in all cases may end up
with more complexity than with the expr method, it seems.

Here are the use cases:

$ MERGED=3Dfoo.bar.baz && echo ${MERGED%.*} ${MERGED##*.}
foo.bar baz

Good.

$ MERGED=3Dfoo && echo ${MERGED%.*} ${MERGED##*.}
foo foo

Bad.
There's no extension and the substitution doesn't handle it.

$ MERGED=3Dfoo.bar/baz && echo ${MERGED%.*} ${MERGED##*.}
foo bar/baz

Bad.
There's no extension but the substitution thinks the parent directory's
extension-less name is the basename, and thinks that bar/baz is the ext=
ension.

I am curious to know whether there's a nice and elegant way to do it
with shell expansions. Let me know what you think.

cheers,
--=20
David

[Johannes Sixt]

Post by David Aguilar

Post by Jakub Narębski

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
---
+ if BASE=3D$(expr "$MERGED" : '\(.*\)\.[^/]*$')
+ then
+ ext=3D$(expr "$MERGED" : '.*\(\.[^/]*\)$')
+ else
+ BASE=3D$MERGED
+ ext=3D
+ fi

Why use expr and not POSIX shell parameter substitution?
BASE=3D${MERGED%.*}
ext=3D.${MERGED##*.}
Or something like that...

=20
Thanks for the sug.
=20
My POSIX shell parameter expansion-fu is not super advanced, but
if you can help me rework it I'd be happy to reroll.
=20
It does seem simple and robust with expr, though. Extending the
parameter expansion approach to work in all cases may end up
with more complexity than with the expr method, it seems.
=20
=20
$ MERGED=3Dfoo.bar.baz && echo ${MERGED%.*} ${MERGED##*.}
foo.bar baz
=20
Good.
=20
$ MERGED=3Dfoo && echo ${MERGED%.*} ${MERGED##*.}
foo foo
=20
Bad.
There's no extension and the substitution doesn't handle it.
=20
$ MERGED=3Dfoo.bar/baz && echo ${MERGED%.*} ${MERGED##*.}
foo bar/baz
=20
Bad.
There's no extension but the substitution thinks the parent directory=

's

Post by David Aguilar
extension-less name is the basename, and thinks that bar/baz is the e=

xtension.

Post by David Aguilar
=20
I am curious to know whether there's a nice and elegant way to do it
with shell expansions. Let me know what you think.

It's not exactly elegant to do it:

=46irst, you extract the last path part:

file=3D${MERGED##*/}

Then the directory including the trailing slash:

dir=3D${MERGED%"$file"}

Then the basename without an extension:

base=3D${file%.*}

=46inally the extension including the dot:

ext=3D${file#"$base"}

Beware of empty $base and $dir (e.g., for files named .gitignore or so)

-- Hannes

[Junio C Hamano]

Post by Jakub Narębski
Why use expr and not POSIX shell parameter substitution?
BASE=3D${MERGED%.*}
ext=3D.${MERGED##*.}
Or something like that...

Because they are insufficient. See David's illustrations for how.

Parameter expansion (e.g. ${parameter%word}) is fine for the
simplest cases (e.g. you know there is .c suffix to the $string and
want to strip it out) but not sufficient for other cases (e.g. you
may not even know if there is any suffix). You can deconstruct your
regexps manually and use conditional if/else/fi if you are unable to
use expr, but I do not see how it buys us anything.

[Junio C Hamano]

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
---
Changes since v1
The commit message changed to say "./foo" instead of "foo".
The patch now uses Junio's suggestion to minimize variables,
and preserves the original leading ./ just in case there are
tools that rely on having ./ in front of relative paths.

;-)

Perhaps together with the "allow temporary directory" patch, we
would want to have a few tests for these changes?

Post by David Aguilar
git-mergetool.sh | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 9a046b7..96a61ba 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -228,11 +228,17 @@ merge_file () {
return 1
fi
- ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')"
- BACKUP="./$MERGED.BACKUP.$ext"
- LOCAL="./$MERGED.LOCAL.$ext"
- REMOTE="./$MERGED.REMOTE.$ext"
- BASE="./$MERGED.BASE.$ext"
+ if BASE=$(expr "$MERGED" : '\(.*\)\.[^/]*$')
+ then
+ ext=$(expr "$MERGED" : '.*\(\.[^/]*\)$')
+ else
+ BASE=$MERGED
+ ext=
+ fi
+ BACKUP="./${BASE}_BACKUP_$$$ext"
+ LOCAL="./${BASE}_LOCAL_$$$ext"
+ REMOTE="./${BASE}_REMOTE_$$$ext"
+ BASE="./${BASE}_BASE_$$$ext"
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;}')