On Fri, May 28 2021, Felipe Contreras wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> Optimize the time git-send-email takes to do even the simplest of
>> things (such as serving up "-h") from around ~150ms to ~80ms-~90ms by
>> lazily loading the modules it requires.
>>
>> Before this change Devel::TraceUse would report 99/97 used modules
>> under NO_GETTEXT=[|Y], respectively. Now it's 52/37. It now takes ~15s
>> to run t9001-send-email.sh, down from ~20s.
>>
>> Changing File::Spec::Functions::{catdir,catfile} to invoking class
>> methods on File::Spec itself is idiomatic. See [1] for a more
>> elaborate explanation, the resulting code behaves the same way, just
>> without the now-pointless function wrapper.
>
> I would reference `man File::Spec` rather than an email.
>
> And while this change makes sense, I think it should be split in two.
>
> Instead of doing:
>
> -use Term::ANSIColor;
> -print color("reset"), "\n";
> +require Term::ANSIColor;
> +print Term::ANSIColor::color("reset"), "\n";
>
> We could do this in one patch:
>
> -print color("reset"), "\n";
> +print Term::ANSIColor::color("reset"), "\n";
>
> That is just no-op noise that we can mostly ignore in the review.
>
> Then the actual change to require Term::ANSIColor selectively would be
> much simpler to see.
You mean do the change from imported functions in one commit, and then
sprinkle the "require" in another one?
I think it's clearer this way, you can't really assert that it worked as
intended (i.e. you have no more imports) without the s/use/require/g, so
it makes sense to do both as one atomic change.
