Re: [PATCH 9/9 v8] difftool: print list of valid tools with '--tool-help'

From: Tim Henigan <tim.henigan@gmail.com>
Date: Mar 28, 2012, 19:48 UTC
Message-ID: <CAFouetjbq+A04HECXN39KeLBgkTd+HJyxeM0wSDhS2Xo=_gQVQ@mail.gmail.com>
In-Reply-To: <7vvclov9hi.fsf@alter.siamese.dyndns.org>

On Wed, Mar 28, 2012 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 5 quoted lines

> Tim Henigan <tim.henigan@gmail.com> writes:
>
> OK, but doesn't File::Find recurse into its subdirectories?  If you create
> a 'foo' directory there and drop a 'bar' script in it, is the rest of the
> code prepared to give you "git difftool -t foo/bar"?

It does recurse, but in this context '$_' only contains the current file name within the directory...not the directory itself [1]. So if we call 'find' on a directory that contains:

  foo
  bar/
      baz

then @tools = ('foo', 'baz')

[1]: http://perldoc.perl.org/File/Find.html#The-wanted-function

Show 6 quoted lines

>> +     for (@tools) {
>> +             my $tool = $_;
>> +             next if ($tool eq "defaults");
>
> Now you use File::Find::find(), you probably should do this kind of
> trivial filtering inside the callback, no?

I thought about that, but the filter is so simple that it seemed like overkill to add another function. If there is another revision of this patch, I will reconsider.

As always, thanks for the review :)