{"thread":{"id":"61596","subject":"[PATCH 0/2] dropping stdin support from test-terminal","startedAt":"2024-06-06T08:17:25Z","lastAt":"2024-06-10T08:36:34Z","messageCount":9,"participants":["Jeff King","Rubén Justo","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"496459","messageId":"20240606081724.GA1166769@coredump.intra.peff.net","threadId":"61596","inReplyTo":null,"subject":"[PATCH 0/2] dropping stdin support from test-terminal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-06T08:17:24Z","receivedAt":"2024-06-06T08:17:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is an alternative to Rubén's patch here:\n\n  https://lore.kernel.org/git/d95180fc-8f8a-4e1d-987d-3aa0811be7de@gmail.com/\n\nand instead just rips out the stdin feature of test-terminal completely.\nThere are two tests in t4153 that rely on it (and for which it was\noriginally added), but I think the fact that we need to use\ntest_terminal there indicates missing functionality in git-am.\n\nSo patch 1 fixes that, and then patch 2 simplifies test-terminal.\n\n  [1/2]: am: add explicit \"--retry\" option\n  [2/2]: test-terminal: drop stdin handling\n\n Documentation/git-am.txt           |  8 +++++++-\n builtin/am.c                       |  3 +++\n t/t4153-am-resume-override-opts.sh | 14 +++++++++-----\n t/test-terminal.perl               | 29 +++--------------------------\n 4 files changed, 22 insertions(+), 32 deletions(-)\n\n-Peff\n"},{"id":"496460","messageId":"20240606082114.GA1167215@coredump.intra.peff.net","threadId":"61596","inReplyTo":"20240606081724.GA1166769@coredump.intra.peff.net","subject":"[PATCH 1/2] am: add explicit \"--retry\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-06T08:21:14Z","receivedAt":"2024-06-06T08:21:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After a patch fails, you can ask \"git am\" to try applying it again with\nnew options by running without any of the resume options. E.g.:\n\n  git am <patch\n  # oops, it failed; let's try again\n  git am --3way\n\nBut since this second command has no explicit resume option (like\n\"--continue\"), it looks just like an invocation to read a fresh patch\nfrom stdin. To avoid confusing the two cases, there are some heuristics,\ncourtesy of 8d18550318 (builtin-am: reject patches when there's a\nsession in progress, 2015-08-04):\n\n\tif (in_progress) {\n\t\t/*\n\t\t * Catch user error to feed us patches when there is a session\n\t\t * in progress:\n\t\t *\n\t\t * 1. mbox path(s) are provided on the command-line.\n\t\t * 2. stdin is not a tty: the user is trying to feed us a patch\n\t\t *    from standard input. This is somewhat unreliable -- stdin\n\t\t *    could be /dev/null for example and the caller did not\n\t\t *    intend to feed us a patch but wanted to continue\n\t\t *    unattended.\n\t\t */\n\t\tif (argc || (resume_mode == RESUME_FALSE && !isatty(0)))\n\t\t\tdie(_(\"previous rebase directory %s still exists but mbox given.\"),\n\t\t\t\tstate.dir);\n\n\t\tif (resume_mode == RESUME_FALSE)\n\t\t\tresume_mode = RESUME_APPLY;\n\t\t[...]\n\nSo if no resume command is given, then we require that stdin be a tty,\nand otherwise complain about (potentially) receiving an mbox on stdin.\nBut of course you might not actually have a terminal available! And\nsadly there is no explicit way to hit this same code path; this is the\nonly place that sets RESUME_APPLY. So you're stuck, and scripts like our\ntest suite have to bend over backwards to create a pseudo-tty.\n\nLet's provide an explicit option to trigger this mode. The code turns\nout to be quite simple; just setting \"resume_mode\" to RESUME_FALSE is\nenough to dodge the tty check, and then our state is the same as it\nwould be with the heuristic case (which we'll continue to allow).\n\nWhen we don't have a session in progress, there's already code to\ncomplain when resume_mode is set (but we'll add a new test to cover\nthat).\n\nTo test the new option, we'll convert the existing tests that rely on\nthe fake stdin tty. That lets us test them on more platforms, and will\nlet us simplify test_terminal a bit in a future patch.\n\nIt does, however, mean we're not testing the tty heuristic at all.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think even without the test-terminal cleanup, this is a good thing.\nAny time there is a heuristic like isatty(), we should have a way for\nthe user to be more explicit about what they want().\n\nDo we are about losing the testing of the tty heuristic?\n\n Documentation/git-am.txt           |  8 +++++++-\n builtin/am.c                       |  3 +++\n t/t4153-am-resume-override-opts.sh | 14 +++++++++-----\n 3 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex 624a6e6fe4..69d5cc9f21 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -18,7 +18,7 @@ SYNOPSIS\n \t [--quoted-cr=<action>]\n \t [--empty=(stop|drop|keep)]\n \t [(<mbox> | <Maildir>)...]\n-'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=(diff|raw)] | --allow-empty)\n+'git am' (--continue | --skip | --abort | --quit | --retry | --show-current-patch[=(diff|raw)] | --allow-empty)\n \n DESCRIPTION\n -----------\n@@ -208,6 +208,12 @@ Valid <action> for the `--whitespace` option are:\n \tAbort the patching operation but keep HEAD and the index\n \tuntouched.\n \n+--retry::\n+\tTry to apply the last conflicting patch again. This is generally\n+\tonly useful for passing extra options to the retry attempt\n+\t(e.g., `--3way`), since otherwise you'll just see the same\n+\tfailure again.\n+\n --show-current-patch[=(diff|raw)]::\n \tShow the message at which `git am` has stopped due to\n \tconflicts.  If `raw` is specified, show the raw contents of\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 36839029d2..926592691a 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2393,6 +2393,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t  N_(\"show the patch being applied\"),\n \t\t  PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n \t\t  parse_opt_show_current_patch, RESUME_SHOW_PATCH_RAW },\n+\t\tOPT_CMDMODE(0, \"retry\", &resume_mode,\n+\t\t\tN_(\"try to apply current patch again\"),\n+\t\t\tRESUME_APPLY),\n \t\tOPT_CMDMODE(0, \"allow-empty\", &resume_mode,\n \t\t\tN_(\"record the empty patch as an empty commit\"),\n \t\t\tRESUME_ALLOW_EMPTY),\ndiff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh\nindex 4add7c7757..a32cec42aa 100755\n--- a/t/t4153-am-resume-override-opts.sh\n+++ b/t/t4153-am-resume-override-opts.sh\n@@ -3,7 +3,6 @@\n test_description='git-am command-line options override saved options'\n \n . ./test-lib.sh\n-. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n format_patch () {\n \tgit format-patch --stdout -1 \"$1\" >\"$1\".eml\n@@ -27,7 +26,12 @@ test_expect_success 'setup' '\n \tformat_patch side2\n '\n \n-test_expect_success TTY '--3way overrides --no-3way' '\n+test_expect_success '--retry fails without in-progress operation' '\n+\ttest_must_fail git am --retry 2>err &&\n+\ttest_grep \"operation not in progress\" err\n+'\n+\n+test_expect_success '--3way overrides --no-3way' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout renamed-file &&\n@@ -40,7 +44,7 @@ test_expect_success TTY '--3way overrides --no-3way' '\n \n \t# Applying side1 with am --3way will succeed due to the threeway-merge.\n \t# Applying side2 will fail as --3way does not apply to it.\n-\ttest_must_fail test_terminal git am --3way </dev/zero &&\n+\ttest_must_fail git am --retry --3way &&\n \ttest_path_is_dir .git/rebase-apply &&\n \ttest side1 = \"$(cat file2)\"\n '\n@@ -84,7 +88,7 @@ test_expect_success '--signoff overrides --no-signoff' '\n \ttest $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n '\n \n-test_expect_success TTY '--reject overrides --no-reject' '\n+test_expect_success '--reject overrides --no-reject' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout first &&\n@@ -94,7 +98,7 @@ test_expect_success TTY '--reject overrides --no-reject' '\n \ttest_path_is_dir .git/rebase-apply &&\n \ttest_path_is_missing file.rej &&\n \n-\ttest_must_fail test_terminal git am --reject </dev/zero &&\n+\ttest_must_fail git am --retry --reject </dev/zero &&\n \ttest_path_is_dir .git/rebase-apply &&\n \ttest_path_is_file file.rej\n '\n-- \n2.45.2.817.g6f0d0f2a6c\n\n"},{"id":"496461","messageId":"20240606082237.GB1167215@coredump.intra.peff.net","threadId":"61596","inReplyTo":"20240606081724.GA1166769@coredump.intra.peff.net","subject":"[PATCH 2/2] test-terminal: drop stdin handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-06T08:22:37Z","receivedAt":"2024-06-06T08:22:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 18d8c26930 (test_terminal: redirect child process' stdin to a pty,\n2015-08-04), we set up a pty and copy stdin to the child program. But\nthis ends up being racy; once we send all of the bytes and close the\ndescriptor, the child program will no longer see a terminal! isatty()\nwill return 0, and trying to read may return EIO, even if we didn't yet\nget all of the bytes.\n\nThis was mentioned even in the commit message of 18d8c26930, but we\nhacked around it by just sending an infinite input from /dev/zero (in\nthe intended case, we only cared about isatty(0), not reading actual\ninput).\n\nAnd it came up again recently in:\n\n  https://lore.kernel.org/git/d42a55b1-1ba9-4cfb-9c3d-98ea4d86da33@gmail.com/\n\nwhere we tried to actually send bytes, but they don't always all come\nthrough. So this interface is somewhat of an accident waiting to happen;\na caller might not even care about stdin being a tty, but will get bit\nby the flaky behavior.\n\nOne solution would probably be to avoid closing test_terminal's end of\nthe pty altogether. But then the other side would never see EOF on its\nstdin.  That may be OK for some cases, but it's another gotcha that\nmight cause races or deadlocks, depending on what the child expects to\nread.\n\nLet's instead just drop test_terminal's stdin feature completely. Since\nthe previous commit dropped the two cases from t4153 for which the\nfeature was originally added, there are no callers left that need it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/test-terminal.perl | 29 +++--------------------------\n 1 file changed, 3 insertions(+), 26 deletions(-)\n\ndiff --git a/t/test-terminal.perl b/t/test-terminal.perl\nindex 3810e9bb43..b8fd6a4f13 100755\n--- a/t/test-terminal.perl\n+++ b/t/test-terminal.perl\n@@ -5,17 +5,15 @@\n use IO::Pty;\n use File::Copy;\n \n-# Run @$argv in the background with stdio redirected to $in, $out and $err.\n+# Run @$argv in the background with stdio redirected to $out and $err.\n sub start_child {\n-\tmy ($argv, $in, $out, $err) = @_;\n+\tmy ($argv, $out, $err) = @_;\n \tmy $pid = fork;\n \tif (not defined $pid) {\n \t\tdie \"fork failed: $!\"\n \t} elsif ($pid == 0) {\n-\t\topen STDIN, \"<&\", $in;\n \t\topen STDOUT, \">&\", $out;\n \t\topen STDERR, \">&\", $err;\n-\t\tclose $in;\n \t\tclose $out;\n \t\texec(@$argv) or die \"cannot exec '$argv->[0]': $!\"\n \t}\n@@ -51,17 +49,6 @@ sub xsendfile {\n \tcopy($in, $out, 4096) or $!{EIO} or die \"cannot copy from child: $!\";\n }\n \n-sub copy_stdin {\n-\tmy ($in) = @_;\n-\tmy $pid = fork;\n-\tif (!$pid) {\n-\t\txsendfile($in, \\*STDIN);\n-\t\texit 0;\n-\t}\n-\tclose($in);\n-\treturn $pid;\n-}\n-\n sub copy_stdio {\n \tmy ($out, $err) = @_;\n \tmy $pid = fork;\n@@ -81,25 +68,15 @@ sub copy_stdio {\n \tdie \"usage: test-terminal program args\";\n }\n $ENV{TERM} = 'vt100';\n-my $parent_in = new IO::Pty;\n my $parent_out = new IO::Pty;\n my $parent_err = new IO::Pty;\n-$parent_in->set_raw();\n $parent_out->set_raw();\n $parent_err->set_raw();\n-$parent_in->slave->set_raw();\n $parent_out->slave->set_raw();\n $parent_err->slave->set_raw();\n-my $pid = start_child(\\@ARGV, $parent_in->slave, $parent_out->slave, $parent_err->slave);\n-close $parent_in->slave;\n+my $pid = start_child(\\@ARGV, $parent_out->slave, $parent_err->slave);\n close $parent_out->slave;\n close $parent_err->slave;\n-my $in_pid = copy_stdin($parent_in);\n copy_stdio($parent_out, $parent_err);\n my $ret = finish_child($pid);\n-# If the child process terminates before our copy_stdin() process is able to\n-# write all of its data to $parent_in, the copy_stdin() process could stall.\n-# Send SIGTERM to it to ensure it terminates.\n-kill 'TERM', $in_pid;\n-finish_child($in_pid);\n exit($ret);\n-- \n2.45.2.817.g6f0d0f2a6c\n"},{"id":"496538","messageId":"b1ac45ae-05d6-40ad-a9a1-3a83b5f662de@gmail.com","threadId":"61596","inReplyTo":"20240606082237.GB1167215@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-terminal: drop stdin handling","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-06T16:14:06Z","receivedAt":"2024-06-06T16:14:09Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 6/6/24 10:22 AM, Jeff King wrote:\n> Since 18d8c26930 (test_terminal: redirect child process' stdin to a pty,\n> 2015-08-04), we set up a pty and copy stdin to the child program. But\n> this ends up being racy; once we send all of the bytes and close the\n> descriptor, the child program will no longer see a terminal! isatty()\n> will return 0, and trying to read may return EIO, even if we didn't yet\n> get all of the bytes.\n> \n> This was mentioned even in the commit message of 18d8c26930, but we\n> hacked around it by just sending an infinite input from /dev/zero (in\n> the intended case, we only cared about isatty(0), not reading actual\n> input).\n> \n> And it came up again recently in:\n> \n>   https://lore.kernel.org/git/d42a55b1-1ba9-4cfb-9c3d-98ea4d86da33@gmail.com/\n> \n> where we tried to actually send bytes, but they don't always all come\n> through. So this interface is somewhat of an accident waiting to happen;\n> a caller might not even care about stdin being a tty, but will get bit\n> by the flaky behavior.\n> \n> One solution would probably be to avoid closing test_terminal's end of\n> the pty altogether. But then the other side would never see EOF on its\n> stdin.  That may be OK for some cases, but it's another gotcha that\n> might cause races or deadlocks, depending on what the child expects to\n> read.\n> \n> Let's instead just drop test_terminal's stdin feature completely. Since\n> the previous commit dropped the two cases from t4153 for which the\n> feature was originally added, there are no callers left that need it.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n\nAcked-by: Rubén Justo <rjusto@gmail.com>\n"},{"id":"496539","messageId":"c2045a59-fa36-4c42-b270-0b042ff6248d@gmail.com","threadId":"61596","inReplyTo":"20240606081724.GA1166769@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] dropping stdin support from test-terminal","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-06T16:17:24Z","receivedAt":"2024-06-06T16:17:28Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Thu, Jun 06, 2024 at 04:17:24AM -0400, Jeff King wrote:\n> This is an alternative to Rubén's patch here:\n> \n>   https://lore.kernel.org/git/d95180fc-8f8a-4e1d-987d-3aa0811be7de@gmail.com/\n> \n> and instead just rips out the stdin feature of test-terminal completely.\n\nI agree that this is a better approach.\n\nI have no problem building my other series on this.\n\nThanks!\n\n> There are two tests in t4153 that rely on it (and for which it was\n> originally added), but I think the fact that we need to use\n> test_terminal there indicates missing functionality in git-am.\n> \n> So patch 1 fixes that, and then patch 2 simplifies test-terminal.\n> \n>   [1/2]: am: add explicit \"--retry\" option\n>   [2/2]: test-terminal: drop stdin handling\n> \n>  Documentation/git-am.txt           |  8 +++++++-\n>  builtin/am.c                       |  3 +++\n>  t/t4153-am-resume-override-opts.sh | 14 +++++++++-----\n>  t/test-terminal.perl               | 29 +++--------------------------\n>  4 files changed, 22 insertions(+), 32 deletions(-)\n> \n> -Peff\n\nIt has been far less than ten, laps ;-)\n"},{"id":"496544","messageId":"xmqqzfrym3fu.fsf@gitster.g","threadId":"61596","inReplyTo":"20240606082114.GA1167215@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] am: add explicit \"--retry\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T16:38:45Z","receivedAt":"2024-06-06T16:38:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think even without the test-terminal cleanup, this is a good thing.\n> Any time there is a heuristic like isatty(), we should have a way for\n> the user to be more explicit about what they want().\n\nI very often do \"git am --no-3\" to countermand a failed \"git am -3\"\n(or vice versa), so I'll be hit very hard with a need to retrain my\nfingers.  But I'll live ;-)\n\n\"--retry\" is a horrible word, in that it makes it sound like it will\nkeep trying to apply the same patch over and over until it applies\ncleanly or something.  Can't we use \"--continue\" like everybody else\n(like \"git rebase --continue\", etc.), or would that be even more\nconfusing?\n"},{"id":"496545","messageId":"xmqqtti6m2yz.fsf@gitster.g","threadId":"61596","inReplyTo":"xmqqzfrym3fu.fsf@gitster.g","subject":"Re: [PATCH 1/2] am: add explicit \"--retry\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T16:48:52Z","receivedAt":"2024-06-06T16:48:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> I think even without the test-terminal cleanup, this is a good thing.\n>> Any time there is a heuristic like isatty(), we should have a way for\n>> the user to be more explicit about what they want().\n>\n> I very often do \"git am --no-3\" to countermand a failed \"git am -3\"\n> (or vice versa), so I'll be hit very hard with a need to retrain my\n> fingers.  But I'll live ;-)\n\nAh, no, this is not about not paying attention to isatty(0), but\ngive us an additional way.  I can see how it would help our tests;\nit would be nicer if the feature also has real world use.\n\nBut I no longer mind the option existing.\n\n> \"--retry\" is a horrible word, in that it makes it sound like it will\n> keep trying to apply the same patch over and over until it applies\n> cleanly or something.  Can't we use \"--continue\" like everybody else\n> (like \"git rebase --continue\", etc.), or would that be even more\n> confusing?\n"},{"id":"496710","messageId":"20240608112920.GC2966571@coredump.intra.peff.net","threadId":"61596","inReplyTo":"xmqqtti6m2yz.fsf@gitster.g","subject":"Re: [PATCH 1/2] am: add explicit \"--retry\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-08T11:29:20Z","receivedAt":"2024-06-08T11:29:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 06, 2024 at 09:48:52AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Jeff King <peff@peff.net> writes:\n> >\n> >> I think even without the test-terminal cleanup, this is a good thing.\n> >> Any time there is a heuristic like isatty(), we should have a way for\n> >> the user to be more explicit about what they want().\n> >\n> > I very often do \"git am --no-3\" to countermand a failed \"git am -3\"\n> > (or vice versa), so I'll be hit very hard with a need to retrain my\n> > fingers.  But I'll live ;-)\n> \n> Ah, no, this is not about not paying attention to isatty(0), but\n> give us an additional way.  I can see how it would help our tests;\n> it would be nicer if the feature also has real world use.\n\nExactly, you can still do \"git am -3\" as before, and that's what I'd\nexpect everyone to do. It is just about letting you be explicit if you\nwant.\n\nI don't know if it could have real world use or not. In theory if you\nhad a more complex program driving \"git am\", you'd need this. But in\npractice, I think the overlap between \"people who write GUIs for Git\"\nand \"people who think mailing patches is a good idea\" is pretty small.\nLet alone one with advanced features like \"try this patch again with\n--3way\". ;)\n\nBut I do think as a general rule we should never provide any action\n_only_ through heuristics like \"is stdin a tty\". We should let the user\nbe explicit, and use heuristics to guess the right thing when they don't\nfeel like being so.\n\n> > \"--retry\" is a horrible word, in that it makes it sound like it will\n> > keep trying to apply the same patch over and over until it applies\n> > cleanly or something.  Can't we use \"--continue\" like everybody else\n> > (like \"git rebase --continue\", etc.), or would that be even more\n> > confusing?\n\nThere is already \"am --continue\", but it is not quite the same thing. It\nwill try to commit the resolved tree state and keep going. Whereas with\n--retry we really are trying the same patch again. So it really is \"over\nand over again\", but only once per invocation. ;)\n\nI'm open to a better name if you can come up with one.\n\n-Peff\n"},{"id":"496761","messageId":"16a6b206-1733-4d64-89e6-c3e2368903ac@gmail.com","threadId":"61596","inReplyTo":"20240608112920.GC2966571@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] am: add explicit \"--retry\" option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-10T08:36:28Z","receivedAt":"2024-06-10T08:36:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff and Junio\n\nOn 08/06/2024 12:29, Jeff King wrote:\n> On Thu, Jun 06, 2024 at 09:48:52AM -0700, Junio C Hamano wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>> Jeff King <peff@peff.net> writes:\n> But I do think as a general rule we should never provide any action\n> _only_ through heuristics like \"is stdin a tty\". We should let the user\n> be explicit, and use heuristics to guess the right thing when they don't\n> feel like being so.\n\nI agree that's a good general rule to have\n\n>>> \"--retry\" is a horrible word, in that it makes it sound like it will\n>>> keep trying to apply the same patch over and over until it applies\n>>> cleanly or something.\n\n\"retry\" means \"try once more\", so I think it is a reasonable name for \nthis option. (Programs often retrying a failing operation in a loop but \nthen they are retrying many times.)\n\nI haven't looked at these patches in detail so I cannot comment on the \ncode changes but I do think the general aim is a good one.\n\nBest Wishes\n\nPhillip\n"}]}