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

[Carlo Arenas <carenas@gmail.com>]

On Sun, Nov 28, 2021 at 11:44 AM Junio C Hamano <gitster@pobox.com> wrote:
> 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.."

> 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