[PATCH] Make sure to use Araxis' compare and not e.g. ImageMagick's

2012-07-23 Thread Sebastian Schuberth 
Signed-off-by: Sebastian Schuberth sschube...@gmail.com
---
 mergetools/araxis | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/mergetools/araxis b/mergetools/araxis
index 64f97c5..f8899f8 100644
--- a/mergetools/araxis
+++ b/mergetools/araxis
@@ -16,5 +16,12 @@ merge_cmd () {
 }
 
 translate_merge_tool_path() {
-   echo compare
+   # Only accept compare in a path that contains araxis to not
+   # accidently use e.g. ImageMagick's compare.
+   if type compare | grep -i araxis /dev/null 21
+   then
+   echo compare
+   else
+   echo $1
+   fi
 }
-- 
1.7.11.msysgit.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Make sure to use Araxis' compare and not e.g. ImageMagick's

2012-07-23 Thread Junio C Hamano 
Sebastian Schuberth sschube...@gmail.com writes:

 Signed-off-by: Sebastian Schuberth sschube...@gmail.com
 ---
  mergetools/araxis | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

 diff --git a/mergetools/araxis b/mergetools/araxis
 index 64f97c5..f8899f8 100644
 --- a/mergetools/araxis
 +++ b/mergetools/araxis
 @@ -16,5 +16,12 @@ merge_cmd () {
  }
  
  translate_merge_tool_path() {
 - echo compare
 + # Only accept compare in a path that contains araxis to not
 + # accidently use e.g. ImageMagick's compare.
 + if type compare | grep -i araxis /dev/null 21
 + then
 + echo compare
 + else
 + echo $1
 + fi
  }

I do not use araxis, and I do not know how rigid its installation
procedure is to prevent the end user from naming a path that does
not have the string in it.

For example, when the user tells it to install in /home/ss/bin, if
it installs its compare program in /home/ss/bin/araxis/compare
without allowing the /araxis/ part to be stripped away, the above
heuristics is sufficiently safe.  Otherwise, it is not.

It is unclear from your proposed commit log message what assurance
do we have that it is installed under such a path and why the
heuristics the patch implements is the sane way forward.

Please convince me.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Make sure to use Araxis' compare and not e.g. ImageMagick's

2012-07-23 Thread Sebastian Schuberth 
On Mon, Jul 23, 2012 at 10:47 PM, Junio C Hamano gits...@pobox.com wrote:

 For example, when the user tells it to install in /home/ss/bin, if
 it installs its compare program in /home/ss/bin/araxis/compare
 without allowing the /araxis/ part to be stripped away, the above
 heuristics is sufficiently safe.  Otherwise, it is not.

To the best of my knowledge, Araxis does not enforce any naming
convention for the path it gets installed in. That means the user may
indeed install the program in a path that does not contain araxis. I
was aware of this when writing the patch, but should have probably
made it more clear in the commit message.

 It is unclear from your proposed commit log message what assurance
 do we have that it is installed under such a path and why the
 heuristics the patch implements is the sane way forward.

We have no such assurance. That's why you correctly call it a
heuristics after all: it may fail. Personally, I've valued the gain of
the patch to not list araxis as an available diff tool by git
difftool --tool-help when in fact just ImageMagick is in PATH higher
than the loss to support araxis installations that are in a path not
containung araxis but are in PATH.

Please feel free to ignore the patch if you feel the heuristics is not
sufficiently safe. I'm currently unable to come up with a safer
solution while maintaining portability, i.e. not use which or doing
rather laborious string parsing on the output of type.

-- 
Sebastian Schuberth
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Make sure to use Araxis' compare and not e.g. ImageMagick's

2012-07-23 Thread Andreas Schwab 
Sebastian Schuberth sschube...@gmail.com writes:

 Please feel free to ignore the patch if you feel the heuristics is not
 sufficiently safe. I'm currently unable to come up with a safer
 solution while maintaining portability, i.e. not use which or doing
 rather laborious string parsing on the output of type.

How about looking at compare --version?

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Make sure to use Araxis' compare and not e.g. ImageMagick's

2012-07-23 Thread Sebastian Schuberth 
On Mon, Jul 23, 2012 at 11:24 PM, Junio C Hamano gits...@pobox.com wrote:

 Does Araxis compare take --version and behave in a way that is
 cheaply controllable?  If it opens a GUI window and pops up a dialog
 that says Option not understood, then it is not controllable,

Araxis opens up a GUI dialog with usage info in that case, so it's not
controllable, unfortunately.

-- 
Sebastian Schuberth
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Make sure to use Araxis' compare and not e.g. ImageMagick's

2012-07-23 Thread David Aguilar 
On Mon, Jul 23, 2012 at 2:24 PM, Junio C Hamano gits...@pobox.com wrote:
 Sebastian Schuberth sschube...@gmail.com writes:

 We have no such assurance. That's why you correctly call it a
 heuristics after all

 ImageMagick compare takes --version and says something like
 this to its standard output:

 $ compare --version
 Version: ImageMagick 6.6.0-4 2012-05-02 Q16
 http://www.imagemagick.org
 Copyright: Copyright (C) 1999-2010 ImageMagick Studio LLC

 Does Araxis compare take --version and behave in a way that is
 cheaply controllable?  If it opens a GUI window and pops up a dialog
 that says Option not understood, then it is not controllable,
 but if it quickly dies with No such option sent to the standard
 error output, or sending its version string to the standard output,
 then we could use something like:

 case $(compare --version 2/dev/null) in
 Araxis compare version*)
 echo compare ;;
 *)
 echo $1 ;;
 esac

 instead, and that would be more robust than the path based
 heuristics.

Araxis compare (the one I have) does not accept --version.

Also, the GraphicsMagick (ImageMagick fork) compare does not have
--version, but it does have compare version:

$ compare version
GraphicsMagick 1.3.12 2010-03-08 Q8 http://www.GraphicsMagick.org/
Copyright (C) 2002-2010 GraphicsMagick Group.
Additional copyrights and licenses apply to this software.
See http://www.GraphicsMagick.org/www/Copyright.html for details.
...


If we care to blacklist *Magick compare, then we may be able to call
compare version and parse the output looking for Magic.

I tested ImageMagick compare and it also understands compare version.


Araxis compare prints nothing when compare version is called.
Likely because it thinks it has nothing to do.  It does the same if I
say compare blahblah, and returns exit status 0.  So its output is
useless.

It seems like the best we can do is specifically blacklist the *Magick
compare commands so that they do not show up as false positives.
And the only way to identify them is to parse their output, since all
commands return status 0 for compare version.


Another possibility is to parse the output of compare (no args) and
grep for merge.  The name Araxis Merge is never actually printed,
but the help text for -merge does appear yep.. it's a heuristic.

Sebastian, are you testing on Windows?  The araxis compare I used is
OS X.  Does compare version open a GUI window for you?  For me it
does not.
What about compare -h, or just compare ?
-- 
David
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Make sure to use Araxis' compare and not e.g. ImageMagick's

2012-07-23 Thread Junio C Hamano 
Sebastian Schuberth sschube...@gmail.com writes:

 Personally, I've valued the gain of
 the patch to not list araxis as an available diff tool by git
 difftool --tool-help when in fact just ImageMagick is in PATH higher
 than the loss to support araxis installations that are in a path not
 containung araxis but are in PATH.

I agree that running ImageMagick's compare by accident is one thing
we would want to avoid, but once the problem is diagnosed, it is
something the user can easily work around by futzing %PATH%, I
think.

On the other hand, if the user has bought and installed Araxis, but
we incorrectly identify it as unusable, the user has wasted good
money and there is no easy recourse as far as I can see in your
patch.  That is why I wanted to see a reasonable assurance.

If we limit the problem space by special casing Windows installation
(e.g. check uname -s or something), would it make the problem
easier to solve?  Perhaps it is much more likely that the path the
program is installed in can be safely identified with a call to
type --path compare (bash is the only shell shipped in msysgit,
isn't it?), and its output is likely to contain /Program Files/Araxis/
as a substring, or something?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Make sure to use Araxis' compare and not e.g. ImageMagick's

2012-07-23 Thread Sebastian Schuberth 
On Tue, Jul 24, 2012 at 12:22 AM, Junio C Hamano gits...@pobox.com wrote:

 On the other hand, if the user has bought and installed Araxis, but
 we incorrectly identify it as unusable, the user has wasted good
 money and there is no easy recourse as far as I can see in your
 patch.

Agreed.

 If we limit the problem space by special casing Windows installation
 (e.g. check uname -s or something), would it make the problem
 easier to solve?  Perhaps it is much more likely that the path the
 program is installed in can be safely identified with a call to
 type --path compare (bash is the only shell shipped in msysgit,
 isn't it?), and its output is likely to contain /Program Files/Araxis/
 as a substring, or something?

Yes, I could come up with basically a variant of my first version of
the patch that is limited to Windows only and uses type --path
instead of which, but then again this is too non-generic for my
taste. Moreover, as I'm also using Mac OS X, I'd be interested in a
solution that works there, too.

I don't see a good (read generic and concise) solution to the issue. I
think we should just drop my patch and not waste all of our time on it
any more.

-- 
Sebastian Schuberth
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Make sure to use Araxis' compare and not e.g. ImageMagick's

2012-07-23 Thread Sebastian Schuberth 
On 24.07.2012 00:41, Junio C Hamano wrote:

 + if test -f $(dirname $(type --path compare))/AraxisMerge

We would need additional quotes around the whole path here as the Windows 
installation path is usually something like C:\Program Files\Araxis\Araxis 
Merge and contains spaces.

Moreover, test -f requires the .exe extension to be explicitly present for 
the file to test. But I'd rather not do that because the test would be specific 
to Windows then and e.g. not work on Mac OS X. That's why I'd still like to use 
ls like in my first patch:

 mergetools/araxis | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/mergetools/araxis b/mergetools/araxis
index 64f97c5..c406ead 100644
--- a/mergetools/araxis
+++ b/mergetools/araxis
@@ -16,5 +16,18 @@ merge_cmd () {
 }
 
 translate_merge_tool_path() {
-   echo compare
+   case $BASH_VERSION in
+   ??*)
+   # we can safely use type --path
+   if ls $(dirname $(type --path compare))/Araxis* /dev/null 
21
+   then
+   echo compare
+   else
+   echo $1
+   fi
+   ;;
+   *)
+   echo compare
+   ;;
+   esac
 }

-- 
Sebastian Schuberth
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html