On Thu, Oct 09, 2014 at 11:36:00AM -0700, Junio C Hamano wrote:
> David Aguilar <davvid@xxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > Signed-off-by: David Aguilar <davvid@xxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html