git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"

From
David Aguilar <davvid@gmail.com>
Date
Jan 25, 2013, 05:29 UTC
Message-ID
<CAJDDKr4ZpQr029FW0v8LzwvhXZYmvAONbbZNuOq_E=Q1UzufvA@mail.gmail.com>
In-Reply-To
<b791e866c02b0c118f08bde1d7ca6c41d6239989.1359057056.git.john@keeping.me.uk>
On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:
Show 7 quoted lines
> The "--tool-help" option to git-difftool currently displays incorrect
> output since it uses the names of the files in
> "$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
> git-mergetool--lib.
>
> Fix this by simply delegating the "--tool-help" argument to the
> show_tool_help function in git-mergetool--lib.
Very nice.

One thought I had was that the unified show_tool_help should probably check TOOL_MODE=diff and skip over the !can_diff entries.

The current output of "git difftool --tool-help" before your patches has the problem that it will list tools such as "tortoisemerge" as "valid but not available" because it does not differentiate between missing and !can_diff.

Show 41 quoted lines
> diff --git a/git-difftool.perl b/git-difftool.perl
> index edd0493..0a90de4 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -59,57 +59,16 @@ sub find_worktree
>         return $worktree;
>  }
>
> -sub filter_tool_scripts
> -{
> -       my ($tools) = @_;
> -       if (-d $_) {
> -               if ($_ ne ".") {
> -                       # Ignore files in subdirectories
> -                       $File::Find::prune = 1;
> -               }
> -       } else {
> -               if ((-f $_) && ($_ ne "defaults")) {
> -                       push(@$tools, $_);
> -               }
> -       }
> -}
> -
>  sub print_tool_help
>  {
> -       my ($cmd, @found, @notfound, @tools);
> -       my $gitpath = Git::exec_path();
> -
> -       find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools");
> -
> -       foreach my $tool (@tools) {
> -               $cmd  = "TOOL_MODE=diff";
> -               $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
> -               $cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1";
> -               $cmd .= " && can_diff >/dev/null 2>&1";
> -               if (system('sh', '-c', $cmd) == 0) {
> -                       push(@found, $tool);
> -               } else {
> -                       push(@notfound, $tool);
> -               }
> -       }

This is where we conflated can_diff with "is the tool available?". The nice thing is that the old code did actually check can_diff, but since it conflated these things it ended up putting them in the @notfound list. It should ignore those completely, IMO.

That could be a nice follow-up patch.  What do you think?
-- 
David
Previous: John KeepingNext: John Keeping
Message 6 of 10 in “Fix "git difftool --tool-help"”
  1. 0/4 Fix "git difftool --tool-help"John Keeping, Jan 24, 2013
  2. 1/4 git-mergetool: move show_tool_help to mergetool--libJohn Keeping, Jan 24, 2013
  3. 2/4 git-mergetool: remove redundant assignmentJohn Keeping, Jan 24, 2013
  4. 3/4 git-mergetool: don't hardcode 'mergetool' in show_tool_helpJohn Keeping, Jan 24, 2013
  5. 4/4 git-difftool: use git-mergetool--lib for "--tool-help"John Keeping, Jan 24, 2013
  6. David AguilarJan 25, 2013
  7. John KeepingJan 25, 2013
  8. David AguilarJan 25, 2013
  9. John KeepingJan 25, 2013
  10. Junio C HamanoJan 24, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.