Re: [PATCH] add -p: avoid use of undefined $key when ReadKey -> EOF

From: Carlo Arenas <carenas@gmail.com>
Date: Nov 28, 2021, 20:52 UTC
Message-ID: <CAPUEspjo+cKLnE+MoABp_NnGDCCz8fL=pJj3ZNsKmCVetUn1jA@mail.gmail.com>
In-Reply-To: <xmqq8rx85ala.fsf@gitster.g>

On Sun, Nov 28, 2021 at 11:44 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> > Add a test for undefined and encapsulate the loop and the original
> > print that relied on it within it.
>
> I wondered where a change to t/ directory was, but we are not
> talking about that kind of test ;-)  OK.

Will change the wording in v2 with something less ambiguous like "Check for undefined.."

Show 16 quoted lines

> I am undecided, but the minimum patch below seems to makes the above
> intention in the updated code clearer.
>
> diff --git i/git-add--interactive.perl w/git-add--interactive.perl
> index bc3a1e8eff..c459c675e7 100755
> --- i/git-add--interactive.perl
> +++ w/git-add--interactive.perl
> @@ -1175,7 +1175,7 @@ sub prompt_single_character {
>                 ReadMode 'cbreak';
>                 my $key = ReadKey 0;
>                 ReadMode 'restore';
> -               if ($use_termcap and $key eq "\e") {
> +               if (defined $key && $use_termcap and $key eq "\e") {
>                         while (!defined $term_escapes{$key}) {
>                                 my $next = ReadKey 0.5;
>                                 last if (!defined $next);

The issue I had with that was that the order of the checks is slightly awkward and inefficient (ex: $use_termcap = false probably should short circuit, and both checks for $key make more sense next to each other) and doing it outside avoids the extra !defined check for print.

It would also look cleaner IMHO if the expression would be rewritten to use '&&' instead of 'and', which smells like unnecessary refactoring that I intentionally tried to avoid by creating a precondition instead so `diff -w` is really small and straight forward.

I have no strong opinions eitherway, so let me know what you would prefer and will get a v2 with it; thanks for reviewing.

Carlo

PS. list email is for whatever reason taking a long time to arrive from vger, hopefully not many are affected, but I might be slow to respond