{"thread":{"id":"37173","subject":"[PATCH 1/2] mergetool: don't require a work tree for --tool-help","startedAt":"2014-07-19T16:35:16Z","lastAt":"2014-07-29T08:03:23Z","messageCount":6,"participants":["Charles Bailey","John Keeping","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"246384","messageId":"1405787717-30476-1-git-send-email-charles@hashpling.org","threadId":"37173","inReplyTo":null,"subject":"[PATCH 1/2] mergetool: don't require a work tree for --tool-help","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2014-07-19T16:35:16Z","receivedAt":"2014-07-19T16:35:16Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\nSigned-off-by: Charles Bailey <cbailey32@bloomberg.net>\n---\nYou can call git difftool --tool-help outside of a work tree but not\nmergetool --tool-help but there's not real reason for this restriction\nand it can be easily relaxed by deferring the require_work_tree call\nuntil after the options have been parsed.\n\n git-mergetool.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 9a046b7..e969dd0 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -14,7 +14,6 @@ OPTIONS_SPEC=\n TOOL_MODE=merge\n . git-sh-setup\n . git-mergetool--lib\n-require_work_tree\n \n # Returns true if the mode reflects a symlink\n is_symlink () {\n@@ -372,6 +371,8 @@ prompt_after_failed_merge () {\n \tdone\n }\n \n+require_work_tree\n+\n if test -z \"$merge_tool\"\n then\n \t# Check if a merge tool has been configured\n-- \n2.0.2.611.g8c85416\n"},{"id":"246385","messageId":"1405787717-30476-2-git-send-email-charles@hashpling.org","threadId":"37173","inReplyTo":"1405787717-30476-1-git-send-email-charles@hashpling.org","subject":"[PATCH 2/2] difftool: don't assume that default sh is sane","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2014-07-19T16:35:17Z","receivedAt":"2014-07-19T16:35:17Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\ngit-difftool used to create a command list script containing $( ... )\nand explicitly call \"sh -c\" with this list.\n\nInstead, allow mergetool --tool-help to take a mode parameter and call\nmergetool directly to invoke the show_tool_help function. This mode\nparameter is intented for use solely by difftool.\n\nSigned-off-by: Charles Bailey <cbailey32@bloomberg.net>\n---\nAnother issue for Solaris. Originally I had a fix for this that\nsubstituted \"@SHELL_PATH@\" even inside perl scripts but I felt that\nhaving an interface for show_tool_help was a little neater all round but\nI welcome alternative views.\n\n git-difftool.perl |  6 +-----\n git-mergetool.sh  | 12 +++++++++++-\n 2 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 18ca61e..598fcc2 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -47,13 +47,9 @@ sub find_worktree\n \n sub print_tool_help\n {\n-\tmy $cmd = 'TOOL_MODE=diff';\n-\t$cmd .= ' && . \"$(git --exec-path)/git-mergetool--lib\"';\n-\t$cmd .= ' && show_tool_help';\n-\n \t# See the comment at the bottom of file_diff() for the reason behind\n \t# using system() followed by exit() instead of exec().\n-\tmy $rc = system('sh', '-c', $cmd);\n+\tmy $rc = system(qw(git mergetool --tool-help=diff));\n \texit($rc | ($rc >> 8));\n }\n \ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex e969dd0..d32b663 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -320,7 +320,17 @@ guessed_merge_tool=false\n while test $# != 0\n do\n \tcase \"$1\" in\n-\t--tool-help)\n+\t--tool-help*)\n+\t\tcase \"$#,$1\" in\n+\t\t1,*=*)\n+\t\t\tTOOL_MODE=$(expr \"z$1\" : 'z-[^=]*=\\(.*\\)')\n+\t\t\t;;\n+\t\t1,--tool-help)\n+\t\t\t;;\n+\t\t*)\n+\t\t\tusage\n+\t\t\t;;\n+\t\tesac\n \t\tshow_tool_help\n \t\t;;\n \t-t|--tool*)\n-- \n2.0.2.611.g8c85416\n"},{"id":"246388","messageId":"20140719172132.GB26927@serenity.lan","threadId":"37173","inReplyTo":"1405787717-30476-2-git-send-email-charles@hashpling.org","subject":"Re: [PATCH 2/2] difftool: don't assume that default sh is sane","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-07-19T17:21:32Z","receivedAt":"2014-07-19T17:21:32Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sat, Jul 19, 2014 at 05:35:17PM +0100, Charles Bailey wrote:\n> From: Charles Bailey <cbailey32@bloomberg.net>\n> \n> git-difftool used to create a command list script containing $( ... )\n> and explicitly call \"sh -c\" with this list.\n> \n> Instead, allow mergetool --tool-help to take a mode parameter and call\n> mergetool directly to invoke the show_tool_help function. This mode\n> parameter is intented for use solely by difftool.\n> \n> Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>\n> ---\n> Another issue for Solaris. Originally I had a fix for this that\n> substituted \"@SHELL_PATH@\" even inside perl scripts but I felt that\n> having an interface for show_tool_help was a little neater all round but\n> I welcome alternative views.\n> \n>  git-difftool.perl |  6 +-----\n>  git-mergetool.sh  | 12 +++++++++++-\n>  2 files changed, 12 insertions(+), 6 deletions(-)\n> \n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 18ca61e..598fcc2 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -47,13 +47,9 @@ sub find_worktree\n>  \n>  sub print_tool_help\n>  {\n> -\tmy $cmd = 'TOOL_MODE=diff';\n> -\t$cmd .= ' && . \"$(git --exec-path)/git-mergetool--lib\"';\n> -\t$cmd .= ' && show_tool_help';\n> -\n>  \t# See the comment at the bottom of file_diff() for the reason behind\n>  \t# using system() followed by exit() instead of exec().\n> -\tmy $rc = system('sh', '-c', $cmd);\n> +\tmy $rc = system(qw(git mergetool --tool-help=diff));\n>  \texit($rc | ($rc >> 8));\n>  }\n>  \n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index e969dd0..d32b663 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -320,7 +320,17 @@ guessed_merge_tool=false\n>  while test $# != 0\n>  do\n>  \tcase \"$1\" in\n> -\t--tool-help)\n> +\t--tool-help*)\n> +\t\tcase \"$#,$1\" in\n> +\t\t1,*=*)\n\nWhat's the reason for forcing `--tool-help` to be the last option?\nWouldn't it be simpler to just change the top-level case statement to:\n\n\t--tool-help=*)\n\t\tTOOL_MODE=${1#--tool-help=}\n\t\tshow_tool_help\n\t\t;;\n\t--tool-help)\n\t\tshow_tool_help\n\t\t;;\n\n> +\t\t\tTOOL_MODE=$(expr \"z$1\" : 'z-[^=]*=\\(.*\\)')\n> +\t\t\t;;\n> +\t\t1,--tool-help)\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\tusage\n> +\t\t\t;;\n> +\t\tesac\n>  \t\tshow_tool_help\n>  \t\t;;\n>  \t-t|--tool*)\n> -- \n> 2.0.2.611.g8c85416\n"},{"id":"246393","messageId":"20140719182950.GA31037@hashpling.org","threadId":"37173","inReplyTo":"20140719172132.GB26927@serenity.lan","subject":"Re: [PATCH 2/2] difftool: don't assume that default sh is sane","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2014-07-19T18:29:50Z","receivedAt":"2014-07-19T18:29:50Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sat, Jul 19, 2014 at 06:21:32PM +0100, John Keeping wrote:\n> \n> What's the reason for forcing `--tool-help` to be the last option?\n> Wouldn't it be simpler to just change the top-level case statement to:\n> \n> \t--tool-help=*)\n> \t\tTOOL_MODE=${1#--tool-help=}\n> \t\tshow_tool_help\n> \t\t;;\n> \t--tool-help)\n> \t\tshow_tool_help\n> \t\t;;\n\nIt doesn't make sense to use --tool-help with other parameters so issuing\nan error made sense to me at the time. You've pointed out to me that I\ndon't error when those other options come first so I'm now unsure how\nvaluable this behaviour is, now. (I can't immediately see a really neat way\nto give a diagnostic if other options do come first.)\n\nYour version is good, obviously simpler.\n"},{"id":"246877","messageId":"20140729075328.GA20724@gmail.com","threadId":"37173","inReplyTo":"1405787717-30476-2-git-send-email-charles@hashpling.org","subject":"Re: [PATCH 2/2] difftool: don't assume that default sh is sane","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-07-29T07:53:29Z","receivedAt":"2014-07-29T07:53:29Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Jul 19, 2014 at 05:35:17PM +0100, Charles Bailey wrote:\n> From: Charles Bailey <cbailey32@bloomberg.net>\n> \n> git-difftool used to create a command list script containing $( ... )\n> and explicitly call \"sh -c\" with this list.\n> \n> Instead, allow mergetool --tool-help to take a mode parameter and call\n> mergetool directly to invoke the show_tool_help function. This mode\n> parameter is intented for use solely by difftool.\n> \n> Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>\n> ---\n> Another issue for Solaris. Originally I had a fix for this that\n> substituted \"@SHELL_PATH@\" even inside perl scripts but I felt that\n> having an interface for show_tool_help was a little neater all round but\n> I welcome alternative views.\n\nI definitely agree that having an interface is nice and tidy.\n\n>  git-difftool.perl |  6 +-----\n>  git-mergetool.sh  | 12 +++++++++++-\n>  2 files changed, 12 insertions(+), 6 deletions(-)\n> \n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 18ca61e..598fcc2 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -47,13 +47,9 @@ sub find_worktree\n>  \n>  sub print_tool_help\n>  {\n> -\tmy $cmd = 'TOOL_MODE=diff';\n> -\t$cmd .= ' && . \"$(git --exec-path)/git-mergetool--lib\"';\n> -\t$cmd .= ' && show_tool_help';\n> -\n>  \t# See the comment at the bottom of file_diff() for the reason behind\n>  \t# using system() followed by exit() instead of exec().\n> -\tmy $rc = system('sh', '-c', $cmd);\n> +\tmy $rc = system(qw(git mergetool --tool-help=diff));\n\nI believe qw() in list context is considered deprecated.\n\ncheers,\n-- \nDavid\n"},{"id":"246880","messageId":"20140729080322.GB20724@gmail.com","threadId":"37173","inReplyTo":"20140729075328.GA20724@gmail.com","subject":"Re: [PATCH 2/2] difftool: don't assume that default sh is sane","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-07-29T08:03:23Z","receivedAt":"2014-07-29T08:03:23Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Jul 29, 2014 at 12:53:29AM -0700, David Aguilar wrote:\n> On Sat, Jul 19, 2014 at 05:35:17PM +0100, Charles Bailey wrote:\n> > diff --git a/git-difftool.perl b/git-difftool.perl\n> > index 18ca61e..598fcc2 100755\n> > --- a/git-difftool.perl\n> > +++ b/git-difftool.perl\n> > @@ -47,13 +47,9 @@ sub find_worktree\n> >  \n> >  sub print_tool_help\n> >  {\n> > -\tmy $cmd = 'TOOL_MODE=diff';\n> > -\t$cmd .= ' && . \"$(git --exec-path)/git-mergetool--lib\"';\n> > -\t$cmd .= ' && show_tool_help';\n> > -\n> >  \t# See the comment at the bottom of file_diff() for the reason behind\n> >  \t# using system() followed by exit() instead of exec().\n> > -\tmy $rc = system('sh', '-c', $cmd);\n> > +\tmy $rc = system(qw(git mergetool --tool-help=diff));\n> \n> I believe qw() in list context is considered deprecated.\n\nSorry for the noise, I got my warnings mixed up.\nIt's only deprecated when used as parentheses, so this is fine as-is.\n-- \nDavid\n"}]}