# [BUG] git gui blame fails for multi-word textconv filter 9 messages from 2010-08-04 to 2010-08-19. Participants: Kirill Smelkov, Clément Poulain, Matthieu Moy, Pat Thoyts. Thread: https://gitlist.dev/t/24635 ## Kirill Smelkov, 2010-08-04 19:25 Subject: [BUG] git gui blame fails for multi-word textconv filter Message-ID: <20100804192525.GA13086@landau.phys.spbu.ru> URL: https://gitlist.dev/e/20100804192525.GA13086%40landau.phys.spbu.ru ``` Hello, I use [diff "astextplain"] textconv = run-mailcap --action=cat in my ~/.gitconfig, and this works for git `git blame` because of 41a457 in git.git (textconv: use shell to run helper), but fails with git gui: $ git gui blame 21980.2--ИМС-МР231.doc Error in startup script: couldn't execute "run-mailcap --action=cat": no such file or directory while executing "open |[list $textconv $path] r" (procedure "_load" line 56) invoked from within "_load $this $i_jump" (procedure "blame::new" line 185) invoked from within "blame::new $head $path $jump_spec" ("blame" arm line 6) invoked from within "switch -- $subcommand { browser { if {$jump_spec ne {}} usage if {$head eq {}} { if {$path ne {} && [file isdirectory $path]} { set head $..." ("blame" arm line 57) invoked from within "switch -- $subcommand { browser - blame { if {$subcommand eq "blame"} { set subcommand_args {[--line=] rev? path} } else { set subcommand_a..." (file "/home/kirr/local/git/libexec/git-core/git-gui" line 2868) Thats is maybe because we use `git cat-file --textconv` only for in .git entries, but since cat-file lacks support for work-tree git-gui calls textconv filter itself manually on initial "$commit eq {}"? If so, I'd better teach cat-file about worktree, instead of teaching git-gui about running textconv filter through shell. Just a wish... Thanks, Kirill P.S. And thanks for finally collecting all those git-gui patches. ``` ## Clément Poulain, 2010-08-04 23:46 Subject: Re: [BUG] git gui blame fails for multi-word textconv filter Message-ID: <4C59FBD5.5090209@ensimag.imag.fr> URL: https://gitlist.dev/e/4C59FBD5.5090209%40ensimag.imag.fr In-Reply-To: <20100804192525.GA13086@landau.phys.spbu.ru> ``` Le 04/08/2010 21:25, Kirill Smelkov a écrit : > Hello, > > I use > > [diff "astextplain"] > textconv = run-mailcap --action=cat > > in my ~/.gitconfig, and this works for git `git blame` because of 41a457 > in git.git (textconv: use shell to run helper), but fails with git gui: > > $ git gui blame 21980.2--ИМС-МР231.doc > Error in startup script: couldn't execute "run-mailcap --action=cat": no such file or directory > I wonder if spaces can be the reason of this. Looks like Tcl is looking for an executable called "run-mailcap --action=cat", and doesn't distinguish path from options. I do not have much experience with Tcl, so I can't figure out how to solve that. Some help would be appreciate :-) > while executing > "open |[list $textconv $path] r" > (procedure "_load" line 56) > invoked from within > "_load $this $i_jump" > (procedure "blame::new" line 185) > invoked from within > "blame::new $head $path $jump_spec" > ("blame" arm line 6) > invoked from within > "switch -- $subcommand { > browser { > if {$jump_spec ne {}} usage > if {$head eq {}} { > if {$path ne {}&& [file isdirectory $path]} { > set head $..." > ("blame" arm line 57) > invoked from within > "switch -- $subcommand { > browser - > blame { > if {$subcommand eq "blame"} { > set subcommand_args {[--line=] rev? path} > } else { > set subcommand_a..." > (file "/home/kirr/local/git/libexec/git-core/git-gui" line 2868) > > > > Thats is maybe because we use `git cat-file --textconv` only for in .git > entries, but since cat-file lacks support for work-tree git-gui calls > textconv filter itself manually on initial "$commit eq {}"? > Yep, "open |[list $textconv $path] r" is the way we call textconv on the work-tree copy of the concerned file. > If so, I'd better teach cat-file about worktree, instead of teaching > git-gui about running textconv filter through shell. Just a wish... > This was discussed here: a1ace6b77167a2ad4b4995e8c4d09761@ensimag.fr , and your suggestion was considered ;-) ``` ## Matthieu Moy, 2010-08-05 09:59 Subject: Re: [BUG] git gui blame fails for multi-word textconv filter Message-ID: URL: https://gitlist.dev/e/vpqlj8l2xd5.fsf%40bauges.imag.fr In-Reply-To: <4C59FBD5.5090209@ensimag.imag.fr> ``` Clément Poulain writes: > I wonder if spaces can be the reason of this. Looks like Tcl is > looking for an executable called "run-mailcap --action=cat", and > doesn't distinguish path from options. Yes, and it does this because we asked it to do so. Whitespace interpretation is done by the shell, and there's no shell involved here. > I do not have much experience with Tcl, so I can't figure out how to > solve that. Some help would be appreciate :-) Patch follows. It just runs the textconv using a shell, which does whitespace interpretation (and more if needed). Tested with textconv=odt2txt --width=40 and a file containing whitespaces. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Matthieu Moy, 2010-08-05 10:05 Subject: [PATCH] git-gui: Use shell to launch textconv filter in "blame" Message-ID: <1281002722-3042-1-git-send-email-Matthieu.Moy@imag.fr> URL: https://gitlist.dev/e/1281002722-3042-1-git-send-email-Matthieu.Moy%40imag.fr In-Reply-To: ``` This allows one to use textconv commands with arguments. Signed-off-by: Matthieu Moy --- git-gui/Makefile | 1 + git-gui/git-gui.sh | 6 ++++++ git-gui/lib/blame.tcl | 4 +++- 3 files changed, 10 insertions(+), 1 deletions(-) diff --git a/git-gui/Makefile b/git-gui/Makefile index 197b55e..e22ba5c 100644 --- a/git-gui/Makefile +++ b/git-gui/Makefile @@ -215,6 +215,7 @@ endif $(GITGUI_MAIN): git-gui.sh GIT-VERSION-FILE GIT-GUI-VARS $(QUIET_GEN)rm -f $@ $@+ && \ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ + -e 's|@@SHELL_PATH@@|$(SHELL_PATH_SQ)|' \ -e '1,30s|^ argv0=$$0| argv0=$(GITGUI_SCRIPT)|' \ -e '1,30s|^ exec wish | exec '\''$(TCLTK_PATH_SED)'\'' |' \ -e 's/@@GITGUI_VERSION@@/$(GITGUI_VERSION)/g' \ diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh index bb10489..9049abf 100755 --- a/git-gui/git-gui.sh +++ b/git-gui/git-gui.sh @@ -128,6 +128,7 @@ set _githtmldir {} set _reponame {} set _iscygwin {} set _search_path {} +set _shellpath {@@SHELL_PATH@@} set _trace [lsearch -exact $argv --trace] if {$_trace >= 0} { @@ -137,6 +138,11 @@ if {$_trace >= 0} { set _trace 0 } +proc shellpath {} { + global _shellpath + return $_shellpath +} + proc appname {} { global _appname return $_appname diff --git a/git-gui/lib/blame.tcl b/git-gui/lib/blame.tcl index 2137ec9..77656d3 100644 --- a/git-gui/lib/blame.tcl +++ b/git-gui/lib/blame.tcl @@ -460,7 +460,9 @@ method _load {jump} { } if {$commit eq {}} { if {$do_textconv ne 0} { - set fd [open |[list $textconv $path] r] + # Run textconv with sh -c "..." to allow it to + # contain command + arguments. + set fd [open |[list [shellpath] -c "$textconv \"\$0\"" $path] r] } else { set fd [open $path r] } -- 1.7.2.1.30.g18195 ``` ## Pat Thoyts, 2010-08-05 22:58 Subject: Re: [BUG] git gui blame fails for multi-word textconv filter Message-ID: <87iq3osm3h.fsf@fox.patthoyts.tk> URL: https://gitlist.dev/e/87iq3osm3h.fsf%40fox.patthoyts.tk In-Reply-To: <4C59FBD5.5090209@ensimag.imag.fr> ``` Clément Poulain writes: >Le 04/08/2010 21:25, Kirill Smelkov a écrit : >> Hello, >> >> I use >> >> [diff "astextplain"] >> textconv = run-mailcap --action=cat >> >> in my ~/.gitconfig, and this works for git `git blame` because of 41a457 >> in git.git (textconv: use shell to run helper), but fails with git gui: >> >> $ git gui blame 21980.2--ИМС-МР231.doc >> Error in startup script: couldn't execute "run-mailcap --action=cat": no such file or directory >> >I wonder if spaces can be the reason of this. Looks like Tcl is >looking for an executable called "run-mailcap --action=cat", and >doesn't distinguish path from options. indeed. We passed a 2-element list and the first element is the command. To permit this we need to append the pathname to the command instead. open |[linsert $path 0 $cmd_plus_args_list] is a safe way to do that. I should have thought of that too and not just repos with spaces in the path. -- Pat Thoyts http://www.patthoyts.tk/ PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD ``` ## Pat Thoyts, 2010-08-05 23:10 Subject: Re: [PATCH] git-gui: Use shell to launch textconv filter in "blame" Message-ID: <87aap0sljs.fsf@fox.patthoyts.tk> URL: https://gitlist.dev/e/87aap0sljs.fsf%40fox.patthoyts.tk In-Reply-To: <1281002722-3042-1-git-send-email-Matthieu.Moy@imag.fr> ``` Matthieu Moy writes: >This allows one to use textconv commands with arguments. > >Signed-off-by: Matthieu Moy >diff --git a/git-gui/lib/blame.tcl b/git-gui/lib/blame.tcl >index 2137ec9..77656d3 100644 >--- a/git-gui/lib/blame.tcl >+++ b/git-gui/lib/blame.tcl >@@ -460,7 +460,9 @@ method _load {jump} { > } > if {$commit eq {}} { > if {$do_textconv ne 0} { >- set fd [open |[list $textconv $path] r] >+ # Run textconv with sh -c "..." to allow it to >+ # contain command + arguments. >+ set fd [open |[list [shellpath] -c "$textconv \"\$0\"" $path] r] > } else { > set fd [open $path r] > } I don't believe we need to put all this in to launch this via the shell. We just have to pass a list where the first element is the command-name. The following works for me using your 'textconv = odf2txt --width=40' test and also a 'textconv = od -t x1' that I tried for a hex dump output. I couldn't make run-mailcap do anything useful for me. diff --git a/lib/blame.tcl b/lib/blame.tcl index 2137ec9..c06ef04 100644 --- a/lib/blame.tcl +++ b/lib/blame.tcl @@ -460,7 +460,7 @@ method _load {jump} { } if {$commit eq {}} { if {$do_textconv ne 0} { - set fd [open |[list $textconv $path] r] + set fd [open |[linsert $textconv end $path] r] } else { set fd [open $path r] } -- Pat Thoyts http://www.patthoyts.tk/ PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD ``` ## Matthieu Moy, 2010-08-06 08:51 Subject: Re: [PATCH] git-gui: Use shell to launch textconv filter in "blame" Message-ID: URL: https://gitlist.dev/e/vpqaap0cees.fsf%40bauges.imag.fr In-Reply-To: <87aap0sljs.fsf@fox.patthoyts.tk> ``` Pat Thoyts writes: >> if {$commit eq {}} { >> if {$do_textconv ne 0} { >>- set fd [open |[list $textconv $path] r] >>+ # Run textconv with sh -c "..." to allow it to >>+ # contain command + arguments. >>+ set fd [open |[list [shellpath] -c "$textconv \"\$0\"" $path] r] >> } else { >> set fd [open $path r] >> } > > I don't believe we need to put all this in to launch this via the > shell. We just have to pass a list where the first element is the > command-name. > > The following works for me using your 'textconv = odf2txt --width=40' > test and also a 'textconv = od -t x1' that I tried for a hex dump > output. I couldn't make run-mailcap do anything useful for me. > > diff --git a/lib/blame.tcl b/lib/blame.tcl > index 2137ec9..c06ef04 100644 > --- a/lib/blame.tcl > +++ b/lib/blame.tcl > @@ -460,7 +460,7 @@ method _load {jump} { > } > if {$commit eq {}} { > if {$do_textconv ne 0} { > - set fd [open |[list $textconv $path] r] > + set fd [open |[linsert $textconv end $path] r] > } else { > set fd [open $path r] > } I'm not very fluent in Tcl, but I don't think this runs the command through a shell (pstree agrees with me). That will work in most cases, so that may be acceptable, but if you want to have full compatibility with what "git blame" does (by using a shell) and allow e.g. textconv = LANG=C some-command or textconv = cd ../; do-whatever which are already managed by "git blame" and are OK with my version, it's not going to do it. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Pat Thoyts, 2010-08-06 17:56 Subject: Re: [PATCH] git-gui: Use shell to launch textconv filter in "blame" Message-ID: <87eieby69a.fsf@fox.patthoyts.tk> URL: https://gitlist.dev/e/87eieby69a.fsf%40fox.patthoyts.tk In-Reply-To: ``` Matthieu Moy writes: [snip] > >I'm not very fluent in Tcl, but I don't think this runs the command >through a shell (pstree agrees with me). That will work in most cases, >so that may be acceptable, but if you want to have full compatibility >with what "git blame" does (by using a shell) and allow e.g. > >textconv = LANG=C some-command > >or > >textconv = cd ../; do-whatever > >which are already managed by "git blame" and are OK with my version, >it's not going to do it. OK compatibility with 'git blame' is a valid reason to do it your way. Tcl's exec (or open| is equivalent) doesn't go via shell. I'll check this on windows and apply it. Thanks. -- Pat Thoyts http://www.patthoyts.tk/ PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD ``` ## Kirill Smelkov, 2010-08-19 08:05 Subject: Re: [BUG] git gui blame fails for multi-word textconv filter Message-ID: <20100819080506.GA31474@landau.phys.spbu.ru> URL: https://gitlist.dev/e/20100819080506.GA31474%40landau.phys.spbu.ru In-Reply-To: <87iq3osm3h.fsf@fox.patthoyts.tk> ``` On Thu, Aug 05, 2010 at 01:46:29AM +0200, Cl??ment Poulain wrote: > Le 04/08/2010 21:25, Kirill Smelkov a ??crit : > >Hello, > > > >I use > > > > [diff "astextplain"] > > textconv = run-mailcap --action=cat > > > >in my ~/.gitconfig, and this works for git `git blame` because of 41a457 > >in git.git (textconv: use shell to run helper), but fails with git gui: > > > > $ git gui blame 21980.2--ИМС-МР231.doc > > Error in startup script: couldn't execute "run-mailcap --action=cat": > > no such file or directory [...] > >If so, I'd better teach cat-file about worktree, instead of teaching > >git-gui about running textconv filter through shell. Just a wish... > > > This was discussed here: a1ace6b77167a2ad4b4995e8c4d09761@ensimag.fr , > and your suggestion was considered ;-) I see, thanks. Sigh that it is done not that way, but anyway - everybody, thanks for fixing this. Kirill ```