From 1db65f324ecf246f7f5f8e0158daab0420dd18bc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 20 May 2019 08:07:15 -0400 Subject: [PATCH 1/4] am: simplify prompt response handling We'll never see a NULL returned from git_prompt(); if it can't produce any input for us (e.g., because the terminal got EOF) then it will just die(). So there's no need for us to handle NULL here. And even if there was, it doesn't make sense to continue; on a true terminal hangup we'd just loop infinitely trying to get input that will never come. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/am.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 58a2aef28b..56b1738777 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1658,9 +1658,7 @@ static int do_interactive(struct am_state *state) */ reply = git_prompt(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: "), PROMPT_ECHO); - if (!reply) { - continue; - } else if (*reply == 'y' || *reply == 'Y') { + if (*reply == 'y' || *reply == 'Y') { return 0; } else if (*reply == 'a' || *reply == 'A') { state->interactive = 0; From 97387c8bdd92596d79a84ce8a479c6b3c16d0cb7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 20 May 2019 08:09:26 -0400 Subject: [PATCH 2/4] am: read interactive input from stdin In the conversion of git-am from shell script to C, we switched to using git_prompt(). Unlike the original shell command "read reply", this doesn't read from stdin at all, but rather from /dev/tty. In most cases this distinction wouldn't matter. We require (as the shell script did) that stdin is a tty, so they would generally be the same thing. But one important exception is our test suite: even with test_terminal, we cannot test "am --interactive" because it insists on reading from /dev/tty, not the pseudo-tty we've set up in the test script. Fixing this clears the way to adding tests in a future patch. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/am.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 56b1738777..9d8437cc26 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1644,7 +1644,7 @@ static int do_interactive(struct am_state *state) die(_("cannot be interactive without stdin connected to a terminal.")); for (;;) { - const char *reply; + char reply[64]; puts(_("Commit Body is:")); puts("--------------------------"); @@ -1656,7 +1656,9 @@ static int do_interactive(struct am_state *state) * in your translation. The program will only accept English * input at this point. */ - reply = git_prompt(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: "), PROMPT_ECHO); + printf(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: ")); + if (!fgets(reply, sizeof(reply), stdin)) + die("unable to read from stdin; aborting"); if (*reply == 'y' || *reply == 'Y') { return 0; From 6e7baf246a2d12480d4dbc04c47efb66408c1927 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 20 May 2019 08:11:13 -0400 Subject: [PATCH 3/4] am: drop tty requirement for --interactive We have required that the stdin of "am --interactive" be a tty since a1451104ac (git-am: interactive should fail gracefully., 2005-10-12). However, this isn't strictly necessary, and makes the tool harder to test (and is unlike all of our other --interactive commands). The goal of that commit was to make sure that somebody does not do: git am --interactive Signed-off-by: Junio C Hamano --- builtin/am.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 9d8437cc26..9cbbd8eda8 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1640,9 +1640,6 @@ static int do_interactive(struct am_state *state) { assert(state->msg); - if (!isatty(0)) - die(_("cannot be interactive without stdin connected to a terminal.")); - for (;;) { char reply[64]; @@ -2327,6 +2324,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) argv_array_push(&paths, mkpath("%s/%s", prefix, argv[i])); } + if (state.interactive && !paths.argc) + die(_("interactive mode requires patches on the command line")); + am_setup(&state, patch_format, paths.argv, keep_cr); argv_array_clear(&paths); From 7663e438c5ca5a1dbcfcb1a9cbf68480362998a2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 May 2019 02:46:27 -0400 Subject: [PATCH 4/4] am: fix --interactive HEAD tree resolution In --interactive mode, "git am --resolved" will try to generate a patch based on what is in the index, so that it can prompt "apply this patch?". To do so it needs the tree of HEAD, which it tries to get with get_oid_tree(). However, this doesn't yield a tree object; the "tree" part just means "if you must disambiguate short oids, then prefer trees" (and we do not need to disambiguate at all, since we are feeding a ref). Instead, we must parse the oid as a commit (which should always be true in a non-corrupt repository), and access its tree pointer manually. This has been broken since the conversion to C in 7ff2683253 (builtin-am: implement -i/--interactive, 2015-08-04), but there was no test coverage because of interactive-mode's insistence on having a tty. That was lifted in the previous commit, so we can now add a test for this case. Note that before this patch, the test would result in a BUG() which comes from 3506dc9445 (has_uncommitted_changes(): fall back to empty tree, 2018-07-11). But before that, we'd have simply segfaulted (and in fact this is the exact type of case the BUG() added there was trying to catch!). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/am.c | 7 +++--- t/t4257-am-interactive.sh | 52 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) create mode 100755 t/t4257-am-interactive.sh diff --git a/builtin/am.c b/builtin/am.c index 9cbbd8eda8..d9199015f5 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1337,9 +1337,10 @@ static void write_index_patch(const struct am_state *state) struct rev_info rev_info; FILE *fp; - if (!get_oid_tree("HEAD", &head)) - tree = lookup_tree(the_repository, &head); - else + if (!get_oid("HEAD", &head)) { + struct commit *commit = lookup_commit_or_die(&head, "HEAD"); + tree = get_commit_tree(commit); + } else tree = lookup_tree(the_repository, the_repository->hash_algo->empty_tree); diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh new file mode 100755 index 0000000000..5344bd248a --- /dev/null +++ b/t/t4257-am-interactive.sh @@ -0,0 +1,52 @@ +#!/bin/sh + +test_description='am --interactive tests' +. ./test-lib.sh + +test_expect_success 'set up patches to apply' ' + test_commit unrelated && + test_commit no-conflict && + test_commit conflict-patch file patch && + git format-patch --stdout -2 >mbox && + + git reset --hard unrelated && + test_commit conflict-master file master base +' + +# Sanity check our setup. +test_expect_success 'applying all patches generates conflict' ' + test_must_fail git am mbox && + echo resolved >file && + git add -u && + git am --resolved +' + +test_expect_success 'interactive am can apply a single patch' ' + git reset --hard base && + # apply the first, but not the second + test_write_lines y n | git am -i mbox && + + echo no-conflict >expect && + git log -1 --format=%s >actual && + test_cmp expect actual +' + +test_expect_success 'interactive am can resolve conflict' ' + git reset --hard base && + # apply both; the second one will conflict + test_write_lines y y | test_must_fail git am -i mbox && + echo resolved >file && + git add -u && + # interactive "--resolved" will ask us if we want to apply the result + echo y | git am -i --resolved && + + echo conflict-patch >expect && + git log -1 --format=%s >actual && + test_cmp expect actual && + + echo resolved >expect && + git cat-file blob HEAD:file >actual && + test_cmp expect actual +' + +test_done