Re: [PATCH 2/2] difftool: don't assume that default sh is sane
On Tue, Jul 29, 2014 at 12:53:29AM -0700, David Aguilar wrote: > On Sat, Jul 19, 2014 at 05:35:17PM +0100, Charles Bailey wrote: > > diff --git a/git-difftool.perl b/git-difftool.perl > > index 18ca61e..598fcc2 100755 > > --- a/git-difftool.perl > > +++ b/git-difftool.perl > > @@ -47,13 +47,9 @@ sub find_worktree > > > > sub print_tool_help > > { > > - my $cmd = 'TOOL_MODE=diff'; > > - $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"'; > > - $cmd .= ' && show_tool_help'; > > - > > # See the comment at the bottom of file_diff() for the reason behind > > # using system() followed by exit() instead of exec(). > > - my $rc = system('sh', '-c', $cmd); > > + my $rc = system(qw(git mergetool --tool-help=diff)); > > I believe qw() in list context is considered deprecated. Sorry for the noise, I got my warnings mixed up. It's only deprecated when used as parentheses, so this is fine as-is. -- 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 2/2] difftool: don't assume that default sh is sane
On Sat, Jul 19, 2014 at 05:35:17PM +0100, Charles Bailey wrote: > From: Charles Bailey > > git-difftool used to create a command list script containing $( ... ) > and explicitly call "sh -c" with this list. > > Instead, allow mergetool --tool-help to take a mode parameter and call > mergetool directly to invoke the show_tool_help function. This mode > parameter is intented for use solely by difftool. > > Signed-off-by: Charles Bailey > --- > Another issue for Solaris. Originally I had a fix for this that > substituted "@SHELL_PATH@" even inside perl scripts but I felt that > having an interface for show_tool_help was a little neater all round but > I welcome alternative views. I definitely agree that having an interface is nice and tidy. > git-difftool.perl | 6 +- > git-mergetool.sh | 12 +++- > 2 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/git-difftool.perl b/git-difftool.perl > index 18ca61e..598fcc2 100755 > --- a/git-difftool.perl > +++ b/git-difftool.perl > @@ -47,13 +47,9 @@ sub find_worktree > > sub print_tool_help > { > - my $cmd = 'TOOL_MODE=diff'; > - $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"'; > - $cmd .= ' && show_tool_help'; > - > # See the comment at the bottom of file_diff() for the reason behind > # using system() followed by exit() instead of exec(). > - my $rc = system('sh', '-c', $cmd); > + my $rc = system(qw(git mergetool --tool-help=diff)); I believe qw() in list context is considered deprecated. cheers, -- 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 2/2] difftool: don't assume that default sh is sane
On Sat, Jul 19, 2014 at 06:21:32PM +0100, John Keeping wrote: > > What's the reason for forcing `--tool-help` to be the last option? > Wouldn't it be simpler to just change the top-level case statement to: > > --tool-help=*) > TOOL_MODE=${1#--tool-help=} > show_tool_help > ;; > --tool-help) > show_tool_help > ;; It doesn't make sense to use --tool-help with other parameters so issuing an error made sense to me at the time. You've pointed out to me that I don't error when those other options come first so I'm now unsure how valuable this behaviour is, now. (I can't immediately see a really neat way to give a diagnostic if other options do come first.) Your version is good, obviously simpler. -- 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 2/2] difftool: don't assume that default sh is sane
On Sat, Jul 19, 2014 at 05:35:17PM +0100, Charles Bailey wrote: > From: Charles Bailey > > git-difftool used to create a command list script containing $( ... ) > and explicitly call "sh -c" with this list. > > Instead, allow mergetool --tool-help to take a mode parameter and call > mergetool directly to invoke the show_tool_help function. This mode > parameter is intented for use solely by difftool. > > Signed-off-by: Charles Bailey > --- > Another issue for Solaris. Originally I had a fix for this that > substituted "@SHELL_PATH@" even inside perl scripts but I felt that > having an interface for show_tool_help was a little neater all round but > I welcome alternative views. > > git-difftool.perl | 6 +- > git-mergetool.sh | 12 +++- > 2 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/git-difftool.perl b/git-difftool.perl > index 18ca61e..598fcc2 100755 > --- a/git-difftool.perl > +++ b/git-difftool.perl > @@ -47,13 +47,9 @@ sub find_worktree > > sub print_tool_help > { > - my $cmd = 'TOOL_MODE=diff'; > - $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"'; > - $cmd .= ' && show_tool_help'; > - > # See the comment at the bottom of file_diff() for the reason behind > # using system() followed by exit() instead of exec(). > - my $rc = system('sh', '-c', $cmd); > + my $rc = system(qw(git mergetool --tool-help=diff)); > exit($rc | ($rc >> 8)); > } > > diff --git a/git-mergetool.sh b/git-mergetool.sh > index e969dd0..d32b663 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -320,7 +320,17 @@ guessed_merge_tool=false > while test $# != 0 > do > case "$1" in > - --tool-help) > + --tool-help*) > + case "$#,$1" in > + 1,*=*) What's the reason for forcing `--tool-help` to be the last option? Wouldn't it be simpler to just change the top-level case statement to: --tool-help=*) TOOL_MODE=${1#--tool-help=} show_tool_help ;; --tool-help) show_tool_help ;; > + TOOL_MODE=$(expr "z$1" : 'z-[^=]*=\(.*\)') > + ;; > + 1,--tool-help) > + ;; > + *) > + usage > + ;; > + esac > show_tool_help > ;; > -t|--tool*) > -- > 2.0.2.611.g8c85416 -- 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
[PATCH 2/2] difftool: don't assume that default sh is sane
From: Charles Bailey git-difftool used to create a command list script containing $( ... ) and explicitly call "sh -c" with this list. Instead, allow mergetool --tool-help to take a mode parameter and call mergetool directly to invoke the show_tool_help function. This mode parameter is intented for use solely by difftool. Signed-off-by: Charles Bailey --- Another issue for Solaris. Originally I had a fix for this that substituted "@SHELL_PATH@" even inside perl scripts but I felt that having an interface for show_tool_help was a little neater all round but I welcome alternative views. git-difftool.perl | 6 +- git-mergetool.sh | 12 +++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/git-difftool.perl b/git-difftool.perl index 18ca61e..598fcc2 100755 --- a/git-difftool.perl +++ b/git-difftool.perl @@ -47,13 +47,9 @@ sub find_worktree sub print_tool_help { - my $cmd = 'TOOL_MODE=diff'; - $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"'; - $cmd .= ' && show_tool_help'; - # See the comment at the bottom of file_diff() for the reason behind # using system() followed by exit() instead of exec(). - my $rc = system('sh', '-c', $cmd); + my $rc = system(qw(git mergetool --tool-help=diff)); exit($rc | ($rc >> 8)); } diff --git a/git-mergetool.sh b/git-mergetool.sh index e969dd0..d32b663 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -320,7 +320,17 @@ guessed_merge_tool=false while test $# != 0 do case "$1" in - --tool-help) + --tool-help*) + case "$#,$1" in + 1,*=*) + TOOL_MODE=$(expr "z$1" : 'z-[^=]*=\(.*\)') + ;; + 1,--tool-help) + ;; + *) + usage + ;; + esac show_tool_help ;; -t|--tool*) -- 2.0.2.611.g8c85416 -- 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