Re: [PATCH v4 10/13] send-email: lazily load modules for a big speedup

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 27, 2021, 11:36 UTC
Message-ID: <8735u8mmj9.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqv9759e0z.fsf@gitster.g>

On Thu, May 27 2021, Junio C Hamano wrote:

Show 31 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> -use File::Temp qw/ tempdir tempfile /;
>> -use File::Spec::Functions qw(catdir catfile);
>> ...
>> +		require File::Spec;
>> +		push @files, grep { -f $_ } map { File::Spec->catfile($f, $_) }
>>  				sort readdir $dh;
>> ...
>> -	push @files, $repo->command('format-patch', '-o', tempdir(CLEANUP => 1), @rev_list_opts);
>> +	require File::Temp;
>> +	push @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts);
>
> I suspect I would not be the only one who finds it curious to have
> the distinction between the use of "->" in File::Spec->catfile() and
> "::" in File::Temp::tempdir (and all the other function calls
> touched by this patch).
>
> Wouldn't it be more faithful to the original to do:
>
>     require File::Spec::Functions;
>     push @files, ... map { File::Spec::Functions::catfile($f, $_) }
>
> in the first hunk?  It would save the time readers of "git log -p"
> has to spend on wondering why "catfile" is an oddball.
>
> Of course "man File::Spec" would educate readers where the
> difference comes from, but it is not immediately obvious to me why
> we want to switch in a "lazily load for speedup" that is supposed to
> be optimization-only, even if the class method ends up calling the
> same underlying thing.

It was idiomatic to me so I didn't think of explaining it. Yes it looks odd, but it's the most straightforward and correct thing to do in this s/use/require/ change.

The reason to use the File::Spec::Functions wrapper is because you want File::Spec class methods in your namespace. I.e. to do catfile() instead of File::Spec->catfile().

To do that requires importing the "catfile" symbol. Since we're doing a s/use/requires/ here we won't do that (symbols like that are compile-time), so we'd need to call File::Spec::Functions::catfile() as you point out.

Except the whole point of that Functions.pm package is to not need that fully-qualified name, so once we're doing that we might as well call File::Spec->catfile() instead.

The implementation of File::Spec::Functions is to literally give you a generaler boilerplate function in your namespace that calls File::Spec::catfile("File::Spec", <your argument here>), which is what the "->" class method syntax does implicitly. Calling that wrapper doesn't make sense here.