* Abort (core dumped) @ 2019-05-20 8:35 Alejandro Sanchez 2019-05-20 10:02 ` Jeff King 0 siblings, 1 reply; 30+ messages in thread From: Alejandro Sanchez @ 2019-05-20 8:35 UTC (permalink / raw) To: git A core dump occurred while trying to interactively apply (3-way) a series of patches from a mailbox. git version 2.20.1 Steps to reproduce: alex@polaris:~/slurm/source$ git am -3 -i ~/Downloads/6033-18.08-final-patchset-v2.patch Commit Body is: -------------------------- Use correct signed/unsiged types. Change a few variables in archiving to use the correct signed or unsigned type to avoid implicit casting. Bug 6033. -------------------------- Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y Applying: Use correct signed/unsiged types. Commit Body is: -------------------------- Remove unused static variable high_buffer_size. It was set but never read. Bug 6033. -------------------------- Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y Applying: Remove unused static variable high_buffer_size. Commit Body is: -------------------------- Handle duplicate archive file names. The time period of the archive file currently depends on submit or start time and whether the purge period is in hours, days, or months. Previously, if the archive file name already exists, we would overwrite the old archive file with the assumption that these are duplicate records being archived after an archive load. However, that could result in lost records in a couple of ways: * If there were runaway jobs that were part of an old archive file's time period and are later fixed and then purged, the old file would be overwritten. * If jobs or steps are purged but there are still jobs or steps in that time period that are pending or running, the pending or running jobs and steps won't be purged. When they finish and are purged, the old file would be overwritten. Instead of overwriting the old file, we append a number to the file name to create a new file. This will also be important in an upcoming commit. Bug 6033. -------------------------- Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: y Applying: Handle duplicate archive file names. Using index info to reconstruct a base tree... M NEWS Falling back to patching base and 3-way merge... Auto-merging NEWS CONFLICT (content): Merge conflict in NEWS error: Failed to merge in the changes. Patch failed at 0003 Handle duplicate archive file names. hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am -i --continue". If you prefer to skip this patch, run "git am -i --skip" instead. To restore the original branch and stop patching, run "git am -i --abort". alex@polaris:~/slurm/source$ vi NEWS alex@polaris:~/slurm/source$ man git add alex@polaris:~/slurm/source$ git add -u alex@polaris:~/slurm/source$ git am -i --continue Applying: Handle duplicate archive file names. error: object 861d3c6f689a3ca5eb5fb5c409d46de0ad5555e1 is a commit, not a tree BUG: diff-lib.c:526: run_diff_index must be passed exactly one tree Aborted (core dumped) alex@polaris:~/slurm/source$ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Abort (core dumped) 2019-05-20 8:35 Abort (core dumped) Alejandro Sanchez @ 2019-05-20 10:02 ` Jeff King 2019-05-20 12:06 ` [PATCH 0/4] fix BUG() with "git am -i --resolved" Jeff King 0 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2019-05-20 10:02 UTC (permalink / raw) To: Alejandro Sanchez; +Cc: git On Mon, May 20, 2019 at 10:35:53AM +0200, Alejandro Sanchez wrote: > alex@polaris:~/slurm/source$ git add -u > alex@polaris:~/slurm/source$ git am -i --continue > Applying: Handle duplicate archive file names. > error: object 861d3c6f689a3ca5eb5fb5c409d46de0ad5555e1 is a commit, not a tree > BUG: diff-lib.c:526: run_diff_index must be passed exactly one tree > Aborted (core dumped) Hmm. So I think the interesting error is probably that first line: some code expects to look up a tree but sees a commit, and then as a result it probably feeds too few items to run_diff_index(), triggering the assertion failure. Just grepping around, this looks quite suspicious: $ git grep -hW get_oid_tree builtin/am.c static void write_index_patch(const struct am_state *state) ... if (!get_oid_tree("HEAD", &head)) tree = lookup_tree(the_repository, &head); else tree = lookup_tree(the_repository, the_repository->hash_algo->empty_tree); Using get_oid_tree() does not actually return a tree; it just prioritizes trees when disambiguating names (which is pointless here, since we're not feeding an ambiguous oid). HEAD will always be a commit, and then lookup_tree() similarly does not peel that down to an actual tree. And this whole function is called only in interactive-mode, so it's possible that it's simply not used much and nobody noticed. I haven't tried to reproduce yet. Is the repository (and patch) that you used to demonstrate this publicly available? Or alternatively, is it possible to show a backtrace from the coredump? If my blind guess is right, then something like this probably fixes it: diff --git a/builtin/am.c b/builtin/am.c index bdd1bbc35d..93305560c1 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1339,9 +1339,17 @@ static void write_index_patch(const struct am_state *state) struct rev_info rev_info; FILE *fp; - if (!get_oid_tree("HEAD", &head)) - tree = lookup_tree(the_repository, &head); - else + if (!get_oid("HEAD", &head)) { + struct object *obj; + struct commit *commit; + + obj = parse_object_or_die(&head, NULL); + commit = object_as_type(the_repository, obj, OBJ_COMMIT, 0); + if (!commit) + die("unable to parse HEAD as a commit"); + + tree = get_commit_tree(commit); + } else tree = lookup_tree(the_repository, the_repository->hash_algo->empty_tree); -Peff ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 0/4] fix BUG() with "git am -i --resolved" 2019-05-20 10:02 ` Jeff King @ 2019-05-20 12:06 ` Jeff King 2019-05-20 12:07 ` [PATCH 1/4] am: simplify prompt response handling Jeff King ` (3 more replies) 0 siblings, 4 replies; 30+ messages in thread From: Jeff King @ 2019-05-20 12:06 UTC (permalink / raw) To: Alejandro Sanchez; +Cc: git On Mon, May 20, 2019 at 06:02:11AM -0400, Jeff King wrote: > I haven't tried to reproduce yet. Is the repository (and patch) that you > used to demonstrate this publicly available? Or alternatively, is it > possible to show a backtrace from the coredump? Never mind. It reproduces quite easily, because AFAICT "am -i --resolved" has been totally broken since it was converted to C (in 2015!). I guess it does not have many users. :) > If my blind guess is right, then something like this probably fixes it: This is indeed the problem. Here are some patches. The actual fix is what I showed already. The other three are just making it possible to cover this with the test suite (until now, we had no coverage of "am --interactive" at all). [1/4]: am: simplify prompt response handling [2/4]: am: read interactive input from stdin [3/4]: am: drop tty requirement for --interactive [4/4]: am: fix --interactive HEAD tree resolution builtin/am.c | 30 +++++++++++++++--------- t/t4257-am-interactive.sh | 49 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 11 deletions(-) create mode 100755 t/t4257-am-interactive.sh -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/4] am: simplify prompt response handling 2019-05-20 12:06 ` [PATCH 0/4] fix BUG() with "git am -i --resolved" Jeff King @ 2019-05-20 12:07 ` Jeff King 2019-05-20 12:09 ` [PATCH 2/4] am: read interactive input from stdin Jeff King ` (2 subsequent siblings) 3 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2019-05-20 12:07 UTC (permalink / raw) To: Alejandro Sanchez; +Cc: git We'll never see a NULL returned from git_prompt(); if it can't produce any input for us (e.g., because the terminal got EOF) then it will just die(). So there's no need for us to handle NULL here. And even if there was, it doesn't make sense to continue; on a true terminal hangup we'd just loop infinitely trying to get input that will never come. Signed-off-by: Jeff King <peff@peff.net> --- builtin/am.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 912d9821b1..644bb11f6c 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1661,9 +1661,7 @@ static int do_interactive(struct am_state *state) */ reply = git_prompt(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: "), PROMPT_ECHO); - if (!reply) { - continue; - } else if (*reply == 'y' || *reply == 'Y') { + if (*reply == 'y' || *reply == 'Y') { return 0; } else if (*reply == 'a' || *reply == 'A') { state->interactive = 0; -- 2.22.0.rc1.539.g7bfcdfe86d ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/4] am: read interactive input from stdin 2019-05-20 12:06 ` [PATCH 0/4] fix BUG() with "git am -i --resolved" Jeff King 2019-05-20 12:07 ` [PATCH 1/4] am: simplify prompt response handling Jeff King @ 2019-05-20 12:09 ` Jeff King 2019-05-20 12:11 ` [PATCH 3/4] am: drop tty requirement for --interactive Jeff King 2019-05-20 12:13 ` [PATCH 4/4] am: fix --interactive HEAD tree resolution Jeff King 3 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2019-05-20 12:09 UTC (permalink / raw) To: Alejandro Sanchez; +Cc: git In the conversion of git-am from shell script to C, we switched to using git_prompt(). Unlike the original shell command "read reply", this doesn't read from stdin at all, but rather from /dev/tty. In most cases this distinction wouldn't matter. We require (as the shell script did) that stdin is a tty, so they would generally be the same thing. But one important exception is our test suite: even with test_terminal, we cannot test "am --interactive" because it insists on reading from /dev/tty, not the pseudo-tty we've set up in the test script. Fixing this clears the way to adding tests in a future patch. Signed-off-by: Jeff King <peff@peff.net> --- Part of me dies inside when I look at adding the magical "64". But we expect this input to be single-line (or at most, somebody might actually type out "edit", etc), and avoiding strbuf_getline() saves us from having to free the strbuf in the many early return paths. I dunno. If it's too ugly it would not be too hard to switch it over. builtin/am.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 644bb11f6c..47ad7a5a70 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1647,7 +1647,7 @@ static int do_interactive(struct am_state *state) die(_("cannot be interactive without stdin connected to a terminal.")); for (;;) { - const char *reply; + char reply[64]; puts(_("Commit Body is:")); puts("--------------------------"); @@ -1659,7 +1659,9 @@ static int do_interactive(struct am_state *state) * in your translation. The program will only accept English * input at this point. */ - reply = git_prompt(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: "), PROMPT_ECHO); + printf(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: ")); + if (!fgets(reply, sizeof(reply), stdin)) + die("unable to read from stdin; aborting"); if (*reply == 'y' || *reply == 'Y') { return 0; -- 2.22.0.rc1.539.g7bfcdfe86d ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/4] am: drop tty requirement for --interactive 2019-05-20 12:06 ` [PATCH 0/4] fix BUG() with "git am -i --resolved" Jeff King 2019-05-20 12:07 ` [PATCH 1/4] am: simplify prompt response handling Jeff King 2019-05-20 12:09 ` [PATCH 2/4] am: read interactive input from stdin Jeff King @ 2019-05-20 12:11 ` Jeff King 2019-05-20 12:50 ` Jeff King 2019-05-20 12:13 ` [PATCH 4/4] am: fix --interactive HEAD tree resolution Jeff King 3 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2019-05-20 12:11 UTC (permalink / raw) To: Alejandro Sanchez; +Cc: git We have required that the stdin of "am --interactive" be a tty since a1451104ac (git-am: interactive should fail gracefully., 2005-10-12). However, this isn't strictly necessary, and makes the tool harder to test (and is unlike all of our other --interactive commands). The goal of that commit was to make sure that somebody does not do: git am --interactive <mbox and cause us to read commands from the mbox. But we can simply check up front for this case and complain before entering the interactive loop. Technically this disallows: git am --interactive </dev/null where our lack of patches means we would never prompt for anything, and so the old code would not notice our lack of tty (and now we'd die early). But since such a command is totally pointless, it's no loss. Signed-off-by: Jeff King <peff@peff.net> --- builtin/am.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 47ad7a5a70..ea16b844f1 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1643,9 +1643,6 @@ static int do_interactive(struct am_state *state) { assert(state->msg); - if (!isatty(0)) - die(_("cannot be interactive without stdin connected to a terminal.")); - for (;;) { char reply[64]; @@ -2334,6 +2331,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) argv_array_push(&paths, mkpath("%s/%s", prefix, argv[i])); } + if (state.interactive && !paths.argc) + die(_("interactive mode requires patches on the command line")); + am_setup(&state, patch_format, paths.argv, keep_cr); argv_array_clear(&paths); -- 2.22.0.rc1.539.g7bfcdfe86d ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] am: drop tty requirement for --interactive 2019-05-20 12:11 ` [PATCH 3/4] am: drop tty requirement for --interactive Jeff King @ 2019-05-20 12:50 ` Jeff King 2019-05-23 6:44 ` Johannes Schindelin 0 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2019-05-20 12:50 UTC (permalink / raw) To: Alejandro Sanchez; +Cc: git On Mon, May 20, 2019 at 08:11:13AM -0400, Jeff King wrote: > We have required that the stdin of "am --interactive" be a tty since > a1451104ac (git-am: interactive should fail gracefully., 2005-10-12). > However, this isn't strictly necessary, and makes the tool harder to > test (and is unlike all of our other --interactive commands). I think this is worth doing for simplicity and consistency. But as you might guess, my ulterior motive was making it easier to add tests. In theory we _should_ be able to use test_terminal for this, but it seems to be racy, because it will quickly read all input and close the descriptor (to give the reader EOF). But after that close, isatty() will no longer report it correctly. E.g., if I run this: perl test-terminal.perl sh -c ' for i in 0 1 2; do echo $i is $(test -t $i || echo not) a tty done ' </dev/null it _usually_ says "0 is a tty", but racily may say "not a tty". If you put a sleep into the beginning of the shell, then it will basically always lose the race and say "not". It might be possible to overcome this by making test-terminal more clever (i.e., is there a way for us to send an "EOF" over the pty without actually _closing_ it? That would behave like a real terminal, where you can hit ^D to generate an EOF but then type more). But barring that, this works by just avoiding it entirely. :) Curiously, my script above also reports consistently that stdout is not a tty, but that stderr is. I'm not sure why this is, but it no tests seem to care either way. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] am: drop tty requirement for --interactive 2019-05-20 12:50 ` Jeff King @ 2019-05-23 6:44 ` Johannes Schindelin 2019-05-24 6:27 ` Jeff King 0 siblings, 1 reply; 30+ messages in thread From: Johannes Schindelin @ 2019-05-23 6:44 UTC (permalink / raw) To: Jeff King; +Cc: Alejandro Sanchez, git Hi Peff, On Mon, 20 May 2019, Jeff King wrote: > On Mon, May 20, 2019 at 08:11:13AM -0400, Jeff King wrote: > > > We have required that the stdin of "am --interactive" be a tty since > > a1451104ac (git-am: interactive should fail gracefully., 2005-10-12). > > However, this isn't strictly necessary, and makes the tool harder to > > test (and is unlike all of our other --interactive commands). > > I think this is worth doing for simplicity and consistency. But as you > might guess, my ulterior motive was making it easier to add tests. > > In theory we _should_ be able to use test_terminal for this, but it > seems to be racy, because it will quickly read all input and close the > descriptor (to give the reader EOF). But after that close, isatty() will > no longer report it correctly. E.g., if I run this: > > perl test-terminal.perl sh -c ' > for i in 0 1 2; do > echo $i is $(test -t $i || echo not) a tty > done > ' </dev/null > > it _usually_ says "0 is a tty", but racily may say "not a tty". If you > put a sleep into the beginning of the shell, then it will basically > always lose the race and say "not". This is just another nail in the coffin for `test-terminal.perl`, as far as I am concerned. In the built-in `add -i` patch series, I followed a strategy where I move totally away from `test-terminal`, in favor of using some knobs to force Git into thinking that we are in a terminal. But at the same time, I *also* remove the limitation (for most cases) of "read from /dev/tty", in favor of reading from stdin, and making things testable, and more importantly: scriptable. So I am *very* much in favor of this here patch. Thanks, Dscho P.S.: There are even more reasons to get rid of `test-terminal`, of course: it is an unnecessary dependency on Perl, works only when certain Perl modules are installed (that are *not* installed on Ubuntu by default, for example), and it requires pseudo terminals, so it will *never* work on Windows. > It might be possible to overcome this by making test-terminal more > clever (i.e., is there a way for us to send an "EOF" over the pty > without actually _closing_ it? That would behave like a real terminal, > where you can hit ^D to generate an EOF but then type more). > > But barring that, this works by just avoiding it entirely. :) > > Curiously, my script above also reports consistently that stdout is not > a tty, but that stderr is. I'm not sure why this is, but it no tests > seem to care either way. > > -Peff > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] am: drop tty requirement for --interactive 2019-05-23 6:44 ` Johannes Schindelin @ 2019-05-24 6:27 ` Jeff King 2021-11-02 16:48 ` [PATCH 0/2] prompt.c: split up git_prompt(), read from /dev/tty, not STDIN Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2019-05-24 6:27 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Alejandro Sanchez, git On Thu, May 23, 2019 at 08:44:31AM +0200, Johannes Schindelin wrote: > > perl test-terminal.perl sh -c ' > > for i in 0 1 2; do > > echo $i is $(test -t $i || echo not) a tty > > done > > ' </dev/null > > > > it _usually_ says "0 is a tty", but racily may say "not a tty". If you > > put a sleep into the beginning of the shell, then it will basically > > always lose the race and say "not". > > This is just another nail in the coffin for `test-terminal.perl`, as far > as I am concerned. I think it's only broken for stdin, but yeah, it's not great. I think the fact that test-terminal is not available everywhere (and thus many people are skipping a bunch of tests) is much more damning. :) > In the built-in `add -i` patch series, I followed a strategy where I move > totally away from `test-terminal`, in favor of using some knobs to force > Git into thinking that we are in a terminal. I'm in favor of this. The current "add -i" is pretty accepting of reading from stdin, and I think we can do that in most places. The main use of test_terminal has been to check color and progress decisions. I'd be just as happy to see something like this: int git_isatty(int fd) { static int override[3]; static int initialized; if (!initialized) { const char *x = getenv("GIT_PRETEND_TTY"); if (x) { for (; *x; x++) { int n = *x - '0'; if (n > 0 && n < ARRAY_SIZE(override) override[n] = 1; } } initialized = 1; } if (fd > 0 && fd < ARRAY_SIZE(override) && override[fd]) return 1; return isatty(fd); } > But at the same time, I *also* remove the limitation (for most cases) of > "read from /dev/tty", in favor of reading from stdin, and making things > testable, and more importantly: scriptable. As far as I know, apart from this git-am fix, the only thing that reads from the terminal is the credential prompt. That one has to be a bit picky, because: - we need to prompt from processes which have no stdio connected to the user (e.g., remote-curl). - we need to put the terminal into no-echo mode for passwords (and probably should bail if that fails, to be paranoid) In the case of credentials we already have multiple mechanisms for scripting the input (credential helpers and askpass). It would be nice to be able to test the terminal-level code automatically, but I'm just not sure how that would work. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 0/2] prompt.c: split up git_prompt(), read from /dev/tty, not STDIN 2019-05-24 6:27 ` Jeff King @ 2021-11-02 16:48 ` Ævar Arnfjörð Bjarmason 2021-11-02 16:48 ` [PATCH 1/2] prompt.c: split up the password and non-password handling Ævar Arnfjörð Bjarmason 2021-11-02 16:48 ` [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 30+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-11-02 16:48 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Alejandro Sanchez, Ævar Arnfjörð Bjarmason I had a stupid bug in another series of mine[1] breaking code around git_prompt() in bisect--helper.c, and in trying to follow-up to add testing to that discovered that doing so was rather hard, due to how git_prompt() behaves. In another case in 2019 (which is the In-Reply-To here) we added a seeming for-the-tests workaround for the same issue in "git am --interacive". This starts out by splitting the more complex "get a password interactively" case out of prompt.c into a new prompt-password.c, most of prompt.c's complexity was to cater to one API user in credential.c. That being done move these callers to using git_prompt(), and have it understanda a new GIT_TEST_TERMINAL_PROMPT=true variable so we can have our cake and it it too in the test suite. If that's set the prompt function will allow input on stdin. This then allowed my to add the missing test for "git bisect" for the code I'd accidentally broken in [1]. As noted in the discussion in 2/2 I'm not 100% sure this is the right direction in terms of swiching us back to using /dev/tty and not stdin, but see that commit & commit message for details. In any case, whatever behavior we go for as far as the UX goes shouldn't be catering only to the test suite at the cost of trade-offs elsewhere. 1. https://lore.kernel.org/git/cover-v6-0.8-00000000000-20211102T122507Z-avarab@gmail.com/ Ævar Arnfjörð Bjarmason (2): prompt.c: split up the password and non-password handling prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true Makefile | 1 + builtin/am.c | 8 ++-- builtin/bisect--helper.c | 8 +--- credential.c | 17 ++++---- help.c | 2 +- prompt-password.c | 63 +++++++++++++++++++++++++++++ prompt-password.h | 7 ++++ prompt.c | 79 ++++++++----------------------------- prompt.h | 7 +--- t/t6030-bisect-porcelain.sh | 41 +++++++++++++++++++ t/test-lib.sh | 4 ++ 11 files changed, 151 insertions(+), 86 deletions(-) create mode 100644 prompt-password.c create mode 100644 prompt-password.h -- 2.33.1.1570.g069344fdd45 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] prompt.c: split up the password and non-password handling 2021-11-02 16:48 ` [PATCH 0/2] prompt.c: split up git_prompt(), read from /dev/tty, not STDIN Ævar Arnfjörð Bjarmason @ 2021-11-02 16:48 ` Ævar Arnfjörð Bjarmason 2021-11-03 11:53 ` Jeff King 2021-11-02 16:48 ` [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 30+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-11-02 16:48 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Alejandro Sanchez, Ævar Arnfjörð Bjarmason Rather than pass PROMPT_ASKPASS and PROMPT_ECHO flags to git_prompt() let's split it up into git_prompt_echo() and git_prompt_noecho() functions, the latter only ever needs to be called from credential.c, and the same goes for the previous password-only codepath in git_prompt(), which is now exposed as a git_prompt_password(). Doing this paves the way towards addressing some of the issues brought up in 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 1 + builtin/bisect--helper.c | 5 ++- credential.c | 17 ++++++---- help.c | 2 +- prompt-password.c | 63 ++++++++++++++++++++++++++++++++++ prompt-password.h | 7 ++++ prompt.c | 73 ++++++---------------------------------- prompt.h | 7 ++-- 8 files changed, 97 insertions(+), 78 deletions(-) create mode 100644 prompt-password.c create mode 100644 prompt-password.h diff --git a/Makefile b/Makefile index 12be39ac497..e8815860e22 100644 --- a/Makefile +++ b/Makefile @@ -967,6 +967,7 @@ LIB_OBJS += pretty.o LIB_OBJS += prio-queue.o LIB_OBJS += progress.o LIB_OBJS += promisor-remote.o +LIB_OBJS += prompt-password.o LIB_OBJS += prompt.o LIB_OBJS += protocol.o LIB_OBJS += protocol-caps.o diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 28a2e6a5750..30533a70b53 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -370,7 +370,7 @@ static int decide_next(const struct bisect_terms *terms, * translation. The program will only accept English input * at this point. */ - yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO); + yesno = git_prompt_echo(_("Are you sure [Y/n]? ")); if (starts_with(yesno, "N") || starts_with(yesno, "n")) return -1; return 0; @@ -838,8 +838,7 @@ static int bisect_autostart(struct bisect_terms *terms) * translation. The program will only accept English input * at this point. */ - yesno = git_prompt(_("Do you want me to do it for you " - "[Y/n]? "), PROMPT_ECHO); + yesno = git_prompt_echo(_("Do you want me to do it for you [Y/n]? ")); res = tolower(*yesno) == 'n' ? -1 : bisect_start(terms, empty_strvec, 0); diff --git a/credential.c b/credential.c index e7240f3f636..5509de382a5 100644 --- a/credential.c +++ b/credential.c @@ -5,6 +5,7 @@ #include "run-command.h" #include "url.h" #include "prompt.h" +#include "prompt-password.h" #include "sigchain.h" #include "urlmatch.h" @@ -169,11 +170,11 @@ static void credential_format(struct credential *c, struct strbuf *out) } } -static char *credential_ask_one(const char *what, struct credential *c, - int flags) +static char *credential_ask_one(struct credential *c, unsigned int password) { struct strbuf desc = STRBUF_INIT; struct strbuf prompt = STRBUF_INIT; + const char *what = password ? "Password" : "Username"; char *r; credential_describe(c, &desc); @@ -182,7 +183,11 @@ static char *credential_ask_one(const char *what, struct credential *c, else strbuf_addf(&prompt, "%s: ", what); - r = git_prompt(prompt.buf, flags); + /* We'll try "askpass" for both usernames and passwords */ + r = git_prompt_askpass(prompt.buf); + if (!r) + r = password ? git_prompt_noecho(prompt.buf) + : git_prompt_echo(prompt.buf); strbuf_release(&desc); strbuf_release(&prompt); @@ -192,11 +197,9 @@ static char *credential_ask_one(const char *what, struct credential *c, static void credential_getpass(struct credential *c) { if (!c->username) - c->username = credential_ask_one("Username", c, - PROMPT_ASKPASS|PROMPT_ECHO); + c->username = credential_ask_one(c, 0); if (!c->password) - c->password = credential_ask_one("Password", c, - PROMPT_ASKPASS); + c->password = credential_ask_one(c, 1); } int credential_read(struct credential *c, FILE *fp) diff --git a/help.c b/help.c index 973e47cdc30..6eebc26fbeb 100644 --- a/help.c +++ b/help.c @@ -644,7 +644,7 @@ const char *help_unknown_cmd(const char *cmd) char *answer; struct strbuf msg = STRBUF_INIT; strbuf_addf(&msg, _("Run '%s' instead? (y/N)"), assumed); - answer = git_prompt(msg.buf, PROMPT_ECHO); + answer = git_prompt_echo(msg.buf); strbuf_release(&msg); if (!(starts_with(answer, "y") || starts_with(answer, "Y"))) diff --git a/prompt-password.c b/prompt-password.c new file mode 100644 index 00000000000..5fa00363d6c --- /dev/null +++ b/prompt-password.c @@ -0,0 +1,63 @@ +#include "cache.h" +#include "prompt-password.h" +#include "strbuf.h" +#include "run-command.h" +#include "prompt.h" + +static char *do_askpass(const char *cmd, const char *prompt) +{ + struct child_process pass = CHILD_PROCESS_INIT; + const char *args[3]; + static struct strbuf buffer = STRBUF_INIT; + int err = 0; + + args[0] = cmd; + args[1] = prompt; + args[2] = NULL; + + pass.argv = args; + pass.out = -1; + + if (start_command(&pass)) + return NULL; + + strbuf_reset(&buffer); + if (strbuf_read(&buffer, pass.out, 20) < 0) + err = 1; + + close(pass.out); + + if (finish_command(&pass)) + err = 1; + + if (err) { + error("unable to read askpass response from '%s'", cmd); + strbuf_release(&buffer); + return NULL; + } + + strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n")); + + return buffer.buf; +} + +char *git_prompt_askpass(const char *prompt) +{ + const char *askpass; + char *r = NULL; + + askpass = getenv("GIT_ASKPASS"); + if (!askpass) + askpass = askpass_program; + if (!askpass) + askpass = getenv("SSH_ASKPASS"); + if (askpass && *askpass) + r = do_askpass(askpass, prompt); + + return r; +} + +char *git_prompt_noecho(const char *prompt) +{ + return git_prompt(prompt, 0); +} diff --git a/prompt-password.h b/prompt-password.h new file mode 100644 index 00000000000..84933d2b7c5 --- /dev/null +++ b/prompt-password.h @@ -0,0 +1,7 @@ +#ifndef PROMPT_PASSWORD_H +#define PROMPT_PASSWORD_H + +char *git_prompt_askpass(const char *prompt); +char *git_prompt_noecho(const char *prompt); + +#endif diff --git a/prompt.c b/prompt.c index 5ded21a017f..458d6637506 100644 --- a/prompt.c +++ b/prompt.c @@ -1,78 +1,27 @@ #include "cache.h" #include "config.h" -#include "run-command.h" #include "strbuf.h" #include "prompt.h" #include "compat/terminal.h" -static char *do_askpass(const char *cmd, const char *prompt) +char *git_prompt(const char *prompt, unsigned int echo) { - struct child_process pass = CHILD_PROCESS_INIT; - const char *args[3]; - static struct strbuf buffer = STRBUF_INIT; - int err = 0; - - args[0] = cmd; - args[1] = prompt; - args[2] = NULL; - - pass.argv = args; - pass.out = -1; - - if (start_command(&pass)) - return NULL; - - strbuf_reset(&buffer); - if (strbuf_read(&buffer, pass.out, 20) < 0) - err = 1; - - close(pass.out); - - if (finish_command(&pass)) - err = 1; + char *r = NULL; - if (err) { - error("unable to read askpass response from '%s'", cmd); - strbuf_release(&buffer); - return NULL; + if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) { + r = git_terminal_prompt(prompt, echo); + if (!r) + die_errno("could not read"); + } else { + die("could not read terminal prompts disabled"); } - strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n")); - - return buffer.buf; + return r; } -char *git_prompt(const char *prompt, int flags) +char *git_prompt_echo(const char *prompt) { - char *r = NULL; - - if (flags & PROMPT_ASKPASS) { - const char *askpass; - - askpass = getenv("GIT_ASKPASS"); - if (!askpass) - askpass = askpass_program; - if (!askpass) - askpass = getenv("SSH_ASKPASS"); - if (askpass && *askpass) - r = do_askpass(askpass, prompt); - } - - if (!r) { - const char *err; - - if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) { - r = git_terminal_prompt(prompt, flags & PROMPT_ECHO); - err = strerror(errno); - } else { - err = "terminal prompts disabled"; - } - if (!r) { - /* prompts already contain ": " at the end */ - die("could not read %s%s", prompt, err); - } - } - return r; + return git_prompt(prompt, 1); } int git_read_line_interactively(struct strbuf *line) diff --git a/prompt.h b/prompt.h index e294e93541c..f4d38178bc4 100644 --- a/prompt.h +++ b/prompt.h @@ -1,11 +1,8 @@ #ifndef PROMPT_H #define PROMPT_H -#define PROMPT_ASKPASS (1<<0) -#define PROMPT_ECHO (1<<1) - -char *git_prompt(const char *prompt, int flags); - +char *git_prompt(const char *prompt, unsigned int echo); +char *git_prompt_echo(const char *prompt); int git_read_line_interactively(struct strbuf *line); #endif /* PROMPT_H */ -- 2.33.1.1570.g069344fdd45 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] prompt.c: split up the password and non-password handling 2021-11-02 16:48 ` [PATCH 1/2] prompt.c: split up the password and non-password handling Ævar Arnfjörð Bjarmason @ 2021-11-03 11:53 ` Jeff King 2021-11-03 17:28 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2021-11-03 11:53 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Johannes Schindelin, Alejandro Sanchez On Tue, Nov 02, 2021 at 05:48:09PM +0100, Ævar Arnfjörð Bjarmason wrote: > Rather than pass PROMPT_ASKPASS and PROMPT_ECHO flags to git_prompt() > let's split it up into git_prompt_echo() and git_prompt_noecho() > functions, the latter only ever needs to be called from credential.c, > and the same goes for the previous password-only codepath in > git_prompt(), which is now exposed as a git_prompt_password(). I'm pretty "meh" on this direction. Moving from a flag field to separate functions means a potential combinatoric explosion if new callers are added later, or new flags are added. And callers get more awkward, as the choices are no longer expressed as data, so you need new conditionals like: > - r = git_prompt(prompt.buf, flags); > + /* We'll try "askpass" for both usernames and passwords */ > + r = git_prompt_askpass(prompt.buf); > + if (!r) > + r = password ? git_prompt_noecho(prompt.buf) > + : git_prompt_echo(prompt.buf); And we lose the human-readable names for the flags in favor of ad-hoc booleans: > @@ -192,11 +197,9 @@ static char *credential_ask_one(const char *what, struct credential *c, > static void credential_getpass(struct credential *c) > { > if (!c->username) > - c->username = credential_ask_one("Username", c, > - PROMPT_ASKPASS|PROMPT_ECHO); > + c->username = credential_ask_one(c, 0); > if (!c->password) > - c->password = credential_ask_one("Password", c, > - PROMPT_ASKPASS); > + c->password = credential_ask_one(c, 1); The only thing I do like here is that askpass can be split into its own function (as in the first hunk above), and callers can simply decide whether they need to go on to prompt at the terminal. But even there it feels like closing a potential door around PROMPT_ECHO. The standard askpass interface doesn't have a way for us to ask for it to show the contents to the user (which is useful for the username prompt). But it would be a reasonable extension to add one. Say, a GIT_ASKPASS_ECHO that, if set, is preferred for the username prompt. So I dunno. This seems somewhere between churn and making things worse, and I don't see what it's buying us at all. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] prompt.c: split up the password and non-password handling 2021-11-03 11:53 ` Jeff King @ 2021-11-03 17:28 ` Junio C Hamano 0 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2021-11-03 17:28 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, git, Johannes Schindelin, Alejandro Sanchez Jeff King <peff@peff.net> writes: > So I dunno. This seems somewhere between churn and making things worse, > and I don't see what it's buying us at all. Unfortunately I have to agree with the summary. Not a material I want to spend time on this time in the cycle anyway X-<. Thanks for reading. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true 2021-11-02 16:48 ` [PATCH 0/2] prompt.c: split up git_prompt(), read from /dev/tty, not STDIN Ævar Arnfjörð Bjarmason 2021-11-02 16:48 ` [PATCH 1/2] prompt.c: split up the password and non-password handling Ævar Arnfjörð Bjarmason @ 2021-11-02 16:48 ` Ævar Arnfjörð Bjarmason 2021-11-03 11:57 ` Jeff King 1 sibling, 1 reply; 30+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-11-02 16:48 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Alejandro Sanchez, Ævar Arnfjörð Bjarmason In 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20) we we fixed a behavior change in the conversion of git-am from a shellscript to a C program by changing it from using git_prompt() to using fgets(..., stdin). This ensured that we could run: echo y | git am --interactive [...] But along with that in the subsequent 6e7baf246a2 (am: drop tty requirement for --interactive, 2019-05-20) we had to remove support for: git am --interactive </dev/null This change builds on the refactoring of git_prompt() into "normal prompt" and "wants password" functions in the preceding commit, and moves "git am --interactive" back to using the prompt function. This allows us to have our cake and eat it too by adding a GIT_TERMINAL_PROMPT=true mode to test-lib.sh. Adjusting "git am --interactive" for use in our tests (see e.g. "t/t4257-am-interactive.sh") was what 97387c8bdd9 and 6e7baf246a2 were aiming for. Then more recently in 09535f056b0 (bisect--helper: reimplement `bisect_autostart` shell function in C, 2020-09-24) we've had the same sort of behavior change happen to "git bisect"'s interactive question mode, it now uses git_prompt()'s /dev/tty, not stdin. It seems to me that using /dev/tty is desirable over using stdin, these prompts are meant to be interactive, and our acceptance of stdin was an artifact of how these commands were originally implemented in shellscript. So let's move "git am --interactive" back to using "git_prompt()" (which is called "git_prompt_echo()" as of the preceding commit), and similarly remove the "!isatty(STDIN_FILENO)" test added in 09535f056b0, that control flow was converted as-is from the shellscript behavior. Let's also change a similar assertion added to "git am" in 6e7baf246a2. Now we'll die on: # no arguments provided git am --interactive But not: git am --interactive </dev/null Or: git am --interactive <mbox To do this we'll need to add a GIT_TEST_TERMINAL_PROMPT variable for use in test-lib.sh, by doing so this "echo input | git cmd ..." behavior of interactive commands is now isolated to our own test suite, instead of leaking out into the wild. Now that we've done that we can exhaustively test the prompt behavior of "git bisect", which wasn't previously possible. There is some discussion downthread of the series 97387c8bdd9 is in about whether we should always accept stdin input in these commands[1]. I think that's arguably a good idea, and perhaps we'll need to change the approach here. Using a git_prompt_echo() that we know never needs to handle passwords should provide us with an easy path towards deciding what to do in those cases, we'll be able to consistently pick one behavior or the other, instead of having the behavior of specific commands cater to test-only needs. The lack of _() on the new die() message is intentional. This message will only be emitted if there's a bug in our own test suite, so it's a waste of translator time to translate it. 1. https://lore.kernel.org/git/20190520125016.GA13474@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/am.c | 8 +++----- builtin/bisect--helper.c | 3 --- prompt.c | 8 +++++++- t/t6030-bisect-porcelain.sh | 41 +++++++++++++++++++++++++++++++++++++ t/test-lib.sh | 4 ++++ 5 files changed, 55 insertions(+), 9 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 8677ea2348a..1e90b9ea0cd 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1693,7 +1693,7 @@ static int do_interactive(struct am_state *state) assert(state->msg); for (;;) { - char reply[64]; + const char *reply; puts(_("Commit Body is:")); puts("--------------------------"); @@ -1705,9 +1705,7 @@ static int do_interactive(struct am_state *state) * in your translation. The program will only accept English * input at this point. */ - printf(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: ")); - if (!fgets(reply, sizeof(reply), stdin)) - die("unable to read from stdin; aborting"); + reply = git_prompt_echo(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: ")); if (*reply == 'y' || *reply == 'Y') { return 0; @@ -2437,7 +2435,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) strvec_push(&paths, mkpath("%s/%s", prefix, argv[i])); } - if (state.interactive && !paths.nr) + if (state.interactive && !paths.nr && isatty(0)) die(_("interactive mode requires patches on the command line")); am_setup(&state, patch_format, paths.v, keep_cr); diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 30533a70b53..dd73d76df3e 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -830,9 +830,6 @@ static int bisect_autostart(struct bisect_terms *terms) fprintf_ln(stderr, _("You need to start by \"git bisect " "start\"\n")); - if (!isatty(STDIN_FILENO)) - return -1; - /* * TRANSLATORS: Make sure to include [Y] and [n] in your * translation. The program will only accept English input diff --git a/prompt.c b/prompt.c index 458d6637506..273bc30bf0e 100644 --- a/prompt.c +++ b/prompt.c @@ -6,9 +6,15 @@ char *git_prompt(const char *prompt, unsigned int echo) { + const char *test_var = "GIT_TEST_TERMINAL_PROMPT"; char *r = NULL; - if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) { + if (git_env_bool(test_var, 0) && !isatty(0)) { + char reply[64]; + if (!fgets(reply, sizeof(reply), stdin)) + die("unable to read from stdin in '%s=true' mode", test_var); + return xstrdup(reply); + } else if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) { r = git_terminal_prompt(prompt, echo); if (!r) die_errno("could not read"); diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 1be85d064e7..2afb1b57b45 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -45,6 +45,47 @@ test_expect_success 'set up basic repo with 1 file (hello) and 4 commits' ' HASH4=$(git rev-parse --verify HEAD) ' +test_expect_success 'bisect "good" without a "start": no prompt' ' + cat >expect <<-\EOF && + You need to start by "git bisect start" + + fatal: unable to read from stdin in '\''GIT_TEST_TERMINAL_PROMPT=true'\'' mode + EOF + test_expect_code 128 git bisect good HEAD 2>actual && + test_cmp expect actual && + test_must_fail git bisect bad HEAD~ 2>actual && + test_cmp expect actual +' + +test_expect_success 'bisect "good" without a "start": have prompt' ' + cat >expect <<-\EOF && + You need to start by "git bisect start" + + EOF + echo n | test_expect_code 1 git bisect good HEAD 2>actual && + test_cmp expect actual && + echo n | test_must_fail git bisect bad HEAD~ 2>actual && + test_cmp expect actual +' + +test_expect_success 'bisect "good" without a "start": answer prompt' ' + cat >expect <<-\EOF && + You need to start by "git bisect start" + + EOF + echo Y | git bisect good HEAD 2>actual && + test_cmp expect actual && + + # We will only get this far with the "Y" prompt + cat >expect <<-\EOF && + Some good revs are not ancestors of the bad rev. + git bisect cannot work properly in this case. + Maybe you mistook good and bad revs? + EOF + test_must_fail git bisect bad HEAD~ 2>actual && + test_cmp expect actual +' + test_expect_success 'bisect starts with only one bad' ' git bisect reset && git bisect start && diff --git a/t/test-lib.sh b/t/test-lib.sh index 2679a7596a6..778a08ffe4b 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -476,6 +476,10 @@ export GIT_TEST_MERGE_ALGORITHM GIT_TRACE_BARE=1 export GIT_TRACE_BARE +# Have git_prompt_noecho() accept stdin +GIT_TEST_TERMINAL_PROMPT=true +export GIT_TEST_TERMINAL_PROMPT + # Use specific version of the index file format if test -n "${GIT_TEST_INDEX_VERSION:+isset}" then -- 2.33.1.1570.g069344fdd45 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true 2021-11-02 16:48 ` [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true Ævar Arnfjörð Bjarmason @ 2021-11-03 11:57 ` Jeff King 2021-11-03 15:12 ` Ævar Arnfjörð Bjarmason 2021-11-03 17:42 ` Junio C Hamano 0 siblings, 2 replies; 30+ messages in thread From: Jeff King @ 2021-11-03 11:57 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Johannes Schindelin, Alejandro Sanchez On Tue, Nov 02, 2021 at 05:48:10PM +0100, Ævar Arnfjörð Bjarmason wrote: > In 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20) we > we fixed a behavior change in the conversion of git-am from a > shellscript to a C program by changing it from using git_prompt() to > using fgets(..., stdin). This ensured that we could run: > > echo y | git am --interactive [...] > > But along with that in the subsequent 6e7baf246a2 (am: drop tty > requirement for --interactive, 2019-05-20) we had to remove support > for: > > git am --interactive </dev/null > > This change builds on the refactoring of git_prompt() into "normal > prompt" and "wants password" functions in the preceding commit, and > moves "git am --interactive" back to using the prompt function. Why do we want to do that? The only reason I mentioned that "/dev/null" thing in the earlier commit is that it's pointless. IMHO nothing should be using git_prompt() outside of the credential code. They should just be reading from stdin, which is much more flexible. If a caller knows that stdin is coming from elsewhere, they can redirect from /dev/tty. > It seems to me that using /dev/tty is desirable over using stdin, > these prompts are meant to be interactive, and our acceptance of stdin > was an artifact of how these commands were originally implemented in > shellscript. Basically, I think I just disagree with this paragraph entirely. Moving to stdin in the commits you referenced was done to help testing, but I also think it's just a more flexible direction overall. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true 2021-11-03 11:57 ` Jeff King @ 2021-11-03 15:12 ` Ævar Arnfjörð Bjarmason 2021-11-04 9:58 ` Jeff King 2021-11-03 17:42 ` Junio C Hamano 1 sibling, 1 reply; 30+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-11-03 15:12 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, Johannes Schindelin, Alejandro Sanchez On Wed, Nov 03 2021, Jeff King wrote: > On Tue, Nov 02, 2021 at 05:48:10PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> In 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20) we >> we fixed a behavior change in the conversion of git-am from a >> shellscript to a C program by changing it from using git_prompt() to >> using fgets(..., stdin). This ensured that we could run: >> >> echo y | git am --interactive [...] >> >> But along with that in the subsequent 6e7baf246a2 (am: drop tty >> requirement for --interactive, 2019-05-20) we had to remove support >> for: >> >> git am --interactive </dev/null >> >> This change builds on the refactoring of git_prompt() into "normal >> prompt" and "wants password" functions in the preceding commit, and >> moves "git am --interactive" back to using the prompt function. > > Why do we want to do that? The only reason I mentioned that "/dev/null" > thing in the earlier commit is that it's pointless. > > IMHO nothing should be using git_prompt() outside of the credential > code. They should just be reading from stdin, which is much more > flexible. If a caller knows that stdin is coming from elsewhere, they > can redirect from /dev/tty. > >> It seems to me that using /dev/tty is desirable over using stdin, >> these prompts are meant to be interactive, and our acceptance of stdin >> was an artifact of how these commands were originally implemented in >> shellscript. > > Basically, I think I just disagree with this paragraph entirely. Moving > to stdin in the commits you referenced was done to help testing, but I > also think it's just a more flexible direction overall. I'm fine with it either way, my reading of your 2019-ish commits was that you did that not to intentionally get that behavior, but to work around that test issue. So we do always want the "read from stdin" behavior, so I can get those bisect tests by just changing its behavior too, with no need for the test variable? If so I'm fine with that. I think it's a good thing in general to have a not-for-password git_prompt() API, because it makes it easy to make that use some readline-like API, i.e. one that would have tab completion, and handle the loop some (but not all) callers have around handling retries etc, we'd also be able to translate the "Y" "n" characters... ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true 2021-11-03 15:12 ` Ævar Arnfjörð Bjarmason @ 2021-11-04 9:58 ` Jeff King 0 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2021-11-04 9:58 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Johannes Schindelin, Alejandro Sanchez On Wed, Nov 03, 2021 at 04:12:43PM +0100, Ævar Arnfjörð Bjarmason wrote: > I'm fine with it either way, my reading of your 2019-ish commits was > that you did that not to intentionally get that behavior, but to work > around that test issue. > > So we do always want the "read from stdin" behavior, so I can get those > bisect tests by just changing its behavior too, with no need for the > test variable? If so I'm fine with that. Yes. The thing that makes it OK to do from a backwards-compatibility standpoint is that even though the tools read from /dev/tty now, they also insist that stdin is a tty. So in practice it will always be the same thing (technically you _could_ redirect stdin from a different tty, but that's sufficiently bizarre that I think we can discount it). > I think it's a good thing in general to have a not-for-password > git_prompt() API, because it makes it easy to make that use some > readline-like API, i.e. one that would have tab completion, and handle > the loop some (but not all) callers have around handling retries etc, > we'd also be able to translate the "Y" "n" characters... Yeah, I'd be OK with that direction, but I think it is not really the same thing that the existing git_prompt() was designed for (where you most certainly don't want readline-ish things like history for your password). Perhaps a good first step is not so much splitting git_prompt() into two pieces, as simply renaming it in place. It really was designed for the credential request and not much else, but the name is a bit more generic. But I'd probably wait on that until we were looking at actually adding a functional readline-ish interface before even doing that. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true 2021-11-03 11:57 ` Jeff King 2021-11-03 15:12 ` Ævar Arnfjörð Bjarmason @ 2021-11-03 17:42 ` Junio C Hamano 2021-11-04 8:48 ` Johannes Schindelin 2021-11-04 9:53 ` Jeff King 1 sibling, 2 replies; 30+ messages in thread From: Junio C Hamano @ 2021-11-03 17:42 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, git, Johannes Schindelin, Alejandro Sanchez Jeff King <peff@peff.net> writes: > Basically, I think I just disagree with this paragraph entirely. Moving > to stdin in the commits you referenced was done to help testing, but I > also think it's just a more flexible direction overall. It is OK, and it is more convenient for writing test scripts, to take interactive input from the standard input stream, if the command does not use the standard input for other purposes. "git am -i <mbox" cannot take prompted input via the standard input, but "git am -i mbox" is an easy workaround, for example. Commands that are designed to be used in the downstream of a pipe (e.g. "git rev-list ... | git pack-objects") cannot easily use such a workaround, so they may still need to open and interact with /dev/tty if they want to do an interactive input, though [*]. [Footnote] * "pack-objects" is an excellent example of a command that takes its primary input from the standard input, but is a horrible example otherwise, because it probably would not make sense for it to take any prompted input. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true 2021-11-03 17:42 ` Junio C Hamano @ 2021-11-04 8:48 ` Johannes Schindelin 2021-11-04 9:47 ` Jeff King 2021-11-04 9:53 ` Jeff King 1 sibling, 1 reply; 30+ messages in thread From: Johannes Schindelin @ 2021-11-04 8:48 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Ævar Arnfjörð Bjarmason, git, Alejandro Sanchez Hi Junio & Peff, On Wed, 3 Nov 2021, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Basically, I think I just disagree with this paragraph entirely. > > Moving to stdin in the commits you referenced was done to help > > testing, but I also think it's just a more flexible direction overall. > > It is OK, and it is more convenient for writing test scripts, to take > interactive input from the standard input stream, if the command does > not use the standard input for other purposes. I think I remember when we talked about this, it was in the context of `git add -p` becoming a built-in, and we all agreed that it is actually a very nice side effect that you can feed commands to `git add -p` in scripts via stdin, not only for testing. It might have been in the context of another command, but even then it is a fact that this is a very nice side effect. Ciao, Dscho ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true 2021-11-04 8:48 ` Johannes Schindelin @ 2021-11-04 9:47 ` Jeff King 0 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2021-11-04 9:47 UTC (permalink / raw) To: Johannes Schindelin Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git, Alejandro Sanchez On Thu, Nov 04, 2021 at 09:48:35AM +0100, Johannes Schindelin wrote: > Hi Junio & Peff, > > On Wed, 3 Nov 2021, Junio C Hamano wrote: > > > Jeff King <peff@peff.net> writes: > > > > > Basically, I think I just disagree with this paragraph entirely. > > > Moving to stdin in the commits you referenced was done to help > > > testing, but I also think it's just a more flexible direction overall. > > > > It is OK, and it is more convenient for writing test scripts, to take > > interactive input from the standard input stream, if the command does > > not use the standard input for other purposes. > > I think I remember when we talked about this, it was in the context of > `git add -p` becoming a built-in, and we all agreed that it is actually a > very nice side effect that you can feed commands to `git add -p` in > scripts via stdin, not only for testing. > > It might have been in the context of another command, but even then it is > a fact that this is a very nice side effect. Yes, we definitely had that discussion about "add -p", and I agree it is nice. People are probably less likely to drive other tools like git-am and git-bisect in such a way, though, as their interactive modes just do a lot less. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true 2021-11-03 17:42 ` Junio C Hamano 2021-11-04 8:48 ` Johannes Schindelin @ 2021-11-04 9:53 ` Jeff King 1 sibling, 0 replies; 30+ messages in thread From: Jeff King @ 2021-11-04 9:53 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, git, Johannes Schindelin, Alejandro Sanchez On Wed, Nov 03, 2021 at 10:42:14AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Basically, I think I just disagree with this paragraph entirely. Moving > > to stdin in the commits you referenced was done to help testing, but I > > also think it's just a more flexible direction overall. > > It is OK, and it is more convenient for writing test scripts, to > take interactive input from the standard input stream, if the > command does not use the standard input for other purposes. > > "git am -i <mbox" cannot take prompted input via the standard input, > but "git am -i mbox" is an easy workaround, for example. > > Commands that are designed to be used in the downstream of a pipe > (e.g. "git rev-list ... | git pack-objects") cannot easily use such > a workaround, so they may still need to open and interact with > /dev/tty if they want to do an interactive input, though [*]. True. The most Unix-y thing there would be to provide an option for reading interactive input from an arbitrary descriptor. That gives the most flexibility, though it's probably a bit arcane for most folks to do: git foo | git bar --interactive-from=3 3</dev/tty We could directly allow: git foo | git bar --interactive-from=/dev/tty which is a bit less arcane. Or alternatively this could come from the environment, like: export GIT_INTERACTIVE_FROM=/dev/tty git foo | git bar --interactive Which is equivalent-ish to having a boolean env variable to say "read from the terminal", except that it retains some more of the flexibility (especially if we treat a numeric value as a descriptor). Of course yet another option is to teach commands like pack-objects that read input only from stdin to accept a command-line option to read that input from a file. Then stdin is free for interactive use. ;) But I would not do any of that until we had a command that was a good candidate. In the case of git-am and git-bisect, I think it's fine to assume that "-i" will use stdin. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 4/4] am: fix --interactive HEAD tree resolution 2019-05-20 12:06 ` [PATCH 0/4] fix BUG() with "git am -i --resolved" Jeff King ` (2 preceding siblings ...) 2019-05-20 12:11 ` [PATCH 3/4] am: drop tty requirement for --interactive Jeff King @ 2019-05-20 12:13 ` Jeff King 2019-05-23 7:12 ` Johannes Schindelin 3 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2019-05-20 12:13 UTC (permalink / raw) To: Alejandro Sanchez; +Cc: git In interactive mode, "git am -i --resolved" will try to generate a patch based on what is in the index, so that it can prompt "apply this patch?". To do so it needs the tree of HEAD, which it tries to get with get_oid_tree(). However, this doesn't yield a tree oid; the "tree" part just means "if you must disambiguate short oids, then prefer trees" (and we do not need to disambiguate at all, since we are feeding a ref name). Instead, we must parse the oid as a commit (which should always be true in a non-corrupt repository), and access its tree pointer manually. This has been broken since the conversion to C in 7ff2683253 (builtin-am: implement -i/--interactive, 2015-08-04), but there was no test coverage because of interactive-mode's insistence on having a tty. That was lifted in the previous commit, so we can now add a test for this case. Note that before this patch, the test would result in a BUG() which comes from 3506dc9445 (has_uncommitted_changes(): fall back to empty tree, 2018-07-11). But before that, we'd have simply segfaulted (and in fact this is the exact type of case the BUG() added there was trying to catch!). Signed-off-by: Jeff King <peff@peff.net> --- builtin/am.c | 14 ++++++++--- t/t4257-am-interactive.sh | 49 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) create mode 100755 t/t4257-am-interactive.sh diff --git a/builtin/am.c b/builtin/am.c index ea16b844f1..33bd7a6eab 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1339,9 +1339,17 @@ static void write_index_patch(const struct am_state *state) struct rev_info rev_info; FILE *fp; - if (!get_oid_tree("HEAD", &head)) - tree = lookup_tree(the_repository, &head); - else + if (!get_oid("HEAD", &head)) { + struct object *obj; + struct commit *commit; + + obj = parse_object_or_die(&head, NULL); + commit = object_as_type(the_repository, obj, OBJ_COMMIT, 0); + if (!commit) + die("unable to parse HEAD as a commit"); + + tree = get_commit_tree(commit); + } else tree = lookup_tree(the_repository, the_repository->hash_algo->empty_tree); diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh new file mode 100755 index 0000000000..6989bf7aba --- /dev/null +++ b/t/t4257-am-interactive.sh @@ -0,0 +1,49 @@ +#!/bin/sh + +test_description='am --interactive tests' +. ./test-lib.sh + +test_expect_success 'set up patches to apply' ' + test_commit unrelated && + test_commit no-conflict && + test_commit conflict-patch file patch && + git format-patch --stdout -2 >mbox && + + git reset --hard unrelated && + test_commit conflict-master file master base +' + +# Sanity check our setup. +test_expect_success 'applying all patches generates conflict' ' + test_must_fail git am mbox && + echo resolved >file && + git add -u && + git am --resolved +' + +test_expect_success 'interactive am can apply a single patch' ' + git reset --hard base && + printf "%s\n" y n | git am -i mbox && + + echo no-conflict >expect && + git log -1 --format=%s >actual && + test_cmp expect actual +' + +test_expect_success 'interactive am can resolve conflict' ' + git reset --hard base && + printf "%s\n" y y | test_must_fail git am -i mbox && + echo resolved >file && + git add -u && + printf "%s\n" v y | git am -i --resolved && + + echo conflict-patch >expect && + git log -1 --format=%s >actual && + test_cmp expect actual && + + echo resolved >expect && + git cat-file blob HEAD:file >actual && + test_cmp expect actual +' + +test_done -- 2.22.0.rc1.539.g7bfcdfe86d ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution 2019-05-20 12:13 ` [PATCH 4/4] am: fix --interactive HEAD tree resolution Jeff King @ 2019-05-23 7:12 ` Johannes Schindelin 2019-05-24 6:39 ` Jeff King 0 siblings, 1 reply; 30+ messages in thread From: Johannes Schindelin @ 2019-05-23 7:12 UTC (permalink / raw) To: Jeff King; +Cc: Alejandro Sanchez, git Hi Peff, On Mon, 20 May 2019, Jeff King wrote: > In interactive mode, "git am -i --resolved" will try to generate a patch > based on what is in the index, so that it can prompt "apply this > patch?". To do so it needs the tree of HEAD, which it tries to get with > get_oid_tree(). However, this doesn't yield a tree oid; the "tree" part > just means "if you must disambiguate short oids, then prefer trees" (and > we do not need to disambiguate at all, since we are feeding a ref name). > > Instead, we must parse the oid as a commit (which should always be true > in a non-corrupt repository), and access its tree pointer manually. > > This has been broken since the conversion to C in 7ff2683253 > (builtin-am: implement -i/--interactive, 2015-08-04), but there was no > test coverage because of interactive-mode's insistence on having a tty. > That was lifted in the previous commit, so we can now add a test for > this case. > > Note that before this patch, the test would result in a BUG() which > comes from 3506dc9445 (has_uncommitted_changes(): fall back to empty > tree, 2018-07-11). But before that, we'd have simply segfaulted (and in > fact this is the exact type of case the BUG() added there was trying to > catch!). What an old breakage! Thanks for analyzing and fixing it. > diff --git a/builtin/am.c b/builtin/am.c > index ea16b844f1..33bd7a6eab 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -1339,9 +1339,17 @@ static void write_index_patch(const struct am_state *state) > struct rev_info rev_info; > FILE *fp; > > - if (!get_oid_tree("HEAD", &head)) > - tree = lookup_tree(the_repository, &head); > - else > + if (!get_oid("HEAD", &head)) { > + struct object *obj; > + struct commit *commit; > + > + obj = parse_object_or_die(&head, NULL); > + commit = object_as_type(the_repository, obj, OBJ_COMMIT, 0); > + if (!commit) > + die("unable to parse HEAD as a commit"); Wouldn't this be easier to read like this: struct commit *commit = lookup_commit_reference(the_repository, &head); > + > + tree = get_commit_tree(commit); > + } else > tree = lookup_tree(the_repository, > the_repository->hash_algo->empty_tree); > > diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh > new file mode 100755 > index 0000000000..6989bf7aba > --- /dev/null > +++ b/t/t4257-am-interactive.sh > @@ -0,0 +1,49 @@ > +#!/bin/sh > + > +test_description='am --interactive tests' > +. ./test-lib.sh > + > +test_expect_success 'set up patches to apply' ' > + test_commit unrelated && > + test_commit no-conflict && > + test_commit conflict-patch file patch && > + git format-patch --stdout -2 >mbox && > + > + git reset --hard unrelated && > + test_commit conflict-master file master base > +' > + > +# Sanity check our setup. > +test_expect_success 'applying all patches generates conflict' ' > + test_must_fail git am mbox && > + echo resolved >file && > + git add -u && > + git am --resolved > +' > + > +test_expect_success 'interactive am can apply a single patch' ' > + git reset --hard base && > + printf "%s\n" y n | git am -i mbox && Since we want contributors to copy-edit our test cases (even if they do not happen to be Unix shell scripting experts), it would be better to write test_write_lines y n | git am -i mbox && here. Same for similar `printf` invocations further down. > + > + echo no-conflict >expect && > + git log -1 --format=%s >actual && > + test_cmp expect actual I would prefer test no-conflict = "$(git show -s --format=%s HEAD)" or even better: test_cmp_head_oneline () { if test "$1" != "$(git show -s --format=%s HEAD)" then echo >&4 "HEAD's oneline is '$(git show -s \ --format=%s HEAD)'; expected '$1'" return 1 fi } > +' > + > +test_expect_success 'interactive am can resolve conflict' ' > + git reset --hard base && > + printf "%s\n" y y | test_must_fail git am -i mbox && > + echo resolved >file && > + git add -u && > + printf "%s\n" v y | git am -i --resolved && Maybe a comment, to explain to the casual reader what the "v" and the "y" are supposed to do? > + > + echo conflict-patch >expect && > + git log -1 --format=%s >actual && > + test_cmp expect actual && > + > + echo resolved >expect && > + git cat-file blob HEAD:file >actual && > + test_cmp expect actual > +' After wrapping my head around the intentions of these commands, I agree that they test for the right thing. Thanks! Dscho > + > +test_done > -- > 2.22.0.rc1.539.g7bfcdfe86d > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution 2019-05-23 7:12 ` Johannes Schindelin @ 2019-05-24 6:39 ` Jeff King 2019-05-24 6:46 ` [PATCH v2 " Jeff King 2019-05-28 11:06 ` [PATCH " Johannes Schindelin 0 siblings, 2 replies; 30+ messages in thread From: Jeff King @ 2019-05-24 6:39 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Alejandro Sanchez, git On Thu, May 23, 2019 at 09:12:27AM +0200, Johannes Schindelin wrote: > > + if (!get_oid("HEAD", &head)) { > > + struct object *obj; > > + struct commit *commit; > > + > > + obj = parse_object_or_die(&head, NULL); > > + commit = object_as_type(the_repository, obj, OBJ_COMMIT, 0); > > + if (!commit) > > + die("unable to parse HEAD as a commit"); > > Wouldn't this be easier to read like this: > > struct commit *commit = > lookup_commit_reference(the_repository, &head); Just the first two lines, I assume you mean; we still have to die ourselves. There is a lookup_commit_or_die(), but weirdly it warns if there was any tag dereferencing. I guess that would never happen here, since we're reading HEAD (and most of the existing calls appear to be for HEAD). So I'll go with that. > > +test_expect_success 'interactive am can apply a single patch' ' > > + git reset --hard base && > > + printf "%s\n" y n | git am -i mbox && > > Since we want contributors to copy-edit our test cases (even if they do > not happen to be Unix shell scripting experts), it would be better to > write > > test_write_lines y n | git am -i mbox && > > here. Same for similar `printf` invocations further down. I think test_write_lines is mostly about avoiding echo chains, but it's probably a little more readable to avoid having to say "\n". I'll adopt that. > > + echo no-conflict >expect && > > + git log -1 --format=%s >actual && > > + test_cmp expect actual > > I would prefer > > test no-conflict = "$(git show -s --format=%s HEAD)" > > or even better: > > test_cmp_head_oneline () { > if test "$1" != "$(git show -s --format=%s HEAD)" > then > echo >&4 "HEAD's oneline is '$(git show -s \ > --format=%s HEAD)'; expected '$1'" > return 1 > fi > } This, I disagree with. IMHO comparing command output using "test" is harder to read and produces worse debugging output (unless you do a helper as you showed, which I think makes the readability even worse). Not to mention that it raises questions of the shell's whitespace handling (though that does not matter for this case). What's your complaint with test_cmp? Is it the extra process? Could we perhaps deal with that by having it use `read` for the happy-path? Or do you prefer having a one-liner? I'd rather come up with a more generic helper to cover this case, that can run any command and compare it to a single argument (or stdin). E.g.,: test_cmp_cmd no-conflict git log -1 --format=%s or test_cmp_cmd - git foo <<-\EOF multi-line expectation EOF But I'd rather approach those issues separately and systematically, and not hold up this bug fix. > > +test_expect_success 'interactive am can resolve conflict' ' > > + git reset --hard base && > > + printf "%s\n" y y | test_must_fail git am -i mbox && > > + echo resolved >file && > > + git add -u && > > + printf "%s\n" v y | git am -i --resolved && > > Maybe a comment, to explain to the casual reader what the "v" and the "y" > are supposed to do? OK. The "v" is actually optional, but I figured it would not hurt to have us print the patch we just generated. I'll add a comment. > After wrapping my head around the intentions of these commands, I agree > that they test for the right thing. Thanks! -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 4/4] am: fix --interactive HEAD tree resolution 2019-05-24 6:39 ` Jeff King @ 2019-05-24 6:46 ` Jeff King 2019-05-28 11:06 ` [PATCH " Johannes Schindelin 1 sibling, 0 replies; 30+ messages in thread From: Jeff King @ 2019-05-24 6:46 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Alejandro Sanchez, git In --interactive mode, "git am --resolved" will try to generate a patch based on what is in the index, so that it can prompt "apply this patch?". To do so it needs the tree of HEAD, which it tries to get with get_oid_tree(). However, this doesn't yield a tree object; the "tree" part just means "if you must disambiguate short oids, then prefer trees" (and we do not need to disambiguate at all, since we are feeding a ref). Instead, we must parse the oid as a commit (which should always be true in a non-corrupt repository), and access its tree pointer manually. This has been broken since the conversion to C in 7ff2683253 (builtin-am: implement -i/--interactive, 2015-08-04), but there was no test coverage because of interactive-mode's insistence on having a tty. That was lifted in the previous commit, so we can now add a test for this case. Note that before this patch, the test would result in a BUG() which comes from 3506dc9445 (has_uncommitted_changes(): fall back to empty tree, 2018-07-11). But before that, we'd have simply segfaulted (and in fact this is the exact type of case the BUG() added there was trying to catch!). Signed-off-by: Jeff King <peff@peff.net> --- This addresses points raised in Dscho's review (except for the test_cmp thing). Since there were no changes to patches 1-3, I didn't bother re-posting. Diff against v1 below. diff --git a/builtin/am.c b/builtin/am.c index 33bd7a6eab..78389d08b6 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1340,14 +1340,7 @@ static void write_index_patch(const struct am_state *state) FILE *fp; if (!get_oid("HEAD", &head)) { - struct object *obj; - struct commit *commit; - - obj = parse_object_or_die(&head, NULL); - commit = object_as_type(the_repository, obj, OBJ_COMMIT, 0); - if (!commit) - die("unable to parse HEAD as a commit"); - + struct commit *commit = lookup_commit_or_die(&head, "HEAD"); tree = get_commit_tree(commit); } else tree = lookup_tree(the_repository, diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh index 6989bf7aba..5344bd248a 100755 --- a/t/t4257-am-interactive.sh +++ b/t/t4257-am-interactive.sh @@ -23,7 +23,8 @@ test_expect_success 'applying all patches generates conflict' ' test_expect_success 'interactive am can apply a single patch' ' git reset --hard base && - printf "%s\n" y n | git am -i mbox && + # apply the first, but not the second + test_write_lines y n | git am -i mbox && echo no-conflict >expect && git log -1 --format=%s >actual && @@ -32,10 +33,12 @@ test_expect_success 'interactive am can apply a single patch' ' test_expect_success 'interactive am can resolve conflict' ' git reset --hard base && - printf "%s\n" y y | test_must_fail git am -i mbox && + # apply both; the second one will conflict + test_write_lines y y | test_must_fail git am -i mbox && echo resolved >file && git add -u && - printf "%s\n" v y | git am -i --resolved && + # interactive "--resolved" will ask us if we want to apply the result + echo y | git am -i --resolved && echo conflict-patch >expect && git log -1 --format=%s >actual && builtin/am.c | 7 +++--- t/t4257-am-interactive.sh | 52 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) create mode 100755 t/t4257-am-interactive.sh diff --git a/builtin/am.c b/builtin/am.c index ea16b844f1..78389d08b6 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1339,9 +1339,10 @@ static void write_index_patch(const struct am_state *state) struct rev_info rev_info; FILE *fp; - if (!get_oid_tree("HEAD", &head)) - tree = lookup_tree(the_repository, &head); - else + if (!get_oid("HEAD", &head)) { + struct commit *commit = lookup_commit_or_die(&head, "HEAD"); + tree = get_commit_tree(commit); + } else tree = lookup_tree(the_repository, the_repository->hash_algo->empty_tree); diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh new file mode 100755 index 0000000000..5344bd248a --- /dev/null +++ b/t/t4257-am-interactive.sh @@ -0,0 +1,52 @@ +#!/bin/sh + +test_description='am --interactive tests' +. ./test-lib.sh + +test_expect_success 'set up patches to apply' ' + test_commit unrelated && + test_commit no-conflict && + test_commit conflict-patch file patch && + git format-patch --stdout -2 >mbox && + + git reset --hard unrelated && + test_commit conflict-master file master base +' + +# Sanity check our setup. +test_expect_success 'applying all patches generates conflict' ' + test_must_fail git am mbox && + echo resolved >file && + git add -u && + git am --resolved +' + +test_expect_success 'interactive am can apply a single patch' ' + git reset --hard base && + # apply the first, but not the second + test_write_lines y n | git am -i mbox && + + echo no-conflict >expect && + git log -1 --format=%s >actual && + test_cmp expect actual +' + +test_expect_success 'interactive am can resolve conflict' ' + git reset --hard base && + # apply both; the second one will conflict + test_write_lines y y | test_must_fail git am -i mbox && + echo resolved >file && + git add -u && + # interactive "--resolved" will ask us if we want to apply the result + echo y | git am -i --resolved && + + echo conflict-patch >expect && + git log -1 --format=%s >actual && + test_cmp expect actual && + + echo resolved >expect && + git cat-file blob HEAD:file >actual && + test_cmp expect actual +' + +test_done -- 2.22.0.rc1.549.gadb183c4cb ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution 2019-05-24 6:39 ` Jeff King 2019-05-24 6:46 ` [PATCH v2 " Jeff King @ 2019-05-28 11:06 ` Johannes Schindelin 2019-05-28 21:35 ` Jeff King 1 sibling, 1 reply; 30+ messages in thread From: Johannes Schindelin @ 2019-05-28 11:06 UTC (permalink / raw) To: Jeff King; +Cc: Alejandro Sanchez, git Hi Peff, On Fri, 24 May 2019, Jeff King wrote: > On Thu, May 23, 2019 at 09:12:27AM +0200, Johannes Schindelin wrote: > > > > + echo no-conflict >expect && > > > + git log -1 --format=%s >actual && > > > + test_cmp expect actual > > > > I would prefer > > > > test no-conflict = "$(git show -s --format=%s HEAD)" > > > > or even better: > > > > test_cmp_head_oneline () { > > if test "$1" != "$(git show -s --format=%s HEAD)" > > then > > echo >&4 "HEAD's oneline is '$(git show -s \ > > --format=%s HEAD)'; expected '$1'" > > return 1 > > fi > > } > > This, I disagree with. IMHO comparing command output using "test" is > harder to read and produces worse debugging output (unless you do a > helper as you showed, which I think makes the readability even worse). > Not to mention that it raises questions of the shell's whitespace > handling (though that does not matter for this case). > > What's your complaint with test_cmp? Is it the extra process? Could we > perhaps deal with that by having it use `read` for the happy-path? I would prefer it if we adopted a more descriptive style in the test suite, as I always found that style to be much easier to work with (you might have guessed that I am spending a lot of time chasing test failures). Succinctness is just one benefit of that. A more important benefit is that you can teach a helper that verifies onelines to show very useful information in case of a failure, something that `test_cmp` cannot do because it is totally agnostic to what it compares. > Or do you prefer having a one-liner? I'd rather come up with a more > generic helper to cover this case, that can run any command and compare > it to a single argument (or stdin). E.g.,: > > test_cmp_cmd no-conflict git log -1 --format=%s > > or > > test_cmp_cmd - git foo <<-\EOF > multi-line > expectation > EOF I guess that you and me go into completely opposite directions here. I want something *less* general. Because I want to optimize for the unfortunate times when a test fails and most likely somebody else than the original author of the test case is tasked with figuring out what the heck goes wrong. You seem to want to optimize for writing test cases. Which I find -- with all due respect -- the wrong thing to optimize for. It is already dirt easy to write new test cases. But *good* test cases (i.e. easy to debug ones)? Not so much. > But I'd rather approach those issues separately and systematically, and > not hold up this bug fix. Sure. > > > +test_expect_success 'interactive am can resolve conflict' ' > > > + git reset --hard base && > > > + printf "%s\n" y y | test_must_fail git am -i mbox && > > > + echo resolved >file && > > > + git add -u && > > > + printf "%s\n" v y | git am -i --resolved && > > > > Maybe a comment, to explain to the casual reader what the "v" and the "y" > > are supposed to do? > > OK. The "v" is actually optional, but I figured it would not hurt to > have us print the patch we just generated. I'll add a comment. Thank you. Ciao, Dscho ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution 2019-05-28 11:06 ` [PATCH " Johannes Schindelin @ 2019-05-28 21:35 ` Jeff King 2019-05-29 11:56 ` Johannes Schindelin 0 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2019-05-28 21:35 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Alejandro Sanchez, git On Tue, May 28, 2019 at 01:06:21PM +0200, Johannes Schindelin wrote: > > Or do you prefer having a one-liner? I'd rather come up with a more > > generic helper to cover this case, that can run any command and compare > > it to a single argument (or stdin). E.g.,: > > > > test_cmp_cmd no-conflict git log -1 --format=%s > > > > or > > > > test_cmp_cmd - git foo <<-\EOF > > multi-line > > expectation > > EOF > > I guess that you and me go into completely opposite directions here. I > want something *less* general. Because I want to optimize for the > unfortunate times when a test fails and most likely somebody else than the > original author of the test case is tasked with figuring out what the heck > goes wrong. > > You seem to want to optimize for writing test cases. Which I find -- with > all due respect -- the wrong thing to optimize for. It is already dirt > easy to write new test cases. But *good* test cases (i.e. easy to debug > ones)? Not so much. Hmm. I too want the test output to be useful to people other than the test author. But I find the output from test_cmp perfectly fine there. My first step in digging into a failure is usually to look at what commands the test is running, which generally makes it obvious why we are expecting one thing and seeing another (or at least, just as obvious as a hand-written message). So to me the two are equal on that front, which makes me want to go with the thing that is shorter to write, as it makes it more likely the test writer will write it. The _worst_ option IMHO is a straight-up use of "test" which provides no output at all in the test log of what value we _did_ see. That requires the person looking into the failure to re-run the test, which is hard if it's a remote CI, or if the failure does not always reproduce. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution 2019-05-28 21:35 ` Jeff King @ 2019-05-29 11:56 ` Johannes Schindelin 2019-09-26 14:20 ` Alejandro Sanchez 0 siblings, 1 reply; 30+ messages in thread From: Johannes Schindelin @ 2019-05-29 11:56 UTC (permalink / raw) To: Jeff King; +Cc: Alejandro Sanchez, git Hi Peff, On Tue, 28 May 2019, Jeff King wrote: > On Tue, May 28, 2019 at 01:06:21PM +0200, Johannes Schindelin wrote: > > > > Or do you prefer having a one-liner? I'd rather come up with a more > > > generic helper to cover this case, that can run any command and compare > > > it to a single argument (or stdin). E.g.,: > > > > > > test_cmp_cmd no-conflict git log -1 --format=%s > > > > > > or > > > > > > test_cmp_cmd - git foo <<-\EOF > > > multi-line > > > expectation > > > EOF > > > > I guess that you and me go into completely opposite directions here. I > > want something *less* general. Because I want to optimize for the > > unfortunate times when a test fails and most likely somebody else than the > > original author of the test case is tasked with figuring out what the heck > > goes wrong. > > > > You seem to want to optimize for writing test cases. Which I find -- with > > all due respect -- the wrong thing to optimize for. It is already dirt > > easy to write new test cases. But *good* test cases (i.e. easy to debug > > ones)? Not so much. > > Hmm. I too want the test output to be useful to people other than the > test author. But I find the output from test_cmp perfectly fine there. > My first step in digging into a failure is usually to look at what > commands the test is running, which generally makes it obvious why we > are expecting one thing and seeing another (or at least, just as obvious > as a hand-written message). > > So to me the two are equal on that front, which makes me want to go with > the thing that is shorter to write, as it makes it more likely the test > writer will write it. The _worst_ option IMHO is a straight-up use of > "test" which provides no output at all in the test log of what value we > _did_ see. That requires the person looking into the failure to re-run > the test, which is hard if it's a remote CI, or if the failure does not > always reproduce. If you think your version is easier to debug, then I won't object. Thanks, Dscho ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution 2019-05-29 11:56 ` Johannes Schindelin @ 2019-09-26 14:20 ` Alejandro Sanchez 2019-09-26 21:11 ` Jeff King 0 siblings, 1 reply; 30+ messages in thread From: Alejandro Sanchez @ 2019-09-26 14:20 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Jeff King, git Hi, Are there any updates to this problem? Thank you, Alex On Wed, May 29, 2019 at 1:57 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Peff, > > On Tue, 28 May 2019, Jeff King wrote: > > > On Tue, May 28, 2019 at 01:06:21PM +0200, Johannes Schindelin wrote: > > > > > > Or do you prefer having a one-liner? I'd rather come up with a more > > > > generic helper to cover this case, that can run any command and compare > > > > it to a single argument (or stdin). E.g.,: > > > > > > > > test_cmp_cmd no-conflict git log -1 --format=%s > > > > > > > > or > > > > > > > > test_cmp_cmd - git foo <<-\EOF > > > > multi-line > > > > expectation > > > > EOF > > > > > > I guess that you and me go into completely opposite directions here. I > > > want something *less* general. Because I want to optimize for the > > > unfortunate times when a test fails and most likely somebody else than the > > > original author of the test case is tasked with figuring out what the heck > > > goes wrong. > > > > > > You seem to want to optimize for writing test cases. Which I find -- with > > > all due respect -- the wrong thing to optimize for. It is already dirt > > > easy to write new test cases. But *good* test cases (i.e. easy to debug > > > ones)? Not so much. > > > > Hmm. I too want the test output to be useful to people other than the > > test author. But I find the output from test_cmp perfectly fine there. > > My first step in digging into a failure is usually to look at what > > commands the test is running, which generally makes it obvious why we > > are expecting one thing and seeing another (or at least, just as obvious > > as a hand-written message). > > > > So to me the two are equal on that front, which makes me want to go with > > the thing that is shorter to write, as it makes it more likely the test > > writer will write it. The _worst_ option IMHO is a straight-up use of > > "test" which provides no output at all in the test log of what value we > > _did_ see. That requires the person looking into the failure to re-run > > the test, which is hard if it's a remote CI, or if the failure does not > > always reproduce. > > If you think your version is easier to debug, then I won't object. > > Thanks, > Dscho ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] am: fix --interactive HEAD tree resolution 2019-09-26 14:20 ` Alejandro Sanchez @ 2019-09-26 21:11 ` Jeff King 0 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2019-09-26 21:11 UTC (permalink / raw) To: Alejandro Sanchez; +Cc: Johannes Schindelin, git On Thu, Sep 26, 2019 at 04:20:05PM +0200, Alejandro Sanchez wrote: > Are there any updates to this problem? The fix for the original bug went into Git v2.22.1. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2021-11-04 9:58 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-20 8:35 Abort (core dumped) Alejandro Sanchez 2019-05-20 10:02 ` Jeff King 2019-05-20 12:06 ` [PATCH 0/4] fix BUG() with "git am -i --resolved" Jeff King 2019-05-20 12:07 ` [PATCH 1/4] am: simplify prompt response handling Jeff King 2019-05-20 12:09 ` [PATCH 2/4] am: read interactive input from stdin Jeff King 2019-05-20 12:11 ` [PATCH 3/4] am: drop tty requirement for --interactive Jeff King 2019-05-20 12:50 ` Jeff King 2019-05-23 6:44 ` Johannes Schindelin 2019-05-24 6:27 ` Jeff King 2021-11-02 16:48 ` [PATCH 0/2] prompt.c: split up git_prompt(), read from /dev/tty, not STDIN Ævar Arnfjörð Bjarmason 2021-11-02 16:48 ` [PATCH 1/2] prompt.c: split up the password and non-password handling Ævar Arnfjörð Bjarmason 2021-11-03 11:53 ` Jeff King 2021-11-03 17:28 ` Junio C Hamano 2021-11-02 16:48 ` [PATCH 2/2] prompt.c: add and use a GIT_TEST_TERMINAL_PROMPT=true Ævar Arnfjörð Bjarmason 2021-11-03 11:57 ` Jeff King 2021-11-03 15:12 ` Ævar Arnfjörð Bjarmason 2021-11-04 9:58 ` Jeff King 2021-11-03 17:42 ` Junio C Hamano 2021-11-04 8:48 ` Johannes Schindelin 2021-11-04 9:47 ` Jeff King 2021-11-04 9:53 ` Jeff King 2019-05-20 12:13 ` [PATCH 4/4] am: fix --interactive HEAD tree resolution Jeff King 2019-05-23 7:12 ` Johannes Schindelin 2019-05-24 6:39 ` Jeff King 2019-05-24 6:46 ` [PATCH v2 " Jeff King 2019-05-28 11:06 ` [PATCH " Johannes Schindelin 2019-05-28 21:35 ` Jeff King 2019-05-29 11:56 ` Johannes Schindelin 2019-09-26 14:20 ` Alejandro Sanchez 2019-09-26 21:11 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.