{"thread":{"id":"51129","subject":"Abort (core dumped)","startedAt":"2019-05-20T08:36:21Z","lastAt":"2021-11-04T09:58:53Z","messageCount":30,"participants":["Alejandro Sanchez","Jeff King","Johannes Schindelin","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"375898","messageId":"CAA4phoGrA=AsBFHkqZ54=6ev5BH+F7rLd0Hcr-t2kF_YFNrs2g@mail.gmail.com","threadId":"51129","inReplyTo":null,"subject":"Abort (core dumped)","fromName":"Alejandro Sanchez","fromEmail":"asanchez1987@gmail.com","sentAt":"2019-05-20T08:35:53Z","receivedAt":"2019-05-20T08:36:21Z","isPatch":false,"sender":{"key":"asanchez1987@gmail.com","avatar":null},"body":"A core dump occurred while trying to interactively apply (3-way) a\nseries of patches from a mailbox.\n\ngit version 2.20.1\n\nSteps to reproduce:\n\nalex@polaris:~/slurm/source$ git am -3 -i\n~/Downloads/6033-18.08-final-patchset-v2.patch\nCommit Body is:\n--------------------------\nUse correct signed/unsiged types.\n\nChange a few variables in archiving to use the correct signed or\nunsigned type to avoid implicit casting.\n\nBug 6033.\n--------------------------\nApply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y\nApplying: Use correct signed/unsiged types.\nCommit Body is:\n--------------------------\nRemove unused static variable high_buffer_size.\n\nIt was set but never read.\n\nBug 6033.\n--------------------------\nApply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y\nApplying: Remove unused static variable high_buffer_size.\nCommit Body is:\n--------------------------\nHandle duplicate archive file names.\n\nThe time period of the archive file currently depends on submit or start\ntime and whether the purge period is in hours, days, or months.\nPreviously, if the archive file name already exists, we would overwrite\nthe old archive file with the assumption that these are duplicate\nrecords being archived after an archive load. However, that could result\nin lost records in a couple of ways:\n\n * If there were runaway jobs that were part of an old archive file's\n time period and are later fixed and then purged, the old file would\n be overwritten.\n * If jobs or steps are purged but there are still jobs or steps in\n that time period that are pending or running, the pending or running\n jobs and steps won't be purged. When they finish and are purged, the\n old file would be overwritten.\n\nInstead of overwriting the old file, we append a number to the file name\nto create a new file. This will also be important in an upcoming commit.\n\nBug 6033.\n--------------------------\nApply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y\nApplying: Handle duplicate archive file names.\nUsing index info to reconstruct a base tree...\nM    NEWS\nFalling back to patching base and 3-way merge...\nAuto-merging NEWS\nCONFLICT (content): Merge conflict in NEWS\nerror: Failed to merge in the changes.\nPatch failed at 0003 Handle duplicate archive file names.\nhint: Use 'git am --show-current-patch' to see the failed patch\nWhen you have resolved this problem, run \"git am -i --continue\".\nIf you prefer to skip this patch, run \"git am -i --skip\" instead.\nTo restore the original branch and stop patching, run \"git am -i --abort\".\nalex@polaris:~/slurm/source$ vi NEWS\nalex@polaris:~/slurm/source$ man git add\nalex@polaris:~/slurm/source$ git add -u\nalex@polaris:~/slurm/source$ git am -i --continue\nApplying: Handle duplicate archive file names.\nerror: object 861d3c6f689a3ca5eb5fb5c409d46de0ad5555e1 is a commit, not a tree\nBUG: diff-lib.c:526: run_diff_index must be passed exactly one tree\nAborted (core dumped)\nalex@polaris:~/slurm/source$\n"},{"id":"375903","messageId":"20190520100211.GA26426@sigill.intra.peff.net","threadId":"51129","inReplyTo":"CAA4phoGrA=AsBFHkqZ54=6ev5BH+F7rLd0Hcr-t2kF_YFNrs2g@mail.gmail.com","subject":"Re: Abort (core dumped)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T10:02:11Z","receivedAt":"2019-05-20T10:02:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 20, 2019 at 10:35:53AM +0200, Alejandro Sanchez wrote:\n\n> alex@polaris:~/slurm/source$ git add -u\n> alex@polaris:~/slurm/source$ git am -i --continue\n> Applying: Handle duplicate archive file names.\n> error: object 861d3c6f689a3ca5eb5fb5c409d46de0ad5555e1 is a commit, not a tree\n> BUG: diff-lib.c:526: run_diff_index must be passed exactly one tree\n> Aborted (core dumped)\n\nHmm. So I think the interesting error is probably that first line: some\ncode expects to look up a tree but sees a commit, and then as a result\nit probably feeds too few items to run_diff_index(), triggering the\nassertion failure.\n\nJust grepping around, this looks quite suspicious:\n\n  $ git grep -hW get_oid_tree builtin/am.c\n  static void write_index_patch(const struct am_state *state)\n  ...\n  if (!get_oid_tree(\"HEAD\", &head))\n\ttree = lookup_tree(the_repository, &head);\n  else\n\ttree = lookup_tree(the_repository,\n\t\t\t   the_repository->hash_algo->empty_tree);\n\nUsing get_oid_tree() does not actually return a tree; it just\nprioritizes trees when disambiguating names (which is pointless here,\nsince we're not feeding an ambiguous oid). HEAD will always be a commit,\nand then lookup_tree() similarly does not peel that down to an actual\ntree. And this whole function is called only in interactive-mode, so\nit's possible that it's simply not used much and nobody noticed.\n\nI haven't tried to reproduce yet. Is the repository (and patch) that you\nused to demonstrate this publicly available? Or alternatively, is it\npossible to show a backtrace from the coredump?\n\nIf my blind guess is right, then something like this probably fixes it:\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex bdd1bbc35d..93305560c1 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1339,9 +1339,17 @@ static void write_index_patch(const struct am_state *state)\n \tstruct rev_info rev_info;\n \tFILE *fp;\n \n-\tif (!get_oid_tree(\"HEAD\", &head))\n-\t\ttree = lookup_tree(the_repository, &head);\n-\telse\n+\tif (!get_oid(\"HEAD\", &head)) {\n+\t\tstruct object *obj;\n+\t\tstruct commit *commit;\n+\n+\t\tobj = parse_object_or_die(&head, NULL);\n+\t\tcommit = object_as_type(the_repository, obj, OBJ_COMMIT, 0);\n+\t\tif (!commit)\n+\t\t\tdie(\"unable to parse HEAD as a commit\");\n+\n+\t\ttree = get_commit_tree(commit);\n+\t} else\n \t\ttree = lookup_tree(the_repository,\n \t\t\t\t   the_repository->hash_algo->empty_tree);\n \n\n-Peff\n"},{"id":"375912","messageId":"20190520120636.GA12634@sigill.intra.peff.net","threadId":"51129","inReplyTo":"20190520100211.GA26426@sigill.intra.peff.net","subject":"[PATCH 0/4] fix BUG() with \"git am -i --resolved\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T12:06:36Z","receivedAt":"2019-05-20T12:06:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 20, 2019 at 06:02:11AM -0400, Jeff King wrote:\n\n> I haven't tried to reproduce yet. Is the repository (and patch) that you\n> used to demonstrate this publicly available? Or alternatively, is it\n> possible to show a backtrace from the coredump?\n\nNever mind. It reproduces quite easily, because AFAICT \"am -i\n--resolved\" has been totally broken since it was converted to C (in\n2015!). I guess it does not have many users. :)\n\n> If my blind guess is right, then something like this probably fixes it:\n\nThis is indeed the problem. Here are some patches. The actual fix is\nwhat I showed already. The other three are just making it possible to\ncover this with the test suite (until now, we had no coverage of \"am\n--interactive\" at all).\n\n  [1/4]: am: simplify prompt response handling\n  [2/4]: am: read interactive input from stdin\n  [3/4]: am: drop tty requirement for --interactive\n  [4/4]: am: fix --interactive HEAD tree resolution\n\n builtin/am.c              | 30 +++++++++++++++---------\n t/t4257-am-interactive.sh | 49 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 68 insertions(+), 11 deletions(-)\n create mode 100755 t/t4257-am-interactive.sh\n\n-Peff\n"},{"id":"375913","messageId":"20190520120715.GA11212@sigill.intra.peff.net","threadId":"51129","inReplyTo":"20190520120636.GA12634@sigill.intra.peff.net","subject":"[PATCH 1/4] am: simplify prompt response handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T12:07:15Z","receivedAt":"2019-05-20T12:07:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We'll never see a NULL returned from git_prompt(); if it can't produce\nany input for us (e.g., because the terminal got EOF) then it will just\ndie().\n\nSo there's no need for us to handle NULL here. And even if there was, it\ndoesn't make sense to continue; on a true terminal hangup we'd just loop\ninfinitely trying to get input that will never come.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 912d9821b1..644bb11f6c 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1661,9 +1661,7 @@ static int do_interactive(struct am_state *state)\n \t\t */\n \t\treply = git_prompt(_(\"Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: \"), PROMPT_ECHO);\n \n-\t\tif (!reply) {\n-\t\t\tcontinue;\n-\t\t} else if (*reply == 'y' || *reply == 'Y') {\n+\t\tif (*reply == 'y' || *reply == 'Y') {\n \t\t\treturn 0;\n \t\t} else if (*reply == 'a' || *reply == 'A') {\n \t\t\tstate->interactive = 0;\n-- \n2.22.0.rc1.539.g7bfcdfe86d\n\n"},{"id":"375914","messageId":"20190520120926.GB11212@sigill.intra.peff.net","threadId":"51129","inReplyTo":"20190520120636.GA12634@sigill.intra.peff.net","subject":"[PATCH 2/4] am: read interactive input from stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T12:09:26Z","receivedAt":"2019-05-20T12:09:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the conversion of git-am from shell script to C, we switched to using\ngit_prompt(). Unlike the original shell command \"read reply\", this\ndoesn't read from stdin at all, but rather from /dev/tty.\n\nIn most cases this distinction wouldn't matter. We require (as the shell\nscript did) that stdin is a tty, so they would generally be the same\nthing. But one important exception is our test suite: even with\ntest_terminal, we cannot test \"am --interactive\" because it insists on\nreading from /dev/tty, not the pseudo-tty we've set up in the test\nscript.\n\nFixing this clears the way to adding tests in a future patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nPart of me dies inside when I look at adding the magical \"64\". But we\nexpect this input to be single-line (or at most, somebody might actually\ntype out \"edit\", etc), and avoiding strbuf_getline() saves us from\nhaving to free the strbuf in the many early return paths. I dunno. If\nit's too ugly it would not be too hard to switch it over.\n\n builtin/am.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 644bb11f6c..47ad7a5a70 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1647,7 +1647,7 @@ static int do_interactive(struct am_state *state)\n \t\tdie(_(\"cannot be interactive without stdin connected to a terminal.\"));\n \n \tfor (;;) {\n-\t\tconst char *reply;\n+\t\tchar reply[64];\n \n \t\tputs(_(\"Commit Body is:\"));\n \t\tputs(\"--------------------------\");\n@@ -1659,7 +1659,9 @@ static int do_interactive(struct am_state *state)\n \t\t * in your translation. The program will only accept English\n \t\t * input at this point.\n \t\t */\n-\t\treply = git_prompt(_(\"Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: \"), PROMPT_ECHO);\n+\t\tprintf(_(\"Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: \"));\n+\t\tif (!fgets(reply, sizeof(reply), stdin))\n+\t\t\tdie(\"unable to read from stdin; aborting\");\n \n \t\tif (*reply == 'y' || *reply == 'Y') {\n \t\t\treturn 0;\n-- \n2.22.0.rc1.539.g7bfcdfe86d\n\n"},{"id":"375915","messageId":"20190520121113.GC11212@sigill.intra.peff.net","threadId":"51129","inReplyTo":"20190520120636.GA12634@sigill.intra.peff.net","subject":"[PATCH 3/4] am: drop tty requirement for --interactive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T12:11:13Z","receivedAt":"2019-05-20T12:11:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have required that the stdin of \"am --interactive\" be a tty since\na1451104ac (git-am: interactive should fail gracefully., 2005-10-12).\nHowever, this isn't strictly necessary, and makes the tool harder to\ntest (and is unlike all of our other --interactive commands).\n\nThe goal of that commit was to make sure that somebody does not do:\n\n  git am --interactive <mbox\n\nand cause us to read commands from the mbox. But we can simply check\nup front for this case and complain before entering the interactive\nloop.\n\nTechnically this disallows:\n\n  git am --interactive </dev/null\n\nwhere our lack of patches means we would never prompt for anything, and\nso the old code would not notice our lack of tty (and now we'd die\nearly). But since such a command is totally pointless, it's no loss.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 47ad7a5a70..ea16b844f1 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1643,9 +1643,6 @@ static int do_interactive(struct am_state *state)\n {\n \tassert(state->msg);\n \n-\tif (!isatty(0))\n-\t\tdie(_(\"cannot be interactive without stdin connected to a terminal.\"));\n-\n \tfor (;;) {\n \t\tchar reply[64];\n \n@@ -2334,6 +2331,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\t\targv_array_push(&paths, mkpath(\"%s/%s\", prefix, argv[i]));\n \t\t}\n \n+\t\tif (state.interactive && !paths.argc)\n+\t\t\tdie(_(\"interactive mode requires patches on the command line\"));\n+\n \t\tam_setup(&state, patch_format, paths.argv, keep_cr);\n \n \t\targv_array_clear(&paths);\n-- \n2.22.0.rc1.539.g7bfcdfe86d\n\n"},{"id":"375916","messageId":"20190520121301.GD11212@sigill.intra.peff.net","threadId":"51129","inReplyTo":"20190520120636.GA12634@sigill.intra.peff.net","subject":"[PATCH 4/4] am: fix --interactive HEAD tree resolution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T12:13:01Z","receivedAt":"2019-05-20T12:13:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In interactive mode, \"git am -i --resolved\" will try to generate a patch\nbased on what is in the index, so that it can prompt \"apply this\npatch?\". To do so it needs the tree of HEAD, which it tries to get with\nget_oid_tree(). However, this doesn't yield a tree oid; the \"tree\" part\njust means \"if you must disambiguate short oids, then prefer trees\" (and\nwe do not need to disambiguate at all, since we are feeding a ref name).\n\nInstead, we must parse the oid as a commit (which should always be true\nin a non-corrupt repository), and access its tree pointer manually.\n\nThis has been broken since the conversion to C in 7ff2683253\n(builtin-am: implement -i/--interactive, 2015-08-04), but there was no\ntest coverage because of interactive-mode's insistence on having a tty.\nThat was lifted in the previous commit, so we can now add a test for\nthis case.\n\nNote that before this patch, the test would result in a BUG() which\ncomes from 3506dc9445 (has_uncommitted_changes(): fall back to empty\ntree, 2018-07-11). But before that, we'd have simply segfaulted (and in\nfact this is the exact type of case the BUG() added there was trying to\ncatch!).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c              | 14 ++++++++---\n t/t4257-am-interactive.sh | 49 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 60 insertions(+), 3 deletions(-)\n create mode 100755 t/t4257-am-interactive.sh\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex ea16b844f1..33bd7a6eab 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1339,9 +1339,17 @@ static void write_index_patch(const struct am_state *state)\n \tstruct rev_info rev_info;\n \tFILE *fp;\n \n-\tif (!get_oid_tree(\"HEAD\", &head))\n-\t\ttree = lookup_tree(the_repository, &head);\n-\telse\n+\tif (!get_oid(\"HEAD\", &head)) {\n+\t\tstruct object *obj;\n+\t\tstruct commit *commit;\n+\n+\t\tobj = parse_object_or_die(&head, NULL);\n+\t\tcommit = object_as_type(the_repository, obj, OBJ_COMMIT, 0);\n+\t\tif (!commit)\n+\t\t\tdie(\"unable to parse HEAD as a commit\");\n+\n+\t\ttree = get_commit_tree(commit);\n+\t} else\n \t\ttree = lookup_tree(the_repository,\n \t\t\t\t   the_repository->hash_algo->empty_tree);\n \ndiff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh\nnew file mode 100755\nindex 0000000000..6989bf7aba\n--- /dev/null\n+++ b/t/t4257-am-interactive.sh\n@@ -0,0 +1,49 @@\n+#!/bin/sh\n+\n+test_description='am --interactive tests'\n+. ./test-lib.sh\n+\n+test_expect_success 'set up patches to apply' '\n+\ttest_commit unrelated &&\n+\ttest_commit no-conflict &&\n+\ttest_commit conflict-patch file patch &&\n+\tgit format-patch --stdout -2 >mbox &&\n+\n+\tgit reset --hard unrelated &&\n+\ttest_commit conflict-master file master base\n+'\n+\n+# Sanity check our setup.\n+test_expect_success 'applying all patches generates conflict' '\n+\ttest_must_fail git am mbox &&\n+\techo resolved >file &&\n+\tgit add -u &&\n+\tgit am --resolved\n+'\n+\n+test_expect_success 'interactive am can apply a single patch' '\n+\tgit reset --hard base &&\n+\tprintf \"%s\\n\" y n | git am -i mbox &&\n+\n+\techo no-conflict >expect &&\n+\tgit log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'interactive am can resolve conflict' '\n+\tgit reset --hard base &&\n+\tprintf \"%s\\n\" y y | test_must_fail git am -i mbox &&\n+\techo resolved >file &&\n+\tgit add -u &&\n+\tprintf \"%s\\n\" v y | git am -i --resolved &&\n+\n+\techo conflict-patch >expect &&\n+\tgit log -1 --format=%s >actual &&\n+\ttest_cmp expect actual &&\n+\n+\techo resolved >expect &&\n+\tgit cat-file blob HEAD:file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.22.0.rc1.539.g7bfcdfe86d\n"},{"id":"375919","messageId":"20190520125016.GA13474@sigill.intra.peff.net","threadId":"51129","inReplyTo":"20190520121113.GC11212@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] am: drop tty requirement for --interactive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-20T12:50:17Z","receivedAt":"2019-05-20T12:50:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 20, 2019 at 08:11:13AM -0400, Jeff King wrote:\n\n> We have required that the stdin of \"am --interactive\" be a tty since\n> a1451104ac (git-am: interactive should fail gracefully., 2005-10-12).\n> However, this isn't strictly necessary, and makes the tool harder to\n> test (and is unlike all of our other --interactive commands).\n\nI think this is worth doing for simplicity and consistency. But as you\nmight guess, my ulterior motive was making it easier to add tests.\n\nIn theory we _should_ be able to use test_terminal for this, but it\nseems to be racy, because it will quickly read all input and close the\ndescriptor (to give the reader EOF). But after that close, isatty() will\nno longer report it correctly. E.g., if I run this:\n\n  perl test-terminal.perl sh -c '\n\tfor i in 0 1 2; do\n\t\techo $i is $(test -t $i || echo not) a tty\n\tdone\n  ' </dev/null\n\nit _usually_ says \"0 is a tty\", but racily may say \"not a tty\". If you\nput a sleep into the beginning of the shell, then it will basically\nalways lose the race and say \"not\".\n\nIt might be possible to overcome this by making test-terminal more\nclever (i.e., is there a way for us to send an \"EOF\" over the pty\nwithout actually _closing_ it? That would behave like a real terminal,\nwhere you can hit ^D to generate an EOF but then type more).\n\nBut barring that, this works by just avoiding it entirely. :)\n\nCuriously, my script above also reports consistently that stdout is not\na tty, but that stderr is. I'm not sure why this is, but it no tests\nseem to care either way.\n\n-Peff\n"},{"id":"376096","messageId":"nycvar.QRO.7.76.6.1905230840450.46@tvgsbejvaqbjf.bet","threadId":"51129","inReplyTo":"20190520125016.GA13474@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] am: drop tty requirement for --interactive","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-23T06:44:31Z","receivedAt":"2019-05-23T06:44:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 20 May 2019, Jeff King wrote:\n\n> On Mon, May 20, 2019 at 08:11:13AM -0400, Jeff King wrote:\n>\n> > We have required that the stdin of \"am --interactive\" be a tty since\n> > a1451104ac (git-am: interactive should fail gracefully., 2005-10-12).\n> > However, this isn't strictly necessary, and makes the tool harder to\n> > test (and is unlike all of our other --interactive commands).\n>\n> I think this is worth doing for simplicity and consistency. But as you\n> might guess, my ulterior motive was making it easier to add tests.\n>\n> In theory we _should_ be able to use test_terminal for this, but it\n> seems to be racy, because it will quickly read all input and close the\n> descriptor (to give the reader EOF). But after that close, isatty() will\n> no longer report it correctly. E.g., if I run this:\n>\n>   perl test-terminal.perl sh -c '\n> \tfor i in 0 1 2; do\n> \t\techo $i is $(test -t $i || echo not) a tty\n> \tdone\n>   ' </dev/null\n>\n> it _usually_ says \"0 is a tty\", but racily may say \"not a tty\". If you\n> put a sleep into the beginning of the shell, then it will basically\n> always lose the race and say \"not\".\n\nThis is just another nail in the coffin for `test-terminal.perl`, as far\nas I am concerned.\n\nIn the built-in `add -i` patch series, I followed a strategy where I move\ntotally away from `test-terminal`, in favor of using some knobs to force\nGit into thinking that we are in a terminal.\n\nBut at the same time, I *also* remove the limitation (for most cases) of\n\"read from /dev/tty\", in favor of reading from stdin, and making things\ntestable, and more importantly: scriptable.\n\nSo I am *very* much in favor of this here patch.\n\nThanks,\nDscho\n\nP.S.: There are even more reasons to get rid of `test-terminal`, of\ncourse: it is an unnecessary dependency on Perl, works only when certain\nPerl modules are installed (that are *not* installed on Ubuntu by default,\nfor example), and it requires pseudo terminals, so it will *never* work on\nWindows.\n\n> It might be possible to overcome this by making test-terminal more\n> clever (i.e., is there a way for us to send an \"EOF\" over the pty\n> without actually _closing_ it? That would behave like a real terminal,\n> where you can hit ^D to generate an EOF but then type more).\n>\n> But barring that, this works by just avoiding it entirely. :)\n>\n> Curiously, my script above also reports consistently that stdout is not\n> a tty, but that stderr is. I'm not sure why this is, but it no tests\n> seem to care either way.\n>\n> -Peff\n>\n"},{"id":"376097","messageId":"nycvar.QRO.7.76.6.1905230858570.46@tvgsbejvaqbjf.bet","threadId":"51129","inReplyTo":"20190520121301.GD11212@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-23T07:12:27Z","receivedAt":"2019-05-23T07:12:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 20 May 2019, Jeff King wrote:\n\n> In interactive mode, \"git am -i --resolved\" will try to generate a patch\n> based on what is in the index, so that it can prompt \"apply this\n> patch?\". To do so it needs the tree of HEAD, which it tries to get with\n> get_oid_tree(). However, this doesn't yield a tree oid; the \"tree\" part\n> just means \"if you must disambiguate short oids, then prefer trees\" (and\n> we do not need to disambiguate at all, since we are feeding a ref name).\n>\n> Instead, we must parse the oid as a commit (which should always be true\n> in a non-corrupt repository), and access its tree pointer manually.\n>\n> This has been broken since the conversion to C in 7ff2683253\n> (builtin-am: implement -i/--interactive, 2015-08-04), but there was no\n> test coverage because of interactive-mode's insistence on having a tty.\n> That was lifted in the previous commit, so we can now add a test for\n> this case.\n>\n> Note that before this patch, the test would result in a BUG() which\n> comes from 3506dc9445 (has_uncommitted_changes(): fall back to empty\n> tree, 2018-07-11). But before that, we'd have simply segfaulted (and in\n> fact this is the exact type of case the BUG() added there was trying to\n> catch!).\n\nWhat an old breakage! Thanks for analyzing and fixing it.\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index ea16b844f1..33bd7a6eab 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1339,9 +1339,17 @@ static void write_index_patch(const struct am_state *state)\n>  \tstruct rev_info rev_info;\n>  \tFILE *fp;\n>\n> -\tif (!get_oid_tree(\"HEAD\", &head))\n> -\t\ttree = lookup_tree(the_repository, &head);\n> -\telse\n> +\tif (!get_oid(\"HEAD\", &head)) {\n> +\t\tstruct object *obj;\n> +\t\tstruct commit *commit;\n> +\n> +\t\tobj = parse_object_or_die(&head, NULL);\n> +\t\tcommit = object_as_type(the_repository, obj, OBJ_COMMIT, 0);\n> +\t\tif (!commit)\n> +\t\t\tdie(\"unable to parse HEAD as a commit\");\n\nWouldn't this be easier to read like this:\n\n\t\tstruct commit *commit =\n\t\t\tlookup_commit_reference(the_repository, &head);\n\n> +\n> +\t\ttree = get_commit_tree(commit);\n> +\t} else\n>  \t\ttree = lookup_tree(the_repository,\n>  \t\t\t\t   the_repository->hash_algo->empty_tree);\n>\n> diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh\n> new file mode 100755\n> index 0000000000..6989bf7aba\n> --- /dev/null\n> +++ b/t/t4257-am-interactive.sh\n> @@ -0,0 +1,49 @@\n> +#!/bin/sh\n> +\n> +test_description='am --interactive tests'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'set up patches to apply' '\n> +\ttest_commit unrelated &&\n> +\ttest_commit no-conflict &&\n> +\ttest_commit conflict-patch file patch &&\n> +\tgit format-patch --stdout -2 >mbox &&\n> +\n> +\tgit reset --hard unrelated &&\n> +\ttest_commit conflict-master file master base\n> +'\n> +\n> +# Sanity check our setup.\n> +test_expect_success 'applying all patches generates conflict' '\n> +\ttest_must_fail git am mbox &&\n> +\techo resolved >file &&\n> +\tgit add -u &&\n> +\tgit am --resolved\n> +'\n> +\n> +test_expect_success 'interactive am can apply a single patch' '\n> +\tgit reset --hard base &&\n> +\tprintf \"%s\\n\" y n | git am -i mbox &&\n\nSince we want contributors to copy-edit our test cases (even if they do\nnot happen to be Unix shell scripting experts), it would be better to\nwrite\n\n\ttest_write_lines y n | git am -i mbox &&\n\nhere. Same for similar `printf` invocations further down.\n\n> +\n> +\techo no-conflict >expect &&\n> +\tgit log -1 --format=%s >actual &&\n> +\ttest_cmp expect actual\n\nI would prefer\n\n\ttest no-conflict = \"$(git show -s --format=%s HEAD)\"\n\nor even better:\n\ntest_cmp_head_oneline () {\n\tif test \"$1\" != \"$(git show -s --format=%s HEAD)\"\n\tthen\n\t\techo >&4 \"HEAD's oneline is '$(git show -s \\\n\t\t\t--format=%s HEAD)'; expected '$1'\"\n\t\treturn 1\n\tfi\n}\n\n> +'\n> +\n> +test_expect_success 'interactive am can resolve conflict' '\n> +\tgit reset --hard base &&\n> +\tprintf \"%s\\n\" y y | test_must_fail git am -i mbox &&\n> +\techo resolved >file &&\n> +\tgit add -u &&\n> +\tprintf \"%s\\n\" v y | git am -i --resolved &&\n\nMaybe a comment, to explain to the casual reader what the \"v\" and the \"y\"\nare supposed to do?\n\n> +\n> +\techo conflict-patch >expect &&\n> +\tgit log -1 --format=%s >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\techo resolved >expect &&\n> +\tgit cat-file blob HEAD:file >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nAfter wrapping my head around the intentions of these commands, I agree\nthat they test for the right thing.\n\nThanks!\nDscho\n\n> +\n> +test_done\n> --\n> 2.22.0.rc1.539.g7bfcdfe86d\n>\n"},{"id":"376144","messageId":"20190524062724.GC25694@sigill.intra.peff.net","threadId":"51129","inReplyTo":"nycvar.QRO.7.76.6.1905230840450.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/4] am: drop tty requirement for --interactive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-24T06:27:24Z","receivedAt":"2019-05-24T06:27:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 23, 2019 at 08:44:31AM +0200, Johannes Schindelin wrote:\n\n> >   perl test-terminal.perl sh -c '\n> > \tfor i in 0 1 2; do\n> > \t\techo $i is $(test -t $i || echo not) a tty\n> > \tdone\n> >   ' </dev/null\n> >\n> > it _usually_ says \"0 is a tty\", but racily may say \"not a tty\". If you\n> > put a sleep into the beginning of the shell, then it will basically\n> > always lose the race and say \"not\".\n> \n> This is just another nail in the coffin for `test-terminal.perl`, as far\n> as I am concerned.\n\nI think it's only broken for stdin, but yeah, it's not great. I think\nthe fact that test-terminal is not available everywhere (and thus many\npeople are skipping a bunch of tests) is much more damning. :)\n\n> In the built-in `add -i` patch series, I followed a strategy where I move\n> totally away from `test-terminal`, in favor of using some knobs to force\n> Git into thinking that we are in a terminal.\n\nI'm in favor of this. The current \"add -i\" is pretty accepting of\nreading from stdin, and I think we can do that in most places. The main\nuse of test_terminal has been to check color and progress decisions. I'd\nbe just as happy to see something like this:\n\n  int git_isatty(int fd)\n  {\n\tstatic int override[3];\n\tstatic int initialized;\n\tif (!initialized) {\n\t\tconst char *x = getenv(\"GIT_PRETEND_TTY\");\n\t\tif (x) {\n\t\t\tfor (; *x; x++) {\n\t\t\t\tint n = *x - '0';\n\t\t\t\tif (n > 0 && n < ARRAY_SIZE(override)\n\t\t\t\t\toverride[n] = 1;\n\t\t\t}\n\t\t}\n\t\tinitialized = 1;\n\t}\n\tif (fd > 0 && fd < ARRAY_SIZE(override) && override[fd])\n\t\treturn 1;\n\treturn isatty(fd);\n  }\n\n> But at the same time, I *also* remove the limitation (for most cases) of\n> \"read from /dev/tty\", in favor of reading from stdin, and making things\n> testable, and more importantly: scriptable.\n\nAs far as I know, apart from this git-am fix, the only thing that reads\nfrom the terminal is the credential prompt. That one has to be a bit\npicky, because:\n\n  - we need to prompt from processes which have no stdio connected to\n    the user (e.g., remote-curl).\n\n  - we need to put the terminal into no-echo mode for passwords (and\n    probably should bail if that fails, to be paranoid)\n\nIn the case of credentials we already have multiple mechanisms for\nscripting the input (credential helpers and askpass). It would be nice\nto be able to test the terminal-level code automatically, but I'm just\nnot sure how that would work.\n\n-Peff\n"},{"id":"376145","messageId":"20190524063955.GD25694@sigill.intra.peff.net","threadId":"51129","inReplyTo":"nycvar.QRO.7.76.6.1905230858570.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-24T06:39:55Z","receivedAt":"2019-05-24T06:39:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 23, 2019 at 09:12:27AM +0200, Johannes Schindelin wrote:\n\n> > +\tif (!get_oid(\"HEAD\", &head)) {\n> > +\t\tstruct object *obj;\n> > +\t\tstruct commit *commit;\n> > +\n> > +\t\tobj = parse_object_or_die(&head, NULL);\n> > +\t\tcommit = object_as_type(the_repository, obj, OBJ_COMMIT, 0);\n> > +\t\tif (!commit)\n> > +\t\t\tdie(\"unable to parse HEAD as a commit\");\n> \n> Wouldn't this be easier to read like this:\n> \n> \t\tstruct commit *commit =\n> \t\t\tlookup_commit_reference(the_repository, &head);\n\nJust the first two lines, I assume you mean; we still have to die\nourselves. There is a lookup_commit_or_die(), but weirdly it warns if\nthere was any tag dereferencing. I guess that would never happen here,\nsince we're reading HEAD (and most of the existing calls appear to be\nfor HEAD). So I'll go with that.\n\n> > +test_expect_success 'interactive am can apply a single patch' '\n> > +\tgit reset --hard base &&\n> > +\tprintf \"%s\\n\" y n | git am -i mbox &&\n> \n> Since we want contributors to copy-edit our test cases (even if they do\n> not happen to be Unix shell scripting experts), it would be better to\n> write\n> \n> \ttest_write_lines y n | git am -i mbox &&\n> \n> here. Same for similar `printf` invocations further down.\n\nI think test_write_lines is mostly about avoiding echo chains, but it's\nprobably a little more readable to avoid having to say \"\\n\". I'll adopt\nthat.\n\n> > +\techo no-conflict >expect &&\n> > +\tgit log -1 --format=%s >actual &&\n> > +\ttest_cmp expect actual\n> \n> I would prefer\n> \n> \ttest no-conflict = \"$(git show -s --format=%s HEAD)\"\n> \n> or even better:\n> \n> test_cmp_head_oneline () {\n> \tif test \"$1\" != \"$(git show -s --format=%s HEAD)\"\n> \tthen\n> \t\techo >&4 \"HEAD's oneline is '$(git show -s \\\n> \t\t\t--format=%s HEAD)'; expected '$1'\"\n> \t\treturn 1\n> \tfi\n> }\n\nThis, I disagree with. IMHO comparing command output using \"test\" is\nharder to read and produces worse debugging output (unless you do a\nhelper as you showed, which I think makes the readability even worse).\nNot to mention that it raises questions of the shell's whitespace\nhandling (though that does not matter for this case).\n\nWhat's your complaint with test_cmp? Is it the extra process? Could we\nperhaps deal with that by having it use `read` for the happy-path?\n\nOr do you prefer having a one-liner? I'd rather come up with a more\ngeneric helper to cover this case, that can run any command and compare\nit to a single argument (or stdin). E.g.,:\n\n  test_cmp_cmd no-conflict git log -1 --format=%s\n\nor\n\n  test_cmp_cmd - git foo <<-\\EOF\n  multi-line\n  expectation\n  EOF\n\nBut I'd rather approach those issues separately and systematically, and\nnot hold up this bug fix.\n\n> > +test_expect_success 'interactive am can resolve conflict' '\n> > +\tgit reset --hard base &&\n> > +\tprintf \"%s\\n\" y y | test_must_fail git am -i mbox &&\n> > +\techo resolved >file &&\n> > +\tgit add -u &&\n> > +\tprintf \"%s\\n\" v y | git am -i --resolved &&\n> \n> Maybe a comment, to explain to the casual reader what the \"v\" and the \"y\"\n> are supposed to do?\n\nOK. The \"v\" is actually optional, but I figured it would not hurt to\nhave us print the patch we just generated. I'll add a comment.\n\n> After wrapping my head around the intentions of these commands, I agree\n> that they test for the right thing.\n\nThanks!\n\n-Peff\n"},{"id":"376147","messageId":"20190524064626.GE25694@sigill.intra.peff.net","threadId":"51129","inReplyTo":"20190524063955.GD25694@sigill.intra.peff.net","subject":"[PATCH v2 4/4] am: fix --interactive HEAD tree resolution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-24T06:46:27Z","receivedAt":"2019-05-24T06:46:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In --interactive mode, \"git am --resolved\" will try to generate a patch\nbased on what is in the index, so that it can prompt \"apply this\npatch?\". To do so it needs the tree of HEAD, which it tries to get with\nget_oid_tree(). However, this doesn't yield a tree object; the \"tree\"\npart just means \"if you must disambiguate short oids, then prefer trees\"\n(and we do not need to disambiguate at all, since we are feeding a ref).\n\nInstead, we must parse the oid as a commit (which should always be true\nin a non-corrupt repository), and access its tree pointer manually.\n\nThis has been broken since the conversion to C in 7ff2683253\n(builtin-am: implement -i/--interactive, 2015-08-04), but there was no\ntest coverage because of interactive-mode's insistence on having a tty.\nThat was lifted in the previous commit, so we can now add a test for\nthis case.\n\nNote that before this patch, the test would result in a BUG() which\ncomes from 3506dc9445 (has_uncommitted_changes(): fall back to empty\ntree, 2018-07-11). But before that, we'd have simply segfaulted (and in\nfact this is the exact type of case the BUG() added there was trying to\ncatch!).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis addresses points raised in Dscho's review (except for the test_cmp\nthing). Since there were no changes to patches 1-3, I didn't bother\nre-posting. Diff against v1 below.\n\n  diff --git a/builtin/am.c b/builtin/am.c\n  index 33bd7a6eab..78389d08b6 100644\n  --- a/builtin/am.c\n  +++ b/builtin/am.c\n  @@ -1340,14 +1340,7 @@ static void write_index_patch(const struct am_state *state)\n   \tFILE *fp;\n   \n   \tif (!get_oid(\"HEAD\", &head)) {\n  -\t\tstruct object *obj;\n  -\t\tstruct commit *commit;\n  -\n  -\t\tobj = parse_object_or_die(&head, NULL);\n  -\t\tcommit = object_as_type(the_repository, obj, OBJ_COMMIT, 0);\n  -\t\tif (!commit)\n  -\t\t\tdie(\"unable to parse HEAD as a commit\");\n  -\n  +\t\tstruct commit *commit = lookup_commit_or_die(&head, \"HEAD\");\n   \t\ttree = get_commit_tree(commit);\n   \t} else\n   \t\ttree = lookup_tree(the_repository,\n  diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh\n  index 6989bf7aba..5344bd248a 100755\n  --- a/t/t4257-am-interactive.sh\n  +++ b/t/t4257-am-interactive.sh\n  @@ -23,7 +23,8 @@ test_expect_success 'applying all patches generates conflict' '\n   \n   test_expect_success 'interactive am can apply a single patch' '\n   \tgit reset --hard base &&\n  -\tprintf \"%s\\n\" y n | git am -i mbox &&\n  +\t# apply the first, but not the second\n  +\ttest_write_lines y n | git am -i mbox &&\n   \n   \techo no-conflict >expect &&\n   \tgit log -1 --format=%s >actual &&\n  @@ -32,10 +33,12 @@ test_expect_success 'interactive am can apply a single patch' '\n   \n   test_expect_success 'interactive am can resolve conflict' '\n   \tgit reset --hard base &&\n  -\tprintf \"%s\\n\" y y | test_must_fail git am -i mbox &&\n  +\t# apply both; the second one will conflict\n  +\ttest_write_lines y y | test_must_fail git am -i mbox &&\n   \techo resolved >file &&\n   \tgit add -u &&\n  -\tprintf \"%s\\n\" v y | git am -i --resolved &&\n  +\t# interactive \"--resolved\" will ask us if we want to apply the result\n  +\techo y | git am -i --resolved &&\n   \n   \techo conflict-patch >expect &&\n   \tgit log -1 --format=%s >actual &&\n\n builtin/am.c              |  7 +++---\n t/t4257-am-interactive.sh | 52 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 56 insertions(+), 3 deletions(-)\n create mode 100755 t/t4257-am-interactive.sh\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex ea16b844f1..78389d08b6 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1339,9 +1339,10 @@ static void write_index_patch(const struct am_state *state)\n \tstruct rev_info rev_info;\n \tFILE *fp;\n \n-\tif (!get_oid_tree(\"HEAD\", &head))\n-\t\ttree = lookup_tree(the_repository, &head);\n-\telse\n+\tif (!get_oid(\"HEAD\", &head)) {\n+\t\tstruct commit *commit = lookup_commit_or_die(&head, \"HEAD\");\n+\t\ttree = get_commit_tree(commit);\n+\t} else\n \t\ttree = lookup_tree(the_repository,\n \t\t\t\t   the_repository->hash_algo->empty_tree);\n \ndiff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh\nnew file mode 100755\nindex 0000000000..5344bd248a\n--- /dev/null\n+++ b/t/t4257-am-interactive.sh\n@@ -0,0 +1,52 @@\n+#!/bin/sh\n+\n+test_description='am --interactive tests'\n+. ./test-lib.sh\n+\n+test_expect_success 'set up patches to apply' '\n+\ttest_commit unrelated &&\n+\ttest_commit no-conflict &&\n+\ttest_commit conflict-patch file patch &&\n+\tgit format-patch --stdout -2 >mbox &&\n+\n+\tgit reset --hard unrelated &&\n+\ttest_commit conflict-master file master base\n+'\n+\n+# Sanity check our setup.\n+test_expect_success 'applying all patches generates conflict' '\n+\ttest_must_fail git am mbox &&\n+\techo resolved >file &&\n+\tgit add -u &&\n+\tgit am --resolved\n+'\n+\n+test_expect_success 'interactive am can apply a single patch' '\n+\tgit reset --hard base &&\n+\t# apply the first, but not the second\n+\ttest_write_lines y n | git am -i mbox &&\n+\n+\techo no-conflict >expect &&\n+\tgit log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'interactive am can resolve conflict' '\n+\tgit reset --hard base &&\n+\t# apply both; the second one will conflict\n+\ttest_write_lines y y | test_must_fail git am -i mbox &&\n+\techo resolved >file &&\n+\tgit add -u &&\n+\t# interactive \"--resolved\" will ask us if we want to apply the result\n+\techo y | git am -i --resolved &&\n+\n+\techo conflict-patch >expect &&\n+\tgit log -1 --format=%s >actual &&\n+\ttest_cmp expect actual &&\n+\n+\techo resolved >expect &&\n+\tgit cat-file blob HEAD:file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.22.0.rc1.549.gadb183c4cb\n\n"},{"id":"376291","messageId":"nycvar.QRO.7.76.6.1905281301070.44@tvgsbejvaqbjf.bet","threadId":"51129","inReplyTo":"20190524063955.GD25694@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-28T11:06:21Z","receivedAt":"2019-05-28T11:06:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 24 May 2019, Jeff King wrote:\n\n> On Thu, May 23, 2019 at 09:12:27AM +0200, Johannes Schindelin wrote:\n>\n> > > +\techo no-conflict >expect &&\n> > > +\tgit log -1 --format=%s >actual &&\n> > > +\ttest_cmp expect actual\n> >\n> > I would prefer\n> >\n> > \ttest no-conflict = \"$(git show -s --format=%s HEAD)\"\n> >\n> > or even better:\n> >\n> > test_cmp_head_oneline () {\n> > \tif test \"$1\" != \"$(git show -s --format=%s HEAD)\"\n> > \tthen\n> > \t\techo >&4 \"HEAD's oneline is '$(git show -s \\\n> > \t\t\t--format=%s HEAD)'; expected '$1'\"\n> > \t\treturn 1\n> > \tfi\n> > }\n>\n> This, I disagree with. IMHO comparing command output using \"test\" is\n> harder to read and produces worse debugging output (unless you do a\n> helper as you showed, which I think makes the readability even worse).\n> Not to mention that it raises questions of the shell's whitespace\n> handling (though that does not matter for this case).\n>\n> What's your complaint with test_cmp? Is it the extra process? Could we\n> perhaps deal with that by having it use `read` for the happy-path?\n\nI would prefer it if we adopted a more descriptive style in the test\nsuite, as I always found that style to be much easier to work with (you\nmight have guessed that I am spending a lot of time chasing test\nfailures).\n\nSuccinctness is just one benefit of that.\n\nA more important benefit is that you can teach a helper that verifies\nonelines to show very useful information in case of a failure, something\nthat `test_cmp` cannot do because it is totally agnostic to what it\ncompares.\n\n> Or do you prefer having a one-liner? I'd rather come up with a more\n> generic helper to cover this case, that can run any command and compare\n> it to a single argument (or stdin). E.g.,:\n>\n>   test_cmp_cmd no-conflict git log -1 --format=%s\n>\n> or\n>\n>   test_cmp_cmd - git foo <<-\\EOF\n>   multi-line\n>   expectation\n>   EOF\n\nI guess that you and me go into completely opposite directions here. I\nwant something *less* general. Because I want to optimize for the\nunfortunate times when a test fails and most likely somebody else than the\noriginal author of the test case is tasked with figuring out what the heck\ngoes wrong.\n\nYou seem to want to optimize for writing test cases. Which I find -- with\nall due respect -- the wrong thing to optimize for. It is already dirt\neasy to write new test cases. But *good* test cases (i.e. easy to debug\nones)? Not so much.\n\n> But I'd rather approach those issues separately and systematically, and\n> not hold up this bug fix.\n\nSure.\n\n> > > +test_expect_success 'interactive am can resolve conflict' '\n> > > +\tgit reset --hard base &&\n> > > +\tprintf \"%s\\n\" y y | test_must_fail git am -i mbox &&\n> > > +\techo resolved >file &&\n> > > +\tgit add -u &&\n> > > +\tprintf \"%s\\n\" v y | git am -i --resolved &&\n> >\n> > Maybe a comment, to explain to the casual reader what the \"v\" and the \"y\"\n> > are supposed to do?\n>\n> OK. The \"v\" is actually optional, but I figured it would not hurt to\n> have us print the patch we just generated. I'll add a comment.\n\nThank you.\n\nCiao,\nDscho\n"},{"id":"376362","messageId":"20190528213529.GG24650@sigill.intra.peff.net","threadId":"51129","inReplyTo":"nycvar.QRO.7.76.6.1905281301070.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-28T21:35:30Z","receivedAt":"2019-05-28T21:35:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 28, 2019 at 01:06:21PM +0200, Johannes Schindelin wrote:\n\n> > Or do you prefer having a one-liner? I'd rather come up with a more\n> > generic helper to cover this case, that can run any command and compare\n> > it to a single argument (or stdin). E.g.,:\n> >\n> >   test_cmp_cmd no-conflict git log -1 --format=%s\n> >\n> > or\n> >\n> >   test_cmp_cmd - git foo <<-\\EOF\n> >   multi-line\n> >   expectation\n> >   EOF\n> \n> I guess that you and me go into completely opposite directions here. I\n> want something *less* general. Because I want to optimize for the\n> unfortunate times when a test fails and most likely somebody else than the\n> original author of the test case is tasked with figuring out what the heck\n> goes wrong.\n> \n> You seem to want to optimize for writing test cases. Which I find -- with\n> all due respect -- the wrong thing to optimize for. It is already dirt\n> easy to write new test cases. But *good* test cases (i.e. easy to debug\n> ones)? Not so much.\n\nHmm. I too want the test output to be useful to people other than the\ntest author. But I find the output from test_cmp perfectly fine there.\nMy first step in digging into a failure is usually to look at what\ncommands the test is running, which generally makes it obvious why we\nare expecting one thing and seeing another (or at least, just as obvious\nas a hand-written message).\n\nSo to me the two are equal on that front, which makes me want to go with\nthe thing that is shorter to write, as it makes it more likely the test\nwriter will write it. The _worst_ option IMHO is a straight-up use of\n\"test\" which provides no output at all in the test log of what value we\n_did_ see. That requires the person looking into the failure to re-run\nthe test, which is hard if it's a remote CI, or if the failure does not\nalways reproduce.\n\n-Peff\n"},{"id":"376394","messageId":"nycvar.QRO.7.76.6.1905291356240.44@tvgsbejvaqbjf.bet","threadId":"51129","inReplyTo":"20190528213529.GG24650@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-29T11:56:53Z","receivedAt":"2019-05-29T11:57:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 28 May 2019, Jeff King wrote:\n\n> On Tue, May 28, 2019 at 01:06:21PM +0200, Johannes Schindelin wrote:\n>\n> > > Or do you prefer having a one-liner? I'd rather come up with a more\n> > > generic helper to cover this case, that can run any command and compare\n> > > it to a single argument (or stdin). E.g.,:\n> > >\n> > >   test_cmp_cmd no-conflict git log -1 --format=%s\n> > >\n> > > or\n> > >\n> > >   test_cmp_cmd - git foo <<-\\EOF\n> > >   multi-line\n> > >   expectation\n> > >   EOF\n> >\n> > I guess that you and me go into completely opposite directions here. I\n> > want something *less* general. Because I want to optimize for the\n> > unfortunate times when a test fails and most likely somebody else than the\n> > original author of the test case is tasked with figuring out what the heck\n> > goes wrong.\n> >\n> > You seem to want to optimize for writing test cases. Which I find -- with\n> > all due respect -- the wrong thing to optimize for. It is already dirt\n> > easy to write new test cases. But *good* test cases (i.e. easy to debug\n> > ones)? Not so much.\n>\n> Hmm. I too want the test output to be useful to people other than the\n> test author. But I find the output from test_cmp perfectly fine there.\n> My first step in digging into a failure is usually to look at what\n> commands the test is running, which generally makes it obvious why we\n> are expecting one thing and seeing another (or at least, just as obvious\n> as a hand-written message).\n>\n> So to me the two are equal on that front, which makes me want to go with\n> the thing that is shorter to write, as it makes it more likely the test\n> writer will write it. The _worst_ option IMHO is a straight-up use of\n> \"test\" which provides no output at all in the test log of what value we\n> _did_ see. That requires the person looking into the failure to re-run\n> the test, which is hard if it's a remote CI, or if the failure does not\n> always reproduce.\n\nIf you think your version is easier to debug, then I won't object.\n\nThanks,\nDscho\n"},{"id":"382990","messageId":"CAA4phoF5xm=HuoAmor+dQEBqBJn_0thAT38ao8JtA6XhGCDQ6Q@mail.gmail.com","threadId":"51129","inReplyTo":"nycvar.QRO.7.76.6.1905291356240.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution","fromName":"Alejandro Sanchez","fromEmail":"asanchez1987@gmail.com","sentAt":"2019-09-26T14:20:05Z","receivedAt":"2019-09-26T14:20:35Z","isPatch":true,"sender":{"key":"asanchez1987@gmail.com","avatar":null},"body":"Hi,\n\nAre there any updates to this problem?\n\nThank you,\nAlex\n\n\nOn Wed, May 29, 2019 at 1:57 PM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Peff,\n>\n> On Tue, 28 May 2019, Jeff King wrote:\n>\n> > On Tue, May 28, 2019 at 01:06:21PM +0200, Johannes Schindelin wrote:\n> >\n> > > > Or do you prefer having a one-liner? I'd rather come up with a more\n> > > > generic helper to cover this case, that can run any command and compare\n> > > > it to a single argument (or stdin). E.g.,:\n> > > >\n> > > >   test_cmp_cmd no-conflict git log -1 --format=%s\n> > > >\n> > > > or\n> > > >\n> > > >   test_cmp_cmd - git foo <<-\\EOF\n> > > >   multi-line\n> > > >   expectation\n> > > >   EOF\n> > >\n> > > I guess that you and me go into completely opposite directions here. I\n> > > want something *less* general. Because I want to optimize for the\n> > > unfortunate times when a test fails and most likely somebody else than the\n> > > original author of the test case is tasked with figuring out what the heck\n> > > goes wrong.\n> > >\n> > > You seem to want to optimize for writing test cases. Which I find -- with\n> > > all due respect -- the wrong thing to optimize for. It is already dirt\n> > > easy to write new test cases. But *good* test cases (i.e. easy to debug\n> > > ones)? Not so much.\n> >\n> > Hmm. I too want the test output to be useful to people other than the\n> > test author. But I find the output from test_cmp perfectly fine there.\n> > My first step in digging into a failure is usually to look at what\n> > commands the test is running, which generally makes it obvious why we\n> > are expecting one thing and seeing another (or at least, just as obvious\n> > as a hand-written message).\n> >\n> > So to me the two are equal on that front, which makes me want to go with\n> > the thing that is shorter to write, as it makes it more likely the test\n> > writer will write it. The _worst_ option IMHO is a straight-up use of\n> > \"test\" which provides no output at all in the test log of what value we\n> > _did_ see. That requires the person looking into the failure to re-run\n> > the test, which is hard if it's a remote CI, or if the failure does not\n> > always reproduce.\n>\n> If you think your version is easier to debug, then I won't object.\n>\n> Thanks,\n> Dscho\n"},{"id":"383029","messageId":"20190926211132.GA17942@sigill.intra.peff.net","threadId":"51129","inReplyTo":"CAA4phoF5xm=HuoAmor+dQEBqBJn_0thAT38ao8JtA6XhGCDQ6Q@mail.gmail.com","subject":"Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-26T21:11:33Z","receivedAt":"2019-09-26T21:11:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2019 at 04:20:05PM +0200, Alejandro Sanchez wrote:\n\n> Are there any updates to this problem?\n\nThe fix for the original bug went into Git v2.22.1.\n\n-Peff\n"},{"id":"440339","messageId":"cover-0.2-00000000000-20211102T155046Z-avarab@gmail.com","threadId":"51129","inReplyTo":"20190524062724.GC25694@sigill.intra.peff.net","subject":"[PATCH 0/2] prompt.c: split up git_prompt(), read from /dev/tty, not STDIN","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-02T16:48:08Z","receivedAt":"2021-11-02T16:48:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I had a stupid bug in another series of mine[1] breaking code around\ngit_prompt() in bisect--helper.c, and in trying to follow-up to add\ntesting to that discovered that doing so was rather hard, due to how\ngit_prompt() behaves.\n\nIn another case in 2019 (which is the In-Reply-To here) we added a\nseeming for-the-tests workaround for the same issue in \"git am\n--interacive\".\n\nThis starts out by splitting the more complex \"get a password\ninteractively\" case out of prompt.c into a new prompt-password.c, most\nof prompt.c's complexity was to cater to one API user in credential.c.\n\nThat being done move these callers to using git_prompt(), and have it\nunderstanda a new GIT_TEST_TERMINAL_PROMPT=true variable so we can\nhave our cake and it it too in the test suite. If that's set the\nprompt function will allow input on stdin.\n\nThis then allowed my to add the missing test for \"git bisect\" for the\ncode I'd accidentally broken in [1].\n\nAs noted in the discussion in 2/2 I'm not 100% sure this is the right\ndirection in terms of swiching us back to using /dev/tty and not\nstdin, but see that commit & commit message for details. In any case,\nwhatever behavior we go for as far as the UX goes shouldn't be\ncatering only to the test suite at the cost of trade-offs elsewhere.\n\n1. https://lore.kernel.org/git/cover-v6-0.8-00000000000-20211102T122507Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (2):\n  prompt.c: split up the password and non-password handling\n  prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true\n\n Makefile                    |  1 +\n builtin/am.c                |  8 ++--\n builtin/bisect--helper.c    |  8 +---\n credential.c                | 17 ++++----\n help.c                      |  2 +-\n prompt-password.c           | 63 +++++++++++++++++++++++++++++\n prompt-password.h           |  7 ++++\n prompt.c                    | 79 ++++++++-----------------------------\n prompt.h                    |  7 +---\n t/t6030-bisect-porcelain.sh | 41 +++++++++++++++++++\n t/test-lib.sh               |  4 ++\n 11 files changed, 151 insertions(+), 86 deletions(-)\n create mode 100644 prompt-password.c\n create mode 100644 prompt-password.h\n\n-- \n2.33.1.1570.g069344fdd45\n\n"},{"id":"440340","messageId":"patch-1.2-ce5bea43f03-20211102T155046Z-avarab@gmail.com","threadId":"51129","inReplyTo":"cover-0.2-00000000000-20211102T155046Z-avarab@gmail.com","subject":"[PATCH 1/2] prompt.c: split up the password and non-password handling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-02T16:48:09Z","receivedAt":"2021-11-02T16:48:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Rather than pass PROMPT_ASKPASS and PROMPT_ECHO flags to git_prompt()\nlet's split it up into git_prompt_echo() and git_prompt_noecho()\nfunctions, the latter only ever needs to be called from credential.c,\nand the same goes for the previous password-only codepath in\ngit_prompt(), which is now exposed as a git_prompt_password().\n\nDoing this paves the way towards addressing some of the issues brought\nup in 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile                 |  1 +\n builtin/bisect--helper.c |  5 ++-\n credential.c             | 17 ++++++----\n help.c                   |  2 +-\n prompt-password.c        | 63 ++++++++++++++++++++++++++++++++++\n prompt-password.h        |  7 ++++\n prompt.c                 | 73 ++++++----------------------------------\n prompt.h                 |  7 ++--\n 8 files changed, 97 insertions(+), 78 deletions(-)\n create mode 100644 prompt-password.c\n create mode 100644 prompt-password.h\n\ndiff --git a/Makefile b/Makefile\nindex 12be39ac497..e8815860e22 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -967,6 +967,7 @@ LIB_OBJS += pretty.o\n LIB_OBJS += prio-queue.o\n LIB_OBJS += progress.o\n LIB_OBJS += promisor-remote.o\n+LIB_OBJS += prompt-password.o\n LIB_OBJS += prompt.o\n LIB_OBJS += protocol.o\n LIB_OBJS += protocol-caps.o\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 28a2e6a5750..30533a70b53 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -370,7 +370,7 @@ static int decide_next(const struct bisect_terms *terms,\n \t\t * translation. The program will only accept English input\n \t\t * at this point.\n \t\t */\n-\t\tyesno = git_prompt(_(\"Are you sure [Y/n]? \"), PROMPT_ECHO);\n+\t\tyesno = git_prompt_echo(_(\"Are you sure [Y/n]? \"));\n \t\tif (starts_with(yesno, \"N\") || starts_with(yesno, \"n\"))\n \t\t\treturn -1;\n \t\treturn 0;\n@@ -838,8 +838,7 @@ static int bisect_autostart(struct bisect_terms *terms)\n \t * translation. The program will only accept English input\n \t * at this point.\n \t */\n-\tyesno = git_prompt(_(\"Do you want me to do it for you \"\n-\t\t\t     \"[Y/n]? \"), PROMPT_ECHO);\n+\tyesno = git_prompt_echo(_(\"Do you want me to do it for you [Y/n]? \"));\n \tres = tolower(*yesno) == 'n' ?\n \t\t-1 : bisect_start(terms, empty_strvec, 0);\n \ndiff --git a/credential.c b/credential.c\nindex e7240f3f636..5509de382a5 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -5,6 +5,7 @@\n #include \"run-command.h\"\n #include \"url.h\"\n #include \"prompt.h\"\n+#include \"prompt-password.h\"\n #include \"sigchain.h\"\n #include \"urlmatch.h\"\n \n@@ -169,11 +170,11 @@ static void credential_format(struct credential *c, struct strbuf *out)\n \t}\n }\n \n-static char *credential_ask_one(const char *what, struct credential *c,\n-\t\t\t\tint flags)\n+static char *credential_ask_one(struct credential *c, unsigned int password)\n {\n \tstruct strbuf desc = STRBUF_INIT;\n \tstruct strbuf prompt = STRBUF_INIT;\n+\tconst char *what = password ? \"Password\" : \"Username\";\n \tchar *r;\n \n \tcredential_describe(c, &desc);\n@@ -182,7 +183,11 @@ static char *credential_ask_one(const char *what, struct credential *c,\n \telse\n \t\tstrbuf_addf(&prompt, \"%s: \", what);\n \n-\tr = git_prompt(prompt.buf, flags);\n+\t/* We'll try \"askpass\" for both usernames and passwords */\n+\tr = git_prompt_askpass(prompt.buf);\n+\tif (!r)\n+\t\tr = password ? git_prompt_noecho(prompt.buf)\n+\t\t\t     : git_prompt_echo(prompt.buf);\n \n \tstrbuf_release(&desc);\n \tstrbuf_release(&prompt);\n@@ -192,11 +197,9 @@ static char *credential_ask_one(const char *what, struct credential *c,\n static void credential_getpass(struct credential *c)\n {\n \tif (!c->username)\n-\t\tc->username = credential_ask_one(\"Username\", c,\n-\t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n+\t\tc->username = credential_ask_one(c, 0);\n \tif (!c->password)\n-\t\tc->password = credential_ask_one(\"Password\", c,\n-\t\t\t\t\t\t PROMPT_ASKPASS);\n+\t\tc->password = credential_ask_one(c, 1);\n }\n \n int credential_read(struct credential *c, FILE *fp)\ndiff --git a/help.c b/help.c\nindex 973e47cdc30..6eebc26fbeb 100644\n--- a/help.c\n+++ b/help.c\n@@ -644,7 +644,7 @@ const char *help_unknown_cmd(const char *cmd)\n \t\t\tchar *answer;\n \t\t\tstruct strbuf msg = STRBUF_INIT;\n \t\t\tstrbuf_addf(&msg, _(\"Run '%s' instead? (y/N)\"), assumed);\n-\t\t\tanswer = git_prompt(msg.buf, PROMPT_ECHO);\n+\t\t\tanswer = git_prompt_echo(msg.buf);\n \t\t\tstrbuf_release(&msg);\n \t\t\tif (!(starts_with(answer, \"y\") ||\n \t\t\t      starts_with(answer, \"Y\")))\ndiff --git a/prompt-password.c b/prompt-password.c\nnew file mode 100644\nindex 00000000000..5fa00363d6c\n--- /dev/null\n+++ b/prompt-password.c\n@@ -0,0 +1,63 @@\n+#include \"cache.h\"\n+#include \"prompt-password.h\"\n+#include \"strbuf.h\"\n+#include \"run-command.h\"\n+#include \"prompt.h\"\n+\n+static char *do_askpass(const char *cmd, const char *prompt)\n+{\n+\tstruct child_process pass = CHILD_PROCESS_INIT;\n+\tconst char *args[3];\n+\tstatic struct strbuf buffer = STRBUF_INIT;\n+\tint err = 0;\n+\n+\targs[0] = cmd;\n+\targs[1]\t= prompt;\n+\targs[2] = NULL;\n+\n+\tpass.argv = args;\n+\tpass.out = -1;\n+\n+\tif (start_command(&pass))\n+\t\treturn NULL;\n+\n+\tstrbuf_reset(&buffer);\n+\tif (strbuf_read(&buffer, pass.out, 20) < 0)\n+\t\terr = 1;\n+\n+\tclose(pass.out);\n+\n+\tif (finish_command(&pass))\n+\t\terr = 1;\n+\n+\tif (err) {\n+\t\terror(\"unable to read askpass response from '%s'\", cmd);\n+\t\tstrbuf_release(&buffer);\n+\t\treturn NULL;\n+\t}\n+\n+\tstrbuf_setlen(&buffer, strcspn(buffer.buf, \"\\r\\n\"));\n+\n+\treturn buffer.buf;\n+}\n+\n+char *git_prompt_askpass(const char *prompt)\n+{\n+\tconst char *askpass;\n+\tchar *r = NULL;\n+\n+\taskpass = getenv(\"GIT_ASKPASS\");\n+\tif (!askpass)\n+\t\taskpass = askpass_program;\n+\tif (!askpass)\n+\t\taskpass = getenv(\"SSH_ASKPASS\");\n+\tif (askpass && *askpass)\n+\t\tr = do_askpass(askpass, prompt);\n+\n+\treturn r;\n+}\n+\n+char *git_prompt_noecho(const char *prompt)\n+{\n+\treturn git_prompt(prompt, 0);\n+}\ndiff --git a/prompt-password.h b/prompt-password.h\nnew file mode 100644\nindex 00000000000..84933d2b7c5\n--- /dev/null\n+++ b/prompt-password.h\n@@ -0,0 +1,7 @@\n+#ifndef PROMPT_PASSWORD_H\n+#define PROMPT_PASSWORD_H\n+\n+char *git_prompt_askpass(const char *prompt);\n+char *git_prompt_noecho(const char *prompt);\n+\n+#endif\ndiff --git a/prompt.c b/prompt.c\nindex 5ded21a017f..458d6637506 100644\n--- a/prompt.c\n+++ b/prompt.c\n@@ -1,78 +1,27 @@\n #include \"cache.h\"\n #include \"config.h\"\n-#include \"run-command.h\"\n #include \"strbuf.h\"\n #include \"prompt.h\"\n #include \"compat/terminal.h\"\n \n-static char *do_askpass(const char *cmd, const char *prompt)\n+char *git_prompt(const char *prompt, unsigned int echo)\n {\n-\tstruct child_process pass = CHILD_PROCESS_INIT;\n-\tconst char *args[3];\n-\tstatic struct strbuf buffer = STRBUF_INIT;\n-\tint err = 0;\n-\n-\targs[0] = cmd;\n-\targs[1]\t= prompt;\n-\targs[2] = NULL;\n-\n-\tpass.argv = args;\n-\tpass.out = -1;\n-\n-\tif (start_command(&pass))\n-\t\treturn NULL;\n-\n-\tstrbuf_reset(&buffer);\n-\tif (strbuf_read(&buffer, pass.out, 20) < 0)\n-\t\terr = 1;\n-\n-\tclose(pass.out);\n-\n-\tif (finish_command(&pass))\n-\t\terr = 1;\n+\tchar *r = NULL;\n \n-\tif (err) {\n-\t\terror(\"unable to read askpass response from '%s'\", cmd);\n-\t\tstrbuf_release(&buffer);\n-\t\treturn NULL;\n+\tif (git_env_bool(\"GIT_TERMINAL_PROMPT\", 1)) {\n+\t\tr = git_terminal_prompt(prompt, echo);\n+\t\tif (!r)\n+\t\t\tdie_errno(\"could not read\");\n+\t} else {\n+\t\tdie(\"could not read terminal prompts disabled\");\n \t}\n \n-\tstrbuf_setlen(&buffer, strcspn(buffer.buf, \"\\r\\n\"));\n-\n-\treturn buffer.buf;\n+\treturn r;\n }\n \n-char *git_prompt(const char *prompt, int flags)\n+char *git_prompt_echo(const char *prompt)\n {\n-\tchar *r = NULL;\n-\n-\tif (flags & PROMPT_ASKPASS) {\n-\t\tconst char *askpass;\n-\n-\t\taskpass = getenv(\"GIT_ASKPASS\");\n-\t\tif (!askpass)\n-\t\t\taskpass = askpass_program;\n-\t\tif (!askpass)\n-\t\t\taskpass = getenv(\"SSH_ASKPASS\");\n-\t\tif (askpass && *askpass)\n-\t\t\tr = do_askpass(askpass, prompt);\n-\t}\n-\n-\tif (!r) {\n-\t\tconst char *err;\n-\n-\t\tif (git_env_bool(\"GIT_TERMINAL_PROMPT\", 1)) {\n-\t\t\tr = git_terminal_prompt(prompt, flags & PROMPT_ECHO);\n-\t\t\terr = strerror(errno);\n-\t\t} else {\n-\t\t\terr = \"terminal prompts disabled\";\n-\t\t}\n-\t\tif (!r) {\n-\t\t\t/* prompts already contain \": \" at the end */\n-\t\t\tdie(\"could not read %s%s\", prompt, err);\n-\t\t}\n-\t}\n-\treturn r;\n+\treturn git_prompt(prompt, 1);\n }\n \n int git_read_line_interactively(struct strbuf *line)\ndiff --git a/prompt.h b/prompt.h\nindex e294e93541c..f4d38178bc4 100644\n--- a/prompt.h\n+++ b/prompt.h\n@@ -1,11 +1,8 @@\n #ifndef PROMPT_H\n #define PROMPT_H\n \n-#define PROMPT_ASKPASS (1<<0)\n-#define PROMPT_ECHO    (1<<1)\n-\n-char *git_prompt(const char *prompt, int flags);\n-\n+char *git_prompt(const char *prompt, unsigned int echo);\n+char *git_prompt_echo(const char *prompt);\n int git_read_line_interactively(struct strbuf *line);\n \n #endif /* PROMPT_H */\n-- \n2.33.1.1570.g069344fdd45\n\n"},{"id":"440341","messageId":"patch-2.2-964e7f4531f-20211102T155046Z-avarab@gmail.com","threadId":"51129","inReplyTo":"cover-0.2-00000000000-20211102T155046Z-avarab@gmail.com","subject":"[PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-02T16:48:10Z","receivedAt":"2021-11-02T16:48:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20) we\nwe fixed a behavior change in the conversion of git-am from a\nshellscript to a C program by changing it from using git_prompt() to\nusing fgets(..., stdin). This ensured that we could run:\n\n    echo y | git am --interactive [...]\n\nBut along with that in the subsequent 6e7baf246a2 (am: drop tty\nrequirement for --interactive, 2019-05-20) we had to remove support\nfor:\n\n    git am --interactive </dev/null\n\nThis change builds on the refactoring of git_prompt() into \"normal\nprompt\" and \"wants password\" functions in the preceding commit, and\nmoves \"git am --interactive\" back to using the prompt function.\n\nThis allows us to have our cake and eat it too by adding a\nGIT_TERMINAL_PROMPT=true mode to test-lib.sh. Adjusting \"git am\n--interactive\" for use in our tests (see\ne.g. \"t/t4257-am-interactive.sh\") was what 97387c8bdd9 and 6e7baf246a2\nwere aiming for.\n\nThen more recently in 09535f056b0 (bisect--helper: reimplement\n`bisect_autostart` shell function in C, 2020-09-24) we've had the same\nsort of behavior change happen to \"git bisect\"'s interactive question\nmode, it now uses git_prompt()'s /dev/tty, not stdin.\n\nIt seems to me that using /dev/tty is desirable over using stdin,\nthese prompts are meant to be interactive, and our acceptance of stdin\nwas an artifact of how these commands were originally implemented in\nshellscript.\n\nSo let's move \"git am --interactive\" back to using\n\"git_prompt()\" (which is called \"git_prompt_echo()\" as of the\npreceding commit), and similarly remove the \"!isatty(STDIN_FILENO)\"\ntest added in 09535f056b0, that control flow was converted as-is from\nthe shellscript behavior.\n\nLet's also change a similar assertion added to \"git am\" in\n6e7baf246a2. Now we'll die on:\n\n    # no arguments provided\n    git am --interactive\n\nBut not:\n\n    git am --interactive </dev/null\n\nOr:\n\n    git am --interactive <mbox\n\nTo do this we'll need to add a GIT_TEST_TERMINAL_PROMPT variable for\nuse in test-lib.sh, by doing so this \"echo input | git cmd ...\"\nbehavior of interactive commands is now isolated to our own test\nsuite, instead of leaking out into the wild.\n\nNow that we've done that we can exhaustively test the prompt behavior\nof \"git bisect\", which wasn't previously possible.\n\nThere is some discussion downthread of the series 97387c8bdd9 is in\nabout whether we should always accept stdin input in these\ncommands[1]. I think that's arguably a good idea, and perhaps we'll\nneed to change the approach here.\n\nUsing a git_prompt_echo() that we know never needs to handle passwords\nshould provide us with an easy path towards deciding what to do in\nthose cases, we'll be able to consistently pick one behavior or the\nother, instead of having the behavior of specific commands cater to\ntest-only needs.\n\nThe lack of _() on the new die() message is intentional. This message\nwill only be emitted if there's a bug in our own test suite, so it's a\nwaste of translator time to translate it.\n\n1. https://lore.kernel.org/git/20190520125016.GA13474@sigill.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/am.c                |  8 +++-----\n builtin/bisect--helper.c    |  3 ---\n prompt.c                    |  8 +++++++-\n t/t6030-bisect-porcelain.sh | 41 +++++++++++++++++++++++++++++++++++++\n t/test-lib.sh               |  4 ++++\n 5 files changed, 55 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8677ea2348a..1e90b9ea0cd 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1693,7 +1693,7 @@ static int do_interactive(struct am_state *state)\n \tassert(state->msg);\n \n \tfor (;;) {\n-\t\tchar reply[64];\n+\t\tconst char *reply;\n \n \t\tputs(_(\"Commit Body is:\"));\n \t\tputs(\"--------------------------\");\n@@ -1705,9 +1705,7 @@ static int do_interactive(struct am_state *state)\n \t\t * in your translation. The program will only accept English\n \t\t * input at this point.\n \t\t */\n-\t\tprintf(_(\"Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: \"));\n-\t\tif (!fgets(reply, sizeof(reply), stdin))\n-\t\t\tdie(\"unable to read from stdin; aborting\");\n+\t\treply = git_prompt_echo(_(\"Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: \"));\n \n \t\tif (*reply == 'y' || *reply == 'Y') {\n \t\t\treturn 0;\n@@ -2437,7 +2435,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\t\tstrvec_push(&paths, mkpath(\"%s/%s\", prefix, argv[i]));\n \t\t}\n \n-\t\tif (state.interactive && !paths.nr)\n+\t\tif (state.interactive && !paths.nr && isatty(0))\n \t\t\tdie(_(\"interactive mode requires patches on the command line\"));\n \n \t\tam_setup(&state, patch_format, paths.v, keep_cr);\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 30533a70b53..dd73d76df3e 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -830,9 +830,6 @@ static int bisect_autostart(struct bisect_terms *terms)\n \tfprintf_ln(stderr, _(\"You need to start by \\\"git bisect \"\n \t\t\t  \"start\\\"\\n\"));\n \n-\tif (!isatty(STDIN_FILENO))\n-\t\treturn -1;\n-\n \t/*\n \t * TRANSLATORS: Make sure to include [Y] and [n] in your\n \t * translation. The program will only accept English input\ndiff --git a/prompt.c b/prompt.c\nindex 458d6637506..273bc30bf0e 100644\n--- a/prompt.c\n+++ b/prompt.c\n@@ -6,9 +6,15 @@\n \n char *git_prompt(const char *prompt, unsigned int echo)\n {\n+\tconst char *test_var = \"GIT_TEST_TERMINAL_PROMPT\";\n \tchar *r = NULL;\n \n-\tif (git_env_bool(\"GIT_TERMINAL_PROMPT\", 1)) {\n+\tif (git_env_bool(test_var, 0) && !isatty(0)) {\n+\t\tchar reply[64];\n+\t\tif (!fgets(reply, sizeof(reply), stdin))\n+\t\t\tdie(\"unable to read from stdin in '%s=true' mode\", test_var);\n+\t\treturn xstrdup(reply);\n+\t} else if (git_env_bool(\"GIT_TERMINAL_PROMPT\", 1)) {\n \t\tr = git_terminal_prompt(prompt, echo);\n \t\tif (!r)\n \t\t\tdie_errno(\"could not read\");\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 1be85d064e7..2afb1b57b45 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -45,6 +45,47 @@ test_expect_success 'set up basic repo with 1 file (hello) and 4 commits' '\n      HASH4=$(git rev-parse --verify HEAD)\n '\n \n+test_expect_success 'bisect \"good\" without a \"start\": no prompt' '\n+\tcat >expect <<-\\EOF &&\n+\tYou need to start by \"git bisect start\"\n+\n+\tfatal: unable to read from stdin in '\\''GIT_TEST_TERMINAL_PROMPT=true'\\'' mode\n+\tEOF\n+\ttest_expect_code 128 git bisect good HEAD 2>actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git bisect bad HEAD~ 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'bisect \"good\" without a \"start\": have prompt' '\n+\tcat >expect <<-\\EOF &&\n+\tYou need to start by \"git bisect start\"\n+\n+\tEOF\n+\techo n | test_expect_code 1 git bisect good HEAD 2>actual &&\n+\ttest_cmp expect actual &&\n+\techo n | test_must_fail git bisect bad HEAD~ 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'bisect \"good\" without a \"start\": answer prompt' '\n+\tcat >expect <<-\\EOF &&\n+\tYou need to start by \"git bisect start\"\n+\n+\tEOF\n+\techo Y | git bisect good HEAD 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# We will only get this far with the \"Y\" prompt\n+\tcat >expect <<-\\EOF &&\n+\tSome good revs are not ancestors of the bad rev.\n+\tgit bisect cannot work properly in this case.\n+\tMaybe you mistook good and bad revs?\n+\tEOF\n+\ttest_must_fail git bisect bad HEAD~ 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'bisect starts with only one bad' '\n \tgit bisect reset &&\n \tgit bisect start &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 2679a7596a6..778a08ffe4b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -476,6 +476,10 @@ export GIT_TEST_MERGE_ALGORITHM\n GIT_TRACE_BARE=1\n export GIT_TRACE_BARE\n \n+# Have git_prompt_noecho() accept stdin\n+GIT_TEST_TERMINAL_PROMPT=true\n+export GIT_TEST_TERMINAL_PROMPT\n+\n # Use specific version of the index file format\n if test -n \"${GIT_TEST_INDEX_VERSION:+isset}\"\n then\n-- \n2.33.1.1570.g069344fdd45\n\n"},{"id":"440371","messageId":"YYJ4HYyl1tBs2cZN@coredump.intra.peff.net","threadId":"51129","inReplyTo":"patch-1.2-ce5bea43f03-20211102T155046Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] prompt.c: split up the password and non-password handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-03T11:53:01Z","receivedAt":"2021-11-03T11:53:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 02, 2021 at 05:48:09PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Rather than pass PROMPT_ASKPASS and PROMPT_ECHO flags to git_prompt()\n> let's split it up into git_prompt_echo() and git_prompt_noecho()\n> functions, the latter only ever needs to be called from credential.c,\n> and the same goes for the previous password-only codepath in\n> git_prompt(), which is now exposed as a git_prompt_password().\n\nI'm pretty \"meh\" on this direction. Moving from a flag field to separate\nfunctions means a potential combinatoric explosion if new callers are\nadded later, or new flags are added.\n\nAnd callers get more awkward, as the choices are no longer expressed\nas data, so you need new conditionals like:\n\n> -\tr = git_prompt(prompt.buf, flags);\n> +\t/* We'll try \"askpass\" for both usernames and passwords */\n> +\tr = git_prompt_askpass(prompt.buf);\n> +\tif (!r)\n> +\t\tr = password ? git_prompt_noecho(prompt.buf)\n> +\t\t\t     : git_prompt_echo(prompt.buf);\n\nAnd we lose the human-readable names for the flags in favor of ad-hoc\nbooleans:\n\n> @@ -192,11 +197,9 @@ static char *credential_ask_one(const char *what, struct credential *c,\n>  static void credential_getpass(struct credential *c)\n>  {\n>  \tif (!c->username)\n> -\t\tc->username = credential_ask_one(\"Username\", c,\n> -\t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n> +\t\tc->username = credential_ask_one(c, 0);\n>  \tif (!c->password)\n> -\t\tc->password = credential_ask_one(\"Password\", c,\n> -\t\t\t\t\t\t PROMPT_ASKPASS);\n> +\t\tc->password = credential_ask_one(c, 1);\n\nThe only thing I do like here is that askpass can be split into its own\nfunction (as in the first hunk above), and callers can simply decide\nwhether they need to go on to prompt at the terminal. But even there it\nfeels like closing a potential door around PROMPT_ECHO. The standard\naskpass interface doesn't have a way for us to ask for it to show the\ncontents to the user (which is useful for the username prompt). But it\nwould be a reasonable extension to add one. Say, a GIT_ASKPASS_ECHO\nthat, if set, is preferred for the username prompt.\n\nSo I dunno. This seems somewhere between churn and making things worse,\nand I don't see what it's buying us at all.\n\n-Peff\n"},{"id":"440372","messageId":"YYJ5IpvGRoDvp8V6@coredump.intra.peff.net","threadId":"51129","inReplyTo":"patch-2.2-964e7f4531f-20211102T155046Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-03T11:57:22Z","receivedAt":"2021-11-03T11:57:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 02, 2021 at 05:48:10PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> In 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20) we\n> we fixed a behavior change in the conversion of git-am from a\n> shellscript to a C program by changing it from using git_prompt() to\n> using fgets(..., stdin). This ensured that we could run:\n> \n>     echo y | git am --interactive [...]\n> \n> But along with that in the subsequent 6e7baf246a2 (am: drop tty\n> requirement for --interactive, 2019-05-20) we had to remove support\n> for:\n> \n>     git am --interactive </dev/null\n> \n> This change builds on the refactoring of git_prompt() into \"normal\n> prompt\" and \"wants password\" functions in the preceding commit, and\n> moves \"git am --interactive\" back to using the prompt function.\n\nWhy do we want to do that? The only reason I mentioned that \"/dev/null\"\nthing in the earlier commit is that it's pointless.\n\nIMHO nothing should be using git_prompt() outside of the credential\ncode. They should just be reading from stdin, which is much more\nflexible. If a caller knows that stdin is coming from elsewhere, they\ncan redirect from /dev/tty.\n\n> It seems to me that using /dev/tty is desirable over using stdin,\n> these prompts are meant to be interactive, and our acceptance of stdin\n> was an artifact of how these commands were originally implemented in\n> shellscript.\n\nBasically, I think I just disagree with this paragraph entirely. Moving\nto stdin in the commits you referenced was done to help testing, but I\nalso think it's just a more flexible direction overall.\n\n-Peff\n"},{"id":"440378","messageId":"211103.864k8tjmmp.gmgdl@evledraar.gmail.com","threadId":"51129","inReplyTo":"YYJ5IpvGRoDvp8V6@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-03T15:12:43Z","receivedAt":"2021-11-03T15:16:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 03 2021, Jeff King wrote:\n\n> On Tue, Nov 02, 2021 at 05:48:10PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> In 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20) we\n>> we fixed a behavior change in the conversion of git-am from a\n>> shellscript to a C program by changing it from using git_prompt() to\n>> using fgets(..., stdin). This ensured that we could run:\n>> \n>>     echo y | git am --interactive [...]\n>> \n>> But along with that in the subsequent 6e7baf246a2 (am: drop tty\n>> requirement for --interactive, 2019-05-20) we had to remove support\n>> for:\n>> \n>>     git am --interactive </dev/null\n>> \n>> This change builds on the refactoring of git_prompt() into \"normal\n>> prompt\" and \"wants password\" functions in the preceding commit, and\n>> moves \"git am --interactive\" back to using the prompt function.\n>\n> Why do we want to do that? The only reason I mentioned that \"/dev/null\"\n> thing in the earlier commit is that it's pointless.\n>\n> IMHO nothing should be using git_prompt() outside of the credential\n> code. They should just be reading from stdin, which is much more\n> flexible. If a caller knows that stdin is coming from elsewhere, they\n> can redirect from /dev/tty.\n>\n>> It seems to me that using /dev/tty is desirable over using stdin,\n>> these prompts are meant to be interactive, and our acceptance of stdin\n>> was an artifact of how these commands were originally implemented in\n>> shellscript.\n>\n> Basically, I think I just disagree with this paragraph entirely. Moving\n> to stdin in the commits you referenced was done to help testing, but I\n> also think it's just a more flexible direction overall.\n\nI'm fine with it either way, my reading of your 2019-ish commits was\nthat you did that not to intentionally get that behavior, but to work\naround that test issue.\n\nSo we do always want the \"read from stdin\" behavior, so I can get those\nbisect tests by just changing its behavior too, with no need for the\ntest variable? If so I'm fine with that.\n\nI think it's a good thing in general to have a not-for-password\ngit_prompt() API, because it makes it easy to make that use some\nreadline-like API, i.e. one that would have tab completion, and handle\nthe loop some (but not all) callers have around handling retries etc,\nwe'd also be able to translate the \"Y\" \"n\" characters...\n"},{"id":"440388","messageId":"xmqqtugt6te2.fsf@gitster.g","threadId":"51129","inReplyTo":"YYJ4HYyl1tBs2cZN@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] prompt.c: split up the password and non-password handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-03T17:28:21Z","receivedAt":"2021-11-03T17:28:25Z","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> So I dunno. This seems somewhere between churn and making things worse,\n> and I don't see what it's buying us at all.\n\nUnfortunately I have to agree with the summary.  Not a material I\nwant to spend time on this time in the cycle anyway X-<.\n\nThanks for reading.\n"},{"id":"440389","messageId":"xmqqo8716sqx.fsf@gitster.g","threadId":"51129","inReplyTo":"YYJ5IpvGRoDvp8V6@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-03T17:42:14Z","receivedAt":"2021-11-03T17:42:18Z","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> Basically, I think I just disagree with this paragraph entirely. Moving\n> to stdin in the commits you referenced was done to help testing, but I\n> also think it's just a more flexible direction overall.\n\nIt is OK, and it is more convenient for writing test scripts, to\ntake interactive input from the standard input stream, if the\ncommand does not use the standard input for other purposes.\n\n\"git am -i <mbox\" cannot take prompted input via the standard input,\nbut \"git am -i mbox\" is an easy workaround, for example.\n\nCommands that are designed to be used in the downstream of a pipe\n(e.g. \"git rev-list ... | git pack-objects\") cannot easily use such\na workaround, so they may still need to open and interact with\n/dev/tty if they want to do an interactive input, though [*].\n\n[Footnote]\n\n* \"pack-objects\" is an excellent example of a command that takes its\n  primary input from the standard input, but is a horrible example\n  otherwise, because it probably would not make sense for it to take\n  any prompted input.\n"},{"id":"440434","messageId":"nycvar.QRO.7.76.6.2111040945280.56@tvgsbejvaqbjf.bet","threadId":"51129","inReplyTo":"xmqqo8716sqx.fsf@gitster.g","subject":"Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-04T08:48:35Z","receivedAt":"2021-11-04T08:48:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio & Peff,\n\nOn Wed, 3 Nov 2021, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n>\n> > Basically, I think I just disagree with this paragraph entirely.\n> > Moving to stdin in the commits you referenced was done to help\n> > testing, but I also think it's just a more flexible direction overall.\n>\n> It is OK, and it is more convenient for writing test scripts, to take\n> interactive input from the standard input stream, if the command does\n> not use the standard input for other purposes.\n\nI think I remember when we talked about this, it was in the context of\n`git add -p` becoming a built-in, and we all agreed that it is actually a\nvery nice side effect that you can feed commands to `git add -p` in\nscripts via stdin, not only for testing.\n\nIt might have been in the context of another command, but even then it is\na fact that this is a very nice side effect.\n\nCiao,\nDscho\n"},{"id":"440439","messageId":"YYOsHkMgUa7CXLic@coredump.intra.peff.net","threadId":"51129","inReplyTo":"nycvar.QRO.7.76.6.2111040945280.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-04T09:47:10Z","receivedAt":"2021-11-04T09:47:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 04, 2021 at 09:48:35AM +0100, Johannes Schindelin wrote:\n\n> Hi Junio & Peff,\n> \n> On Wed, 3 Nov 2021, Junio C Hamano wrote:\n> \n> > Jeff King <peff@peff.net> writes:\n> >\n> > > Basically, I think I just disagree with this paragraph entirely.\n> > > Moving to stdin in the commits you referenced was done to help\n> > > testing, but I also think it's just a more flexible direction overall.\n> >\n> > It is OK, and it is more convenient for writing test scripts, to take\n> > interactive input from the standard input stream, if the command does\n> > not use the standard input for other purposes.\n> \n> I think I remember when we talked about this, it was in the context of\n> `git add -p` becoming a built-in, and we all agreed that it is actually a\n> very nice side effect that you can feed commands to `git add -p` in\n> scripts via stdin, not only for testing.\n> \n> It might have been in the context of another command, but even then it is\n> a fact that this is a very nice side effect.\n\nYes, we definitely had that discussion about \"add -p\", and I agree it is\nnice. People are probably less likely to drive other tools like git-am\nand git-bisect in such a way, though, as their interactive modes just do\na lot less.\n\n-Peff\n"},{"id":"440440","messageId":"YYOttuoQT2X6HC/C@coredump.intra.peff.net","threadId":"51129","inReplyTo":"xmqqo8716sqx.fsf@gitster.g","subject":"Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-04T09:53:58Z","receivedAt":"2021-11-04T09:54:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 03, 2021 at 10:42:14AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Basically, I think I just disagree with this paragraph entirely. Moving\n> > to stdin in the commits you referenced was done to help testing, but I\n> > also think it's just a more flexible direction overall.\n> \n> It is OK, and it is more convenient for writing test scripts, to\n> take interactive input from the standard input stream, if the\n> command does not use the standard input for other purposes.\n> \n> \"git am -i <mbox\" cannot take prompted input via the standard input,\n> but \"git am -i mbox\" is an easy workaround, for example.\n> \n> Commands that are designed to be used in the downstream of a pipe\n> (e.g. \"git rev-list ... | git pack-objects\") cannot easily use such\n> a workaround, so they may still need to open and interact with\n> /dev/tty if they want to do an interactive input, though [*].\n\nTrue. The most Unix-y thing there would be to provide an option for\nreading interactive input from an arbitrary descriptor. That gives the\nmost flexibility, though it's probably a bit arcane for most folks to\ndo:\n\n  git foo | git bar --interactive-from=3 3</dev/tty\n\nWe could directly allow:\n\n  git foo | git bar --interactive-from=/dev/tty\n\nwhich is a bit less arcane. Or alternatively this could come from the\nenvironment, like:\n\n  export GIT_INTERACTIVE_FROM=/dev/tty\n  git foo | git bar --interactive\n\nWhich is equivalent-ish to having a boolean env variable to say \"read\nfrom the terminal\", except that it retains some more of the flexibility\n(especially if we treat a numeric value as a descriptor).\n\nOf course yet another option is to teach commands like pack-objects that\nread input only from stdin to accept a command-line option to read that\ninput from a file. Then stdin is free for interactive use. ;)\n\nBut I would not do any of that until we had a command that was a good\ncandidate. In the case of git-am and git-bisect, I think it's fine to\nassume that \"-i\" will use stdin.\n\n-Peff\n"},{"id":"440441","messageId":"YYOu2i48Br9sOz4V@coredump.intra.peff.net","threadId":"51129","inReplyTo":"211103.864k8tjmmp.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-04T09:58:50Z","receivedAt":"2021-11-04T09:58:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 03, 2021 at 04:12:43PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I'm fine with it either way, my reading of your 2019-ish commits was\n> that you did that not to intentionally get that behavior, but to work\n> around that test issue.\n> \n> So we do always want the \"read from stdin\" behavior, so I can get those\n> bisect tests by just changing its behavior too, with no need for the\n> test variable? If so I'm fine with that.\n\nYes. The thing that makes it OK to do from a backwards-compatibility\nstandpoint is that even though the tools read from /dev/tty now, they\nalso insist that stdin is a tty. So in practice it will always be the\nsame thing (technically you _could_ redirect stdin from a different tty,\nbut that's sufficiently bizarre that I think we can discount it).\n\n> I think it's a good thing in general to have a not-for-password\n> git_prompt() API, because it makes it easy to make that use some\n> readline-like API, i.e. one that would have tab completion, and handle\n> the loop some (but not all) callers have around handling retries etc,\n> we'd also be able to translate the \"Y\" \"n\" characters...\n\nYeah, I'd be OK with that direction, but I think it is not really the\nsame thing that the existing git_prompt() was designed for (where you\nmost certainly don't want readline-ish things like history for your\npassword).\n\nPerhaps a good first step is not so much splitting git_prompt() into two\npieces, as simply renaming it in place. It really was designed for the\ncredential request and not much else, but the name is a bit more\ngeneric.\n\nBut I'd probably wait on that until we were looking at actually adding a\nfunctional readline-ish interface before even doing that.\n\n-Peff\n"}]}