{"thread":{"id":"56992","subject":"[PATCH] add -p: avoid use of undefined $key when ReadKey -> EOF","startedAt":"2021-11-28T17:56:30Z","lastAt":"2021-11-28T23:17:56Z","messageCount":4,"participants":["Carlo Marcelo Arenas Belón","Junio C Hamano","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"442418","messageId":"20211128174903.5623-1-carenas@gmail.com","threadId":"56992","inReplyTo":null,"subject":"[PATCH] add -p: avoid use of undefined $key when ReadKey -> EOF","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-11-28T17:49:03Z","receivedAt":"2021-11-28T17:56:30Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"b5cc003253 (add -i: ignore terminal escape sequences, 2011-05-17)\nadd an additional check to the original code to better handle keys\nfor escape sequences, but failed to account for the possibility\nthe first ReadKey call returned undef (ex: stdin closes) and that\nwas being handled fine by the original code in ca6ac7f135 (add -p:\nprompt for single characters, 2009-02-05)\n\nAdd a test for undefined and encapsulate the loop and the original\nprint that relied on it within it.\n\nAfter this, the following command (in a suitable repository state)\nwouldn't print any error:\n\n  $ git -c interactive.singleKey add -p </dev/null\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n git-add--interactive.perl | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex bc3a1e8eff..95887fd8e5 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -1175,15 +1175,17 @@ sub prompt_single_character {\n \t\tReadMode 'cbreak';\n \t\tmy $key = ReadKey 0;\n \t\tReadMode 'restore';\n-\t\tif ($use_termcap and $key eq \"\\e\") {\n-\t\t\twhile (!defined $term_escapes{$key}) {\n-\t\t\t\tmy $next = ReadKey 0.5;\n-\t\t\t\tlast if (!defined $next);\n-\t\t\t\t$key .= $next;\n+\t\tif (defined $key) {\n+\t\t\tif ($use_termcap and $key eq \"\\e\") {\n+\t\t\t\twhile (!defined $term_escapes{$key}) {\n+\t\t\t\t\tmy $next = ReadKey 0.5;\n+\t\t\t\t\tlast if (!defined $next);\n+\t\t\t\t\t$key .= $next;\n+\t\t\t\t}\n+\t\t\t\t$key =~ s/\\e/^[/;\n \t\t\t}\n-\t\t\t$key =~ s/\\e/^[/;\n+\t\t\tprint \"$key\";\n \t\t}\n-\t\tprint \"$key\" if defined $key;\n \t\tprint \"\\n\";\n \t\treturn $key;\n \t} else {\n-- \n2.34.0.352.g07dee3c5e1\n\n"},{"id":"442422","messageId":"xmqq8rx85ala.fsf@gitster.g","threadId":"56992","inReplyTo":"20211128174903.5623-1-carenas@gmail.com","subject":"Re: [PATCH] add -p: avoid use of undefined $key when ReadKey -> EOF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-28T19:44:33Z","receivedAt":"2021-11-28T19:46:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> b5cc003253 (add -i: ignore terminal escape sequences, 2011-05-17)\n> add an additional check to the original code to better handle keys\n> for escape sequences, but failed to account for the possibility\n> the first ReadKey call returned undef (ex: stdin closes) and that\n> was being handled fine by the original code in ca6ac7f135 (add -p:\n> prompt for single characters, 2009-02-05)\n>\n> Add a test for undefined and encapsulate the loop and the original\n> print that relied on it within it.\n\nI wondered where a change to t/ directory was, but we are not\ntalking about that kind of test ;-)  OK.\n\n> After this, the following command (in a suitable repository state)\n> wouldn't print any error:\n>\n>   $ git -c interactive.singleKey add -p </dev/null\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  git-add--interactive.perl | 16 +++++++++-------\n>  1 file changed, 9 insertions(+), 7 deletions(-)\n>\n> diff --git a/git-add--interactive.perl b/git-add--interactive.perl\n> index bc3a1e8eff..95887fd8e5 100755\n> --- a/git-add--interactive.perl\n> +++ b/git-add--interactive.perl\n> @@ -1175,15 +1175,17 @@ sub prompt_single_character {\n>  \t\tReadMode 'cbreak';\n>  \t\tmy $key = ReadKey 0;\n>  \t\tReadMode 'restore';\n> -\t\tif ($use_termcap and $key eq \"\\e\") {\n> -\t\t\twhile (!defined $term_escapes{$key}) {\n> -\t\t\t\tmy $next = ReadKey 0.5;\n> -\t\t\t\tlast if (!defined $next);\n> -\t\t\t\t$key .= $next;\n> +\t\tif (defined $key) {\n> +\t\t\tif ($use_termcap and $key eq \"\\e\") {\n> +\t\t\t\twhile (!defined $term_escapes{$key}) {\n> +\t\t\t\t\tmy $next = ReadKey 0.5;\n> +\t\t\t\t\tlast if (!defined $next);\n> +\t\t\t\t\t$key .= $next;\n> +\t\t\t\t}\n> +\t\t\t\t$key =~ s/\\e/^[/;\n>  \t\t\t}\n> -\t\t\t$key =~ s/\\e/^[/;\n> +\t\t\tprint \"$key\";\n>  \t\t}\n> -\t\tprint \"$key\" if defined $key;\n\nThis was from the very original and was a strong clue that $key\ncould be undef; well spotted.\n\n>  \t\tprint \"\\n\";\n>  \t\treturn $key;\n>  \t} else {\n\nEssentially, the code added by b5cc0032 (add -i: ignore terminal\nescape sequences, 2011-05-17) wanted to say\n\n    The original code\n\n    - called ReadKey and took its output in $key, \n    - echoed it if it was defined (otherwise $key wasn't echoed),\n    - emitted a line feed,\n    - and returned $key.\n\n    We deal with the case where a single keystroke of some keys is\n    delivered to us as an escape sequence of multiple \"key\"s from\n    ReadKey, and the way we do so is to replace that single ReadKey\n    with the new code that accumulates these multiple \"key\"s in\n    $key.\n\nI am undecided, but the minimum patch below seems to makes the above\nintention in the updated code clearer.\n\ndiff --git i/git-add--interactive.perl w/git-add--interactive.perl\nindex bc3a1e8eff..c459c675e7 100755\n--- i/git-add--interactive.perl\n+++ w/git-add--interactive.perl\n@@ -1175,7 +1175,7 @@ sub prompt_single_character {\n \t\tReadMode 'cbreak';\n \t\tmy $key = ReadKey 0;\n \t\tReadMode 'restore';\n-\t\tif ($use_termcap and $key eq \"\\e\") {\n+\t\tif (defined $key && $use_termcap and $key eq \"\\e\") {\n \t\t\twhile (!defined $term_escapes{$key}) {\n \t\t\t\tmy $next = ReadKey 0.5;\n \t\t\t\tlast if (!defined $next);\n"},{"id":"442423","messageId":"CAPUEspjo+cKLnE+MoABp_NnGDCCz8fL=pJj3ZNsKmCVetUn1jA@mail.gmail.com","threadId":"56992","inReplyTo":"xmqq8rx85ala.fsf@gitster.g","subject":"Re: [PATCH] add -p: avoid use of undefined $key when ReadKey -> EOF","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-28T20:52:39Z","receivedAt":"2021-11-28T20:54:59Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sun, Nov 28, 2021 at 11:44 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n> > Add a test for undefined and encapsulate the loop and the original\n> > print that relied on it within it.\n>\n> I wondered where a change to t/ directory was, but we are not\n> talking about that kind of test ;-)  OK.\n\nWill change the wording in v2 with something less ambiguous like\n\"Check for undefined..\"\n\n> I am undecided, but the minimum patch below seems to makes the above\n> intention in the updated code clearer.\n>\n> diff --git i/git-add--interactive.perl w/git-add--interactive.perl\n> index bc3a1e8eff..c459c675e7 100755\n> --- i/git-add--interactive.perl\n> +++ w/git-add--interactive.perl\n> @@ -1175,7 +1175,7 @@ sub prompt_single_character {\n>                 ReadMode 'cbreak';\n>                 my $key = ReadKey 0;\n>                 ReadMode 'restore';\n> -               if ($use_termcap and $key eq \"\\e\") {\n> +               if (defined $key && $use_termcap and $key eq \"\\e\") {\n>                         while (!defined $term_escapes{$key}) {\n>                                 my $next = ReadKey 0.5;\n>                                 last if (!defined $next);\n\nThe issue I had with that was that the order of the checks is slightly\nawkward and inefficient (ex: $use_termcap = false probably should\nshort circuit, and both checks for $key make more sense next to each\nother) and doing it outside avoids the extra !defined check for print.\n\nIt would also look cleaner IMHO if the expression would be rewritten\nto use '&&' instead of 'and', which smells like unnecessary\nrefactoring that I intentionally tried to avoid by creating a\nprecondition instead so `diff -w` is really small and straight\nforward.\n\nI have no strong opinions eitherway, so let me know what you would\nprefer and will get a v2 with it; thanks for reviewing.\n\nCarlo\n\nPS. list email is for whatever reason taking a long time to arrive\nfrom vger, hopefully not many are affected, but I might be slow to\nrespond\n"},{"id":"442424","messageId":"xmqq4k7v6fdn.fsf@gitster.g","threadId":"56992","inReplyTo":"CAPUEspjo+cKLnE+MoABp_NnGDCCz8fL=pJj3ZNsKmCVetUn1jA@mail.gmail.com","subject":"Re: [PATCH] add -p: avoid use of undefined $key when ReadKey -> EOF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-28T23:15:48Z","receivedAt":"2021-11-28T23:17:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas <carenas@gmail.com> writes:\n\n> I have no strong opinions eitherway, so let me know what you would\n> prefer and will get a v2 with it; thanks for reviewing.\n\nI have no strong preference either way, so I just queued what we\nreviewed as-is.\n\nThanks for the fix.\n"}]}