[BUG] git gui blame fails for multi-word textconv filter

[BUG] git gui blame fails for multi-word textconv filter

[Kirill Smelkov]

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=<num>] 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.

Re: [BUG] git gui blame fails for multi-word textconv filter

[Clément Poulain]

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=<num>] 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 ;-)

Re: [BUG] git gui blame fails for multi-word textconv filter

[Matthieu Moy]

Clément Poulain <clement.poulain@ensimag.imag.fr> 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/

[PATCH] git-gui: Use shell to launch textconv filter in "blame"

[Matthieu Moy]

This allows one to use textconv commands with arguments.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 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

Re: [PATCH] git-gui: Use shell to launch textconv filter in "blame"

[Pat Thoyts]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

>This allows one to use textconv commands with arguments.
>
>Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>

>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

Re: [PATCH] git-gui: Use shell to launch textconv filter in "blame"

[Matthieu Moy]

Pat Thoyts <patthoyts@users.sourceforge.net> 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/

Re: [PATCH] git-gui: Use shell to launch textconv filter in "blame"

[Pat Thoyts]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> 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

Re: [BUG] git gui blame fails for multi-word textconv filter

[Kirill Smelkov]

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

Re: [BUG] git gui blame fails for multi-word textconv filter

[Pat Thoyts]

Clément Poulain <clement.poulain@ensimag.imag.fr> 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