{"thread":{"id":"64384","subject":"[PATCH 1/2] add-patch: quit without skipping undecided hunks","startedAt":"2025-10-25T05:46:50Z","lastAt":"2025-10-26T16:11:12Z","messageCount":8,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"529640","messageId":"0985f775-fb01-4de0-99a8-4775b602829a@web.de","threadId":"64384","inReplyTo":null,"subject":"[PATCH 1/2] add-patch: quit without skipping undecided hunks","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-25T05:46:42Z","receivedAt":"2025-10-25T05:46:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Option q implies d, i.e., it marks any undecided hunks towards the\nbottom of the hunk array as skipped. This is unnecessary; later code\ntreats undecided and skipped hunks the same: The only functions that\nuse UNDECIDED_HUNK and SKIP_HUNK are patch_update_file() itself (but\nnot after its big for loop) and its helpers get_first_undecided() and\ndisplay_hunks().\n\nStreamline the handling of option q by quitting immediately.\n\nSigned-off-by: René Scharfe \n---\n add-patch.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex ae9a20d8f2..a70def1f81 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1601,7 +1601,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t} else if (hunk->use == UNDECIDED_HUNK) {\n \t\t\t\thunk->use = USE_HUNK;\n \t\t\t}\n-\t\t} else if (ch == 'd' || ch == 'q') {\n+\t\t} else if (ch == 'd') {\n \t\t\tif (file_diff->hunk_nr) {\n \t\t\t\tfor (; hunk_index < file_diff->hunk_nr; hunk_index++) {\n \t\t\t\t\thunk = file_diff->hunk + hunk_index;\n@@ -1613,10 +1613,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t} else if (hunk->use == UNDECIDED_HUNK) {\n \t\t\t\thunk->use = SKIP_HUNK;\n \t\t\t}\n-\t\t\tif (ch == 'q') {\n-\t\t\t\tquit = 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n+\t\t} else if (ch == 'q') {\n+\t\t\tquit = 1;\n+\t\t\tbreak;\n \t\t} else if (s->answer.buf[0] == 'K') {\n \t\t\tif (permitted & ALLOW_GOTO_PREVIOUS_HUNK)\n \t\t\t\thunk_index = dec_mod(hunk_index,\n-- \n2.51.1\n"},{"id":"529641","messageId":"13529bee-1e02-4c20-9461-6569312bfe4f@web.de","threadId":"64384","inReplyTo":"0985f775-fb01-4de0-99a8-4775b602829a@web.de","subject":"[PATCH 2/2] add-patch: quit on EOF","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-25T05:48:28Z","receivedAt":"2025-10-25T05:48:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If we reach the end of the input, e.g. because the user pressed ctrl-D\non Linux, there is no point in showing any more prompts, as we won't get\nany reply. Do the same as option 'q' would: Quit.\n\nSigned-off-by: René Scharfe \n---\n add-patch.c | 4 +++-\n t/t3701-add-interactive.sh | 11 +++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex a70def1f81..173a53241e 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1569,8 +1569,10 @@ static int patch_update_file(struct add_p_state *s,\n \t\tif (*s->s.reset_color_interactive)\n \t\t\tfputs(s->s.reset_color_interactive, stdout);\n \t\tfflush(stdout);\n-\t\tif (read_single_character(s) == EOF)\n+\t\tif (read_single_character(s) == EOF) {\n+\t\t\tquit = 1;\n \t\t\tbreak;\n+\t\t}\n \n \t\tif (!s->answer.len)\n \t\t\tcontinue;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 851ca6dd91..071b78c355 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1431,4 +1431,15 @@ test_expect_success 'invalid option s is rejected' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'EOF quits' '\n+\techo a >file &&\n+\techo a >file2 &&\n+\tgit add file file2 &&\n+\techo X >file &&\n+\techo X >file2 &&\n+\tgit add -p out &&\n+\tgrep file out &&\n+\t! grep file2 out\n+'\n+\n test_done\n-- \n2.51.1\n"},{"id":"529643","messageId":"xmqqv7k3ng3b.fsf@gitster.g","threadId":"64384","inReplyTo":"0985f775-fb01-4de0-99a8-4775b602829a@web.de","subject":"Re: [PATCH 1/2] add-patch: quit without skipping undecided hunks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-25T15:42:00Z","receivedAt":"2025-10-25T15:42:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Option q implies d, i.e., it marks any undecided hunks towards the\n> bottom of the hunk array as skipped. This is unnecessary; later code\n> treats undecided and skipped hunks the same: The only functions that\n> use UNDECIDED_HUNK and SKIP_HUNK are patch_update_file() itself (but\n> not after its big for loop) and its helpers get_first_undecided() and\n> display_hunks().\n>\n> Streamline the handling of option q by quitting immediately.\n>\n> Signed-off-by: René Scharfe \n> ---\n> add-patch.c | 9 ++++-----\n> 1 file changed, 4 insertions(+), 5 deletions(-)\n\nYou are really into \"add -p\" for the past few days, aren't you?\n\nI thought I knew this code fairly well (after all, I wrote the\noriginal version before it got ported to C), and cannot believe an\nidiotic mistake like this one remained in the code X-<.\n\nI very much appreciate your careful reading. Will queue.\n\n>\n> diff --git a/add-patch.c b/add-patch.c\n> index ae9a20d8f2..a70def1f81 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1601,7 +1601,7 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t} else if (hunk->use == UNDECIDED_HUNK) {\n> \t\t\t\thunk->use = USE_HUNK;\n> \t\t\t}\n> -\t\t} else if (ch == 'd' || ch == 'q') {\n> +\t\t} else if (ch == 'd') {\n> \t\t\tif (file_diff->hunk_nr) {\n> \t\t\t\tfor (; hunk_index < file_diff->hunk_nr; hunk_index++) {\n> \t\t\t\t\thunk = file_diff->hunk + hunk_index;\n> @@ -1613,10 +1613,9 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t} else if (hunk->use == UNDECIDED_HUNK) {\n> \t\t\t\thunk->use = SKIP_HUNK;\n> \t\t\t}\n> -\t\t\tif (ch == 'q') {\n> -\t\t\t\tquit = 1;\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> +\t\t} else if (ch == 'q') {\n> +\t\t\tquit = 1;\n> +\t\t\tbreak;\n> \t\t} else if (s->answer.buf[0] == 'K') {\n> \t\t\tif (permitted & ALLOW_GOTO_PREVIOUS_HUNK)\n> \t\t\t\thunk_index = dec_mod(hunk_index,\n"},{"id":"529644","messageId":"xmqqfrb7nebp.fsf@gitster.g","threadId":"64384","inReplyTo":"13529bee-1e02-4c20-9461-6569312bfe4f@web.de","subject":"Re: [PATCH 2/2] add-patch: quit on EOF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-25T16:20:10Z","receivedAt":"2025-10-25T16:20:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> If we reach the end of the input, e.g. because the user pressed ctrl-D\n> on Linux, there is no point in showing any more prompts, as we won't get\n> any reply. Do the same as option 'q' would: Quit.\n>\n> Signed-off-by: René Scharfe \n> ---\n> add-patch.c | 4 +++-\n> t/t3701-add-interactive.sh | 11 +++++++++++\n> 2 files changed, 14 insertions(+), 1 deletion(-)\n\nThe code breaks out of the loop (either with or without setting the\n'quit' flag), after which there is \"which hunks are going to be\nused?\" check, followed by \"apply the selected hunks\". So the new\nctrl-D behaviour does not change the end-result left in the files.\nThe effect of the hunks chosen for application will not be abandoned.\n\nIf you are one of those unfortunate folks living dangeously with\ninteractive.singlekey set to true, your ctrl-D would have given you\n\n Unknown command '' (use '?' for help)\n\nin the code before this change, so, this would give them strict\nimprovement.\n\nThe current code happens to *work* for those without the single key\nsetting, in the sense that when we move to subsequent files, the\nfirst call to read_single_character() in patfch_update_file() for\nthem immediately return EOF, breaking out of the loop before any\nhunks for the file gets marked for application, so we'll iterate\nthrough the remaining files without doing anything to these files.\n\nBut we do show the first hunk of all of them before quitting. And\nthis patch squelches these useless output.\n\nOK. This makes sense and makes the change in this patch worthwhile.\n\nI wonder if we want to 'echo\" something in this case, though. If I\nsay 'q', whether interactive.singlekey is active or not, I see\n\n (1/1) Stage this hunk [y,n,q,a,d,s,e,p,P,?]? q\n\non the last line before getting the shell prompt back. With this\nchange, I won't see anything after the prompt. Perhaps it is OK? I\ndunno. Perhaps we want to pretend as if 'q' were given instead of\nEOF, like the following? I dunno.\n\nWill queue as-is.\n\nThanks.\n\n add-patch.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git c/add-patch.c w/add-patch.c\nindex cd71a0359a..f201ead08e 100644\n--- c/add-patch.c\n+++ w/add-patch.c\n@@ -1558,8 +1558,8 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\tfputs(s->s.reset_color_interactive, stdout);\n \t\tfflush(stdout);\n \t\tif (read_single_character(s) == EOF) {\n-\t\t\tquit = 1;\n-\t\t\tbreak;\n+\t\t\tputs(\"q\");\n+\t\t\tstrbuf_addch(&s->answer, 'q');\n \t\t}\n \n \t\tif (!s->answer.len)\n"},{"id":"529648","messageId":"06b8485e-1e64-4c57-be3a-34b1f900c526@web.de","threadId":"64384","inReplyTo":"xmqqfrb7nebp.fsf@gitster.g","subject":"Re: [PATCH 2/2] add-patch: quit on EOF","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-25T17:23:26Z","receivedAt":"2025-10-25T17:23:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 10/25/25 6:20 PM, Junio C Hamano wrote:\n> \n> I wonder if we want to 'echo\" something in this case, though. If I\n> say 'q', whether interactive.singlekey is active or not, I see\n> \n> (1/1) Stage this hunk [y,n,q,a,d,s,e,p,P,?]? q\n> \n> on the last line before getting the shell prompt back. With this\n> change, I won't see anything after the prompt. Perhaps it is OK? I\n> dunno. Perhaps we want to pretend as if 'q' were given instead of\n> EOF, like the following? I dunno.\nI'm used to no feedback when writing to a file using cat and finishing\nwith ctrl-D to signal end-of-file. So I don't need a q \"echoed\", and\nwould actually be slightly surprised. But that's just me.\n\nRené\n\n"},{"id":"529653","messageId":"xmqqjz0imrax.fsf@gitster.g","threadId":"64384","inReplyTo":"06b8485e-1e64-4c57-be3a-34b1f900c526@web.de","subject":"Re: [PATCH 2/2] add-patch: quit on EOF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-26T00:37:26Z","receivedAt":"2025-10-26T00:37:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> On 10/25/25 6:20 PM, Junio C Hamano wrote:\n>> \n>> I wonder if we want to 'echo\" something in this case, though. If I\n>> say 'q', whether interactive.singlekey is active or not, I see\n>> \n>> (1/1) Stage this hunk [y,n,q,a,d,s,e,p,P,?]? q\n>> \n>> on the last line before getting the shell prompt back. With this\n>> change, I won't see anything after the prompt. Perhaps it is OK? I\n>> dunno. Perhaps we want to pretend as if 'q' were given instead of\n>> EOF, like the following? I dunno.\n> I'm used to no feedback when writing to a file using cat and finishing\n> with ctrl-D to signal end-of-file. So I don't need a q \"echoed\", and\n> would actually be slightly surprised. But that's just me.\n\nI do not have a strong preference either way myself. It just looked\na bit abrupt the way the session transcript ends, when it gets shut\ndown with ctrl-D, but after all it is a shutdown, so it may be more\nnatural that way ;-).\n\nI am kind of surprised that this EOF behaviour has not been brought\nup until now, and your patch did not have to touch expected output\nof existing tests (certainly they are taking prepackaged series of\ncommands but I would not imagine all the previous test authors are\ncareful enough to end their tests with 'q'). Perhaps we do not have\nenough multi-hunk and/or multi-file tests on \"git add -p\" and when\nthe tests react to EOF they were already at the \"final\" hunk of the\n\"final\" file and nobody noticed the unnecessary output to skip all\nthe remaining hunks and files, perhaps.\n\n"},{"id":"529654","messageId":"xmqqfrb6mqv4.fsf@gitster.g","threadId":"64384","inReplyTo":"13529bee-1e02-4c20-9461-6569312bfe4f@web.de","subject":"Re: [PATCH 2/2] add-patch: quit on EOF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-26T00:46:55Z","receivedAt":"2025-10-26T00:46:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 851ca6dd91..071b78c355 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -1431,4 +1431,15 @@ test_expect_success 'invalid option s is rejected' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'EOF quits' '\n> +\techo a >file &&\n> +\techo a >file2 &&\n> +\tgit add file file2 &&\n> +\techo X >file &&\n> +\techo X >file2 &&\n> +\tgit add -p out &&\n> +\tgrep file out &&\n> +\t! grep file2 out\n> +'\n> +\n> test_done\n\nLet's do this squashed in.\n\n t/t3701-add-interactive.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh\nindex 071b78c355..4285314f35 100755\n--- c/t/t3701-add-interactive.sh\n+++ w/t/t3701-add-interactive.sh\n@@ -1438,8 +1438,8 @@ test_expect_success 'EOF quits' '\n \techo X >file &&\n \techo X >file2 &&\n \tgit add -p out &&\n-\tgrep file out &&\n-\t! grep file2 out\n+\ttest_grep file out &&\n+\ttest_grep ! file2 out\n '\n \n test_done\n"},{"id":"529663","messageId":"01fb6bdc-7a42-4e14-b7c7-16860ce8af00@web.de","threadId":"64384","inReplyTo":"xmqqfrb6mqv4.fsf@gitster.g","subject":"Re: [PATCH 2/2] add-patch: quit on EOF","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-26T16:11:04Z","receivedAt":"2025-10-26T16:11:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 10/26/25 2:46 AM, Junio C Hamano wrote:\n> \n> Let's do this squashed in.\n> \n> t/t3701-add-interactive.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh\n> index 071b78c355..4285314f35 100755\n> --- c/t/t3701-add-interactive.sh\n> +++ w/t/t3701-add-interactive.sh\n> @@ -1438,8 +1438,8 @@ test_expect_success 'EOF quits' '\n> \techo X >file &&\n> \techo X >file2 &&\n> \tgit add -p out &&\n> -\tgrep file out &&\n> -\t! grep file2 out\n> +\ttest_grep file out &&\n> +\ttest_grep ! file2 out\n> '\n> \n> test_done\n\nGood idea, thank you!\n\nRené\n\n"}]}