{"thread":{"id":"27381","subject":"[PATCH] add -i: ignore terminal escape sequences","startedAt":"2011-05-17T15:19:08Z","lastAt":"2011-05-18T07:33:16Z","messageCount":6,"participants":["trast@student.ethz.ch","Junio C Hamano","Sverre Rabbelier","Jeff King","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"168062","messageId":"05ce7ccdb3f4e07724d430f6ea2a8c9730971c9d.1305645331.git.trast@student.ethz.ch","threadId":"27381","inReplyTo":null,"subject":"[PATCH] add -i: ignore terminal escape sequences","fromName":"","fromEmail":"trast@student.ethz.ch","sentAt":"2011-05-17T15:19:08Z","receivedAt":"2011-05-17T15:19:08Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"From: Thomas Rast \n\nOn the author's terminal, the up-arrow input sequence is ^[[A, and\nthus fat-fingering an up-arrow into 'git checkout -p' is quite\ndangerous: git-add--interactive.perl will ignore the ^[ and [\ncharacters and happily treat A as \"discard everything\".\n\nAs a band-aid fix, use Term::Cap to get all terminal capabilities.\nThen use the heuristic that any capability value that starts with ^[\n(i.e., \\e in perl) must be a key input sequence. Finally, given an\ninput that starts with ^[, read more characters until we have read a\nfull escape sequence, then return that to the caller. We use a\ntimeout of 0.5 seconds on the subsequent reads to avoid getting stuck\nif the user actually input a lone ^[.\n\nSince none of the currently recognized keys start with ^[, the net\nresult is that the sequence as a whole will be ignored and the help\ndisplayed.\n\nSigned-off-by: Thomas Rast \n---\n\nI nearly managed to lose a bunch of uncommitted work today, but could\nsalvage most of it from the pieces of diffs in the terminal\nscrollback. Sigh.\n\nFuture work might include mapping such sequences to the keys they\nrepresent, so that (shift) up-arrow can be k (K) and (shift)\ndown-arrow j (J), or some such.\n\nOh yeah, PS: I'm alive ;-)\n\n git-add--interactive.perl | 19 +++++++++++++++++++\n 1 files changed, 19 insertions(+), 0 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 4f08fe7..8f0839d 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -45,6 +45,9 @@\n my $normal_color = $repo->get_color(\"\", \"reset\");\n \n my $use_readkey = 0;\n+my $use_termcap = 0;\n+my %term_escapes;\n+\n sub ReadMode;\n sub ReadKey;\n if ($repo->config_bool(\"interactive.singlekey\")) {\n@@ -53,6 +56,14 @@\n \t\tTerm::ReadKey->import;\n \t\t$use_readkey = 1;\n \t};\n+\teval {\n+\t\trequire Term::Cap;\n+\t\tmy $termcap = Term::Cap->Tgetent;\n+\t\tforeach (values %$termcap) {\n+\t\t\t$term_escapes{$_} = 1 if /^\\e/;\n+\t\t}\n+\t\t$use_termcap = 1;\n+\t};\n }\n \n sub colored {\n@@ -1067,6 +1078,14 @@ 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\t\t}\n+\t\t\t$key =~ s/\\e/^[/;\n+\t\t}\n \t\tprint \"$key\" if defined $key;\n \t\tprint \"\\n\";\n \t\treturn $key;\n-- \n1.7.5.1.520.g98107.dirty\n"},{"id":"168095","messageId":"7vipt8fyxj.fsf@alter.siamese.dyndns.org","threadId":"27381","inReplyTo":"05ce7ccdb3f4e07724d430f6ea2a8c9730971c9d.1305645331.git.trast@student.ethz.ch","subject":"Re: [PATCH] add -i: ignore terminal escape sequences","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-18T03:52:08Z","receivedAt":"2011-05-18T03:52:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" writes:\n\n> I nearly managed to lose a bunch of uncommitted work today, but could\n> salvage most of it from the pieces of diffs in the terminal\n> scrollback. Sigh.\n\nI think the take-home lesson is that confirmation offered in the default\nmode is valuable. The \"single-key\" mode is another long rope that I would\nnot use myself, but the users can choose to hang themselves with.\n\nJokes aside, it may make sense to offer an extra confirmation for \"a\" and\npossibly \"s\" in single-key mode. Unlike others, they are destructive when\nthe changes you are splitting from the working tree is large-ish.\n\n> Oh yeah, PS: I'm alive ;-)\n\nGood to hear.\n"},{"id":"168100","messageId":"BANLkTinSKFVP=VYw4PoLP=-AUbLKeGX+gQ@mail.gmail.com","threadId":"27381","inReplyTo":"7vipt8fyxj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] add -i: ignore terminal escape sequences","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-05-18T04:37:50Z","receivedAt":"2011-05-18T04:37:50Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Tue, May 17, 2011 at 20:52, Junio C Hamano wrote:\n> Jokes aside, it may make sense to offer an extra confirmation for \"a\" and\n> possibly \"s\" in single-key mode. Unlike others, they are destructive when\n> the changes you are splitting from the working tree is large-ish.\n\nI don't understand why/how 's' is destructive? Doesn't it just ask you\nagain for each of the split hunks?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"168101","messageId":"7vei3weh2w.fsf@alter.siamese.dyndns.org","threadId":"27381","inReplyTo":"BANLkTinSKFVP=VYw4PoLP=-AUbLKeGX+gQ@mail.gmail.com","subject":"Re: [PATCH] add -i: ignore terminal escape sequences","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-18T05:03:03Z","receivedAt":"2011-05-18T05:03:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Tue, May 17, 2011 at 20:52, Junio C Hamano wrote:\n>> Jokes aside, it may make sense to offer an extra confirmation for \"a\" and\n>> possibly \"s\" in single-key mode. Unlike others, they are destructive when\n>> the changes you are splitting from the working tree is large-ish.\n>\n> I don't understand why/how 's' is destructive? Doesn't it just ask you\n> again for each of the split hunks?\n\nI think \"add -p\" used to have an internal mechanism to merge the adjacent\nsplit hunks back to a single hunk, so if we wanted to we could have given\nusers a way to recover from a mistaken \"s\"plit, but I don't think we kept\nthat code, so there is no way to properly \"recover\" from such a mistake.\n\nYes, it may be just the matter of two easy keystrokes, either yy or nn, to\nrecover from such a mistake, and that is why I said \"possibly\". It is\nnevertheless destructive in the sense that you cannot recover without\nquitting the current session and restarting.\n\nOf course Thomas could have simply done \"reset\" and started from scratch,\nso in that sense nothing is destructive, but we are not talking about the\nkind of destructive operations you cannot possibly recover without typing\neverything again.\n"},{"id":"168105","messageId":"20110518055411.GA27482@sigill.intra.peff.net","threadId":"27381","inReplyTo":"7vei3weh2w.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] add -i: ignore terminal escape sequences","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-18T05:54:11Z","receivedAt":"2011-05-18T05:54:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 17, 2011 at 10:03:03PM -0700, Junio C Hamano wrote:\n\n> I think \"add -p\" used to have an internal mechanism to merge the adjacent\n> split hunks back to a single hunk, so if we wanted to we could have given\n> users a way to recover from a mistaken \"s\"plit, but I don't think we kept\n> that code, so there is no way to properly \"recover\" from such a mistake.\n> \n> Yes, it may be just the matter of two easy keystrokes, either yy or nn, to\n> recover from such a mistake, and that is why I said \"possibly\". It is\n> nevertheless destructive in the sense that you cannot recover without\n> quitting the current session and restarting.\n> \n> Of course Thomas could have simply done \"reset\" and started from scratch,\n> so in that sense nothing is destructive, but we are not talking about the\n> kind of destructive operations you cannot possibly recover without typing\n> everything again.\n\nI'm not that concerned about these type of destructive operations, which\nmight waste a few seconds or a minute of your time. But Thomas' original\nemail indicated he was using \"git checkout -p\", which _is_ destructive\nin a much bigger way, because a \"y\" overwrites worktree files which do\nnot otherwise have a backup (even \"reset -p\" leaves unreferenced blobs\nthat used to be in the index).\n\n-Peff\n"},{"id":"168111","messageId":"201105180933.16650.trast@student.ethz.ch","threadId":"27381","inReplyTo":"20110518055411.GA27482@sigill.intra.peff.net","subject":"Re: [PATCH] add -i: ignore terminal escape sequences","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-05-18T07:33:16Z","receivedAt":"2011-05-18T07:33:16Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King wrote:\n> On Tue, May 17, 2011 at 10:03:03PM -0700, Junio C Hamano wrote:\n> \n> > Of course Thomas could have simply done \"reset\" and started from scratch,\n> > so in that sense nothing is destructive, but we are not talking about the\n> > kind of destructive operations you cannot possibly recover without typing\n> > everything again.\n> \n> I'm not that concerned about these type of destructive operations, which\n> might waste a few seconds or a minute of your time. But Thomas' original\n> email indicated he was using \"git checkout -p\", which _is_ destructive\n> in a much bigger way, because a \"y\" overwrites worktree files which do\n> not otherwise have a backup (even \"reset -p\" leaves unreferenced blobs\n> that used to be in the index).\n\nIndeed. I wouldn't be complaining if I typed \"a\" or \"y\", but\n'checkout -p' trashed all my changes to a file because it treated the\nup-arrow as an \"a\". We should not accidentally \"bind\" things to extra\nkeys, much less dangerous ones. The fact that up-arrow is ^[[A is an\nimplementation detail.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}