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

[David Aguilar <davvid@gmail.com>]

On Thu, Oct 09, 2014 at 11:36:00AM -0700, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
> 
> > 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 <sferrero@ensoftcorp.com>
> > Signed-off-by: David Aguilar <davvid@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"
> 
> 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