Re: [PATCH v2] gitk: add -C <path> commandline parameter to change path

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 6, 2015, 09:48 UTC
Message-ID: <CAPig+cT9K2H_jzVNOHS0vaarU+tfDfp_=Z4c1n6o5EX9wE6JXA@mail.gmail.com>
In-Reply-To: <1446715164-19165-1-git-send-email-juhapekka.heikkila@gmail.com>

On Thu, Nov 5, 2015 at 4:19 AM, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:

> This patch adds -C (change working directory) parameter to
> gitk. With this parameter, instead of need to cd to directory
> with .git folder, one can point the correct folder from
> commandline.

Thanks, -C makes more sense than --cwd, and is more consistent with other commands.

You'll want to also address the patch to Paul Mackerras (I've cc:'d him) since he's the gitk maintainer, and Junio pulls gitk from him.

> v2: Adjusted the parameter as per Eric's suggestion. I think
>     it now work in similar manner as in many GNU tools as well
>     as git itself.

This sort of explanation of of changes between versions is definitely welcome, but should be placed below the "---" line just under your sign-off so that it does not become part of the permanent commit message.

> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---

You'd place your commentary about changes between patch versions here. More below...

Show 36 quoted lines

>  Documentation/gitk.txt |  7 +++++++
>  gitk-git/gitk          | 26 +++++++++++++++++---------
>  2 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> @@ -12279,20 +12279,14 @@ setui $uicolor
>
>  setoptions
>
> -# check that we can find a .git directory somewhere...
> -if {[catch {set gitdir [exec git rev-parse --git-dir]}]} {
> -    show_error {} . [mc "Cannot find a git repository here."]
> -    exit 1
> -}
> -
>  set selecthead {}
>  set selectheadid {}
>
>  set revtreeargs {}
>  set cmdline_files {}
> -set i 0
>  set revtreeargscmd {}
> -foreach arg $argv {
> +for {set i 0} {$i < [llength $argv]} {incr i} {
> +       set arg [lindex $argv [expr {$i}]]
>      switch -glob -- $arg {
>         "" { }
>         "--" {
> @@ -12305,11 +12299,25 @@ foreach arg $argv {
>         "--argscmd=*" {
>             set revtreeargscmd [string range $arg 10 end]
>         }
> +       "-C*" {
> +               if {[string length $arg] < 3} {
> +                       incr i
> +                       cd [lindex $argv [expr {$i}]]

What happens if no arguments follow -C (that is, the entire command-line is "gitk -C")? Will this simply run "cd" with no argument or with an empty argument or error out or what? Should there be a check for this condition?

> +                       continue

Why does this 'continue' need to be here? Am I missing something obvious?

Show 20 quoted lines

> +               } else {
> +                       cd [string range $arg 2 end]
> +               }
> +       }
>         default {
>             lappend revtreeargs $arg
>         }
>      }
> -    incr i
> +}
> +
> +# check that we can find a .git directory somewhere...
> +if {[catch {set gitdir [exec git rev-parse --git-dir]}]} {
> +    show_error {} . [mc "Cannot find a git repository here."]
> +    exit 1
>  }
>
>  if {$selecthead eq "HEAD"} {
> --
> 1.9.1