threads / patch / 37704

v2mergetool: use more conservative temporary filenames

Subject: [PATCH v2] mergetool: use more conservative temporary filenames

## tl;dr

6 messages between Oct 10, 2014 and Oct 13, 2014. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

David Aguilar· Oct 10, 2014, 08:19 UTC · lore

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>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: David Aguilar <davvid@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(-)
Show changes to git-mergetool.sh +11 −5
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;}')

-- 
2.1.2.375.gd89e6a9
Jakub Narębski· Oct 10, 2014, 11:19 UTC · re: David Aguilar · lore

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

David Aguilar wrote:
Show 17 quoted lines
> 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>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
Show 7 quoted lines
> +	if BASE=$(expr "$MERGED" : '\(.*\)\.[^/]*$')
> +	then
> +		ext=$(expr "$MERGED" : '.*\(\.[^/]*\)$')
> +	else
> +		BASE=$MERGED
> +		ext=
> +	fi
Why use expr and not POSIX shell parameter substitution?

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

Or something like that...
-- 
Jakub Narębski
David Aguilar· Oct 10, 2014, 18:48 UTC · re: Jakub Narębski · lore

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

On Fri, Oct 10, 2014 at 01:19:40PM +0200, Jakub Narębski wrote:
Show 33 quoted lines
> David Aguilar wrote:
> >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>
> >Helped-by: Junio C Hamano <gitster@pobox.com>
> >Signed-off-by: David Aguilar <davvid@gmail.com>
> >---
> 
> >+	if BASE=$(expr "$MERGED" : '\(.*\)\.[^/]*$')
> >+	then
> >+		ext=$(expr "$MERGED" : '.*\(\.[^/]*\)$')
> >+	else
> >+		BASE=$MERGED
> >+		ext=
> >+	fi
> 
> Why use expr and not POSIX shell parameter substitution?
> 
> BASE=${MERGED%.*}
> ext=.${MERGED##*.}
> 
> 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=foo.bar.baz && echo ${MERGED%.*} ${MERGED##*.}
	foo.bar baz
Good.
	$ MERGED=foo && echo ${MERGED%.*} ${MERGED##*.}
	foo foo

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

	$ MERGED=foo.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 extension.

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,
-- 
David
Johannes Sixt· Oct 10, 2014, 19:54 UTC · re: David Aguilar · lore

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

Am 10.10.2014 um 20:48 schrieb David Aguilar:
Show 66 quoted lines
> On Fri, Oct 10, 2014 at 01:19:40PM +0200, Jakub Narębski wrote:
>> David Aguilar wrote:
>>> 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>
>>> Helped-by: Junio C Hamano <gitster@pobox.com>
>>> Signed-off-by: David Aguilar <davvid@gmail.com>
>>> ---
>>
>>> +	if BASE=$(expr "$MERGED" : '\(.*\)\.[^/]*$')
>>> +	then
>>> +		ext=$(expr "$MERGED" : '.*\(\.[^/]*\)$')
>>> +	else
>>> +		BASE=$MERGED
>>> +		ext=
>>> +	fi
>>
>> Why use expr and not POSIX shell parameter substitution?
>>
>> BASE=${MERGED%.*}
>> ext=.${MERGED##*.}
>>
>> 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=foo.bar.baz && echo ${MERGED%.*} ${MERGED##*.}
> 	foo.bar baz
> 
> Good.
> 
> 	$ MERGED=foo && echo ${MERGED%.*} ${MERGED##*.}
> 	foo foo
> 
> Bad.
> There's no extension and the substitution doesn't handle it.
> 
> 	$ MERGED=foo.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 extension.
> 
> 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:
First, you extract the last path part:
	file=${MERGED##*/}
Then the directory including the trailing slash:
	dir=${MERGED%"$file"}
Then the basename without an extension:
	base=${file%.*}
Finally the extension including the dot:
	ext=${file#"$base"}
Beware of empty $base and $dir (e.g., for files named .gitignore or so)
-- Hannes
Junio C Hamano· Oct 10, 2014, 21:16 UTC · re: Jakub Narębski · lore

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

Jakub Narębski <jnareb@gmail.com> writes:
Show 6 quoted lines
> Why use expr and not POSIX shell parameter substitution?
>
> BASE=${MERGED%.*}
> ext=.${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· Oct 13, 2014, 19:30 UTC · re: David Aguilar · lore

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

David Aguilar <davvid@gmail.com> writes:
Show 24 quoted lines
> 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>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: David Aguilar <davvid@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.
;-)

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

Show 31 quoted lines
>
>  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;}')

← back to recent threads