| Message ID | 20240606082114.GA1167215@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 53ce2e3f0abf356fb0d2638783fe06711c5337d6 |
| Headers | show |
| Series | dropping stdin support from test-terminal | expand |
Jeff King <peff@peff.net> writes: > I think even without the test-terminal cleanup, this is a good thing. > Any time there is a heuristic like isatty(), we should have a way for > the user to be more explicit about what they want(). I very often do "git am --no-3" to countermand a failed "git am -3" (or vice versa), so I'll be hit very hard with a need to retrain my fingers. But I'll live ;-) "--retry" is a horrible word, in that it makes it sound like it will keep trying to apply the same patch over and over until it applies cleanly or something. Can't we use "--continue" like everybody else (like "git rebase --continue", etc.), or would that be even more confusing?
Junio C Hamano <gitster@pobox.com> writes: > Jeff King <peff@peff.net> writes: > >> I think even without the test-terminal cleanup, this is a good thing. >> Any time there is a heuristic like isatty(), we should have a way for >> the user to be more explicit about what they want(). > > I very often do "git am --no-3" to countermand a failed "git am -3" > (or vice versa), so I'll be hit very hard with a need to retrain my > fingers. But I'll live ;-) Ah, no, this is not about not paying attention to isatty(0), but give us an additional way. I can see how it would help our tests; it would be nicer if the feature also has real world use. But I no longer mind the option existing. > "--retry" is a horrible word, in that it makes it sound like it will > keep trying to apply the same patch over and over until it applies > cleanly or something. Can't we use "--continue" like everybody else > (like "git rebase --continue", etc.), or would that be even more > confusing?
On Thu, Jun 06, 2024 at 09:48:52AM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Jeff King <peff@peff.net> writes: > > > >> I think even without the test-terminal cleanup, this is a good thing. > >> Any time there is a heuristic like isatty(), we should have a way for > >> the user to be more explicit about what they want(). > > > > I very often do "git am --no-3" to countermand a failed "git am -3" > > (or vice versa), so I'll be hit very hard with a need to retrain my > > fingers. But I'll live ;-) > > Ah, no, this is not about not paying attention to isatty(0), but > give us an additional way. I can see how it would help our tests; > it would be nicer if the feature also has real world use. Exactly, you can still do "git am -3" as before, and that's what I'd expect everyone to do. It is just about letting you be explicit if you want. I don't know if it could have real world use or not. In theory if you had a more complex program driving "git am", you'd need this. But in practice, I think the overlap between "people who write GUIs for Git" and "people who think mailing patches is a good idea" is pretty small. Let alone one with advanced features like "try this patch again with --3way". ;) But I do think as a general rule we should never provide any action _only_ through heuristics like "is stdin a tty". We should let the user be explicit, and use heuristics to guess the right thing when they don't feel like being so. > > "--retry" is a horrible word, in that it makes it sound like it will > > keep trying to apply the same patch over and over until it applies > > cleanly or something. Can't we use "--continue" like everybody else > > (like "git rebase --continue", etc.), or would that be even more > > confusing? There is already "am --continue", but it is not quite the same thing. It will try to commit the resolved tree state and keep going. Whereas with --retry we really are trying the same patch again. So it really is "over and over again", but only once per invocation. ;) I'm open to a better name if you can come up with one. -Peff
Hi Peff and Junio On 08/06/2024 12:29, Jeff King wrote: > On Thu, Jun 06, 2024 at 09:48:52AM -0700, Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: >>> Jeff King <peff@peff.net> writes: > But I do think as a general rule we should never provide any action > _only_ through heuristics like "is stdin a tty". We should let the user > be explicit, and use heuristics to guess the right thing when they don't > feel like being so. I agree that's a good general rule to have >>> "--retry" is a horrible word, in that it makes it sound like it will >>> keep trying to apply the same patch over and over until it applies >>> cleanly or something. "retry" means "try once more", so I think it is a reasonable name for this option. (Programs often retrying a failing operation in a loop but then they are retrying many times.) I haven't looked at these patches in detail so I cannot comment on the code changes but I do think the general aim is a good one. Best Wishes Phillip
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index 624a6e6fe4..69d5cc9f21 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -18,7 +18,7 @@ SYNOPSIS [--quoted-cr=<action>] [--empty=(stop|drop|keep)] [(<mbox> | <Maildir>)...] -'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=(diff|raw)] | --allow-empty) +'git am' (--continue | --skip | --abort | --quit | --retry | --show-current-patch[=(diff|raw)] | --allow-empty) DESCRIPTION ----------- @@ -208,6 +208,12 @@ Valid <action> for the `--whitespace` option are: Abort the patching operation but keep HEAD and the index untouched. +--retry:: + Try to apply the last conflicting patch again. This is generally + only useful for passing extra options to the retry attempt + (e.g., `--3way`), since otherwise you'll just see the same + failure again. + --show-current-patch[=(diff|raw)]:: Show the message at which `git am` has stopped due to conflicts. If `raw` is specified, show the raw contents of diff --git a/builtin/am.c b/builtin/am.c index 36839029d2..926592691a 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2393,6 +2393,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) N_("show the patch being applied"), PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, parse_opt_show_current_patch, RESUME_SHOW_PATCH_RAW }, + OPT_CMDMODE(0, "retry", &resume_mode, + N_("try to apply current patch again"), + RESUME_APPLY), OPT_CMDMODE(0, "allow-empty", &resume_mode, N_("record the empty patch as an empty commit"), RESUME_ALLOW_EMPTY), diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh index 4add7c7757..a32cec42aa 100755 --- a/t/t4153-am-resume-override-opts.sh +++ b/t/t4153-am-resume-override-opts.sh @@ -3,7 +3,6 @@ test_description='git-am command-line options override saved options' . ./test-lib.sh -. "$TEST_DIRECTORY"/lib-terminal.sh format_patch () { git format-patch --stdout -1 "$1" >"$1".eml @@ -27,7 +26,12 @@ test_expect_success 'setup' ' format_patch side2 ' -test_expect_success TTY '--3way overrides --no-3way' ' +test_expect_success '--retry fails without in-progress operation' ' + test_must_fail git am --retry 2>err && + test_grep "operation not in progress" err +' + +test_expect_success '--3way overrides --no-3way' ' rm -fr .git/rebase-apply && git reset --hard && git checkout renamed-file && @@ -40,7 +44,7 @@ test_expect_success TTY '--3way overrides --no-3way' ' # Applying side1 with am --3way will succeed due to the threeway-merge. # Applying side2 will fail as --3way does not apply to it. - test_must_fail test_terminal git am --3way </dev/zero && + test_must_fail git am --retry --3way && test_path_is_dir .git/rebase-apply && test side1 = "$(cat file2)" ' @@ -84,7 +88,7 @@ test_expect_success '--signoff overrides --no-signoff' ' test $(git cat-file commit HEAD | grep -c "Signed-off-by:") -eq 0 ' -test_expect_success TTY '--reject overrides --no-reject' ' +test_expect_success '--reject overrides --no-reject' ' rm -fr .git/rebase-apply && git reset --hard && git checkout first && @@ -94,7 +98,7 @@ test_expect_success TTY '--reject overrides --no-reject' ' test_path_is_dir .git/rebase-apply && test_path_is_missing file.rej && - test_must_fail test_terminal git am --reject </dev/zero && + test_must_fail git am --retry --reject </dev/zero && test_path_is_dir .git/rebase-apply && test_path_is_file file.rej '
After a patch fails, you can ask "git am" to try applying it again with new options by running without any of the resume options. E.g.: git am <patch # oops, it failed; let's try again git am --3way But since this second command has no explicit resume option (like "--continue"), it looks just like an invocation to read a fresh patch from stdin. To avoid confusing the two cases, there are some heuristics, courtesy of 8d18550318 (builtin-am: reject patches when there's a session in progress, 2015-08-04): if (in_progress) { /* * Catch user error to feed us patches when there is a session * in progress: * * 1. mbox path(s) are provided on the command-line. * 2. stdin is not a tty: the user is trying to feed us a patch * from standard input. This is somewhat unreliable -- stdin * could be /dev/null for example and the caller did not * intend to feed us a patch but wanted to continue * unattended. */ if (argc || (resume_mode == RESUME_FALSE && !isatty(0))) die(_("previous rebase directory %s still exists but mbox given."), state.dir); if (resume_mode == RESUME_FALSE) resume_mode = RESUME_APPLY; [...] So if no resume command is given, then we require that stdin be a tty, and otherwise complain about (potentially) receiving an mbox on stdin. But of course you might not actually have a terminal available! And sadly there is no explicit way to hit this same code path; this is the only place that sets RESUME_APPLY. So you're stuck, and scripts like our test suite have to bend over backwards to create a pseudo-tty. Let's provide an explicit option to trigger this mode. The code turns out to be quite simple; just setting "resume_mode" to RESUME_FALSE is enough to dodge the tty check, and then our state is the same as it would be with the heuristic case (which we'll continue to allow). When we don't have a session in progress, there's already code to complain when resume_mode is set (but we'll add a new test to cover that). To test the new option, we'll convert the existing tests that rely on the fake stdin tty. That lets us test them on more platforms, and will let us simplify test_terminal a bit in a future patch. It does, however, mean we're not testing the tty heuristic at all. Signed-off-by: Jeff King <peff@peff.net> --- I think even without the test-terminal cleanup, this is a good thing. Any time there is a heuristic like isatty(), we should have a way for the user to be more explicit about what they want(). Do we are about losing the testing of the tty heuristic? Documentation/git-am.txt | 8 +++++++- builtin/am.c | 3 +++ t/t4153-am-resume-override-opts.sh | 14 +++++++++----- 3 files changed, 19 insertions(+), 6 deletions(-)