Re: [PATCH v3] Add to gitk an --argscmd flag to get the list of refs to draw at refresh time.

From: Paul Mackerras <paulus@samba.org>
Date: Nov 17, 2007, 11:44 UTC
Message-ID: <18238.54272.198637.788805@cargo.ozlabs.ibm.com>
In-Reply-To: <20071102222436.23191.91785.stgit@gandelf.nowhere.earth>

Yann Dirson writes:

Show 9 quoted lines

> This allows to display a set of refs, when the refs in the set may
> themselves change between two refresh operations (eg. the set of
> patches in a patch stack), and is expected to be called from other
> porcelain suites.
> 
> The command is expected to generate a list of commits, which will be
> appended to the commits litterally passed on the command-line.  That
> command is handled similarly to the litteral refs, and has its own
> field in the saved view, and an edit field in the view editor.

The idea is fine, but I have some comments on the implementation:

> +--argscmd=<command>::
> +	Command to be run each time gitk has to determine the list of
> +	<revs> to show.  The command is expected to print on its
> +	standard output a list of additional revs to be shown.  Use

, one per line

> +	this instead of explicitely specifying <revs> if the set of

explicitly

> +	commits to show may vary between refreshs.

refreshes

Show 9 quoted lines

> +    set args $viewargs($view)
> +    if {$viewargscmd($view) ne "None"} {
> +	if {[catch {
> +	    set fd [open [concat | $viewargscmd($view)] r]
> +	} err]} {
> +	    puts stderr "Error executing --argscmd command: $err"
> +	    exit 1
> +	}
> +	set args [concat $args [read $fd 500000]]

I don't think you necessarily want to limit the number of characters read to 500000, do you?

What this will do is interpret the output of the program according to Tcl list syntax. I think it would be better to use [split $str "\n"] to split the program's output at the newlines. Also, you could combine the open, read and close into a single exec call. Thirdly, use error_popup rather than just writing to stderr.

I wonder if you should be invoking a shell to interpret the command. As you have it at the moment, the string that the user puts after --argscmd= is treated as a Tcl list, whereas the string they put into the entry field when creating a new view is split with shellsplit, which implements shell-style quoting. I think we should at least be consistent here, and possibly just leave it as a string and pass it to sh -c.

> +set revtreeargscmd None

Why the string "None" rather than the empty string? Is this a python-ism that has crept in?

Show 6 quoted lines

>  foreach arg $argv {
> -    switch -- $arg {
> -	"" { }
> -	"-d" { set datemode 1 }
> -	"--merge" {
> +    switch -regexp -- $arg {

Hmmm, it'd be simpler to use switch -glob here, I think.

Show 13 quoted lines

> +	"^$" { }
> +	"^-d$" { set datemode 1 }
> +	"^--merge$" {
>  	    set mergeonly 1
>  	    lappend revtreeargs $arg
>  	}
> -	"--" {
> +	"^--$" {
>  	    set cmdline_files [lrange $argv [expr {$i + 1}] end]
>  	    break
>  	}
> +	"^--argscmd=" {
> +	    regexp {^--argscmd=(.*)} $arg match revtreeargscmd

set revtreeargscmd [string range $arg 10 end]

Paul.