[PATCH 1/2] add-patch: quit without skipping undecided hunks

[PATCH 1/2] add-patch: quit without skipping undecided hunks

[René Scharfe]

Option q implies d, i.e., it marks any undecided hunks towards the
bottom of the hunk array as skipped.  This is unnecessary; later code
treats undecided and skipped hunks the same: The only functions that
use UNDECIDED_HUNK and SKIP_HUNK are patch_update_file() itself (but
not after its big for loop) and its helpers get_first_undecided() and
display_hunks().

Streamline the handling of option q by quitting immediately.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 add-patch.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index ae9a20d8f2..a70def1f81 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1601,7 +1601,7 @@ static int patch_update_file(struct add_p_state *s,
 			} else if (hunk->use == UNDECIDED_HUNK) {
 				hunk->use = USE_HUNK;
 			}
-		} else if (ch == 'd' || ch == 'q') {
+		} else if (ch == 'd') {
 			if (file_diff->hunk_nr) {
 				for (; hunk_index < file_diff->hunk_nr; hunk_index++) {
 					hunk = file_diff->hunk + hunk_index;
@@ -1613,10 +1613,9 @@ static int patch_update_file(struct add_p_state *s,
 			} else if (hunk->use == UNDECIDED_HUNK) {
 				hunk->use = SKIP_HUNK;
 			}
-			if (ch == 'q') {
-				quit = 1;
-				break;
-			}
+		} else if (ch == 'q') {
+			quit = 1;
+			break;
 		} else if (s->answer.buf[0] == 'K') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
 				hunk_index = dec_mod(hunk_index,
-- 
2.51.1

[PATCH 2/2] add-patch: quit on EOF

[René Scharfe]

If we reach the end of the input, e.g. because the user pressed ctrl-D
on Linux, there is no point in showing any more prompts, as we won't get
any reply.  Do the same as option 'q' would: Quit.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 add-patch.c                |  4 +++-
 t/t3701-add-interactive.sh | 11 +++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index a70def1f81..173a53241e 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1569,8 +1569,10 @@ static int patch_update_file(struct add_p_state *s,
 		if (*s->s.reset_color_interactive)
 			fputs(s->s.reset_color_interactive, stdout);
 		fflush(stdout);
-		if (read_single_character(s) == EOF)
+		if (read_single_character(s) == EOF) {
+			quit = 1;
 			break;
+		}
 
 		if (!s->answer.len)
 			continue;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 851ca6dd91..071b78c355 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1431,4 +1431,15 @@ test_expect_success 'invalid option s is rejected' '
 	test_cmp expect actual
 '
 
+test_expect_success 'EOF quits' '
+	echo a >file &&
+	echo a >file2 &&
+	git add file file2 &&
+	echo X >file &&
+	echo X >file2 &&
+	git add -p </dev/null >out &&
+	grep file out &&
+	! grep file2 out
+'
+
 test_done
-- 
2.51.1

Re: [PATCH 2/2] add-patch: quit on EOF

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> If we reach the end of the input, e.g. because the user pressed ctrl-D
> on Linux, there is no point in showing any more prompts, as we won't get
> any reply.  Do the same as option 'q' would: Quit.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  add-patch.c                |  4 +++-
>  t/t3701-add-interactive.sh | 11 +++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)

The code breaks out of the loop (either with or without setting the
'quit' flag), after which there is "which hunks are going to be
used?"  check, followed by "apply the selected hunks".  So the new
ctrl-D behaviour does not change the end-result left in the files.
The effect of the hunks chosen for application will not be abandoned.

If you are one of those unfortunate folks living dangeously with
interactive.singlekey set to true, your ctrl-D would have given you

    Unknown command '' (use '?' for help)

in the code before this change, so, this would give them strict
improvement.

The current code happens to *work* for those without the single key
setting, in the sense that when we move to subsequent files, the
first call to read_single_character() in patfch_update_file() for
them immediately return EOF, breaking out of the loop before any
hunks for the file gets marked for application, so we'll iterate
through the remaining files without doing anything to these files.

But we do show the first hunk of all of them before quitting.  And
this patch squelches these useless output.

OK.  This makes sense and makes the change in this patch worthwhile.

I wonder if we want to 'echo" something in this case, though.  If I
say 'q', whether interactive.singlekey is active or not, I see

    (1/1) Stage this hunk [y,n,q,a,d,s,e,p,P,?]? q

on the last line before getting the shell prompt back.  With this
change, I won't see anything after the prompt.  Perhaps it is OK?  I
dunno.  Perhaps we want to pretend as if 'q' were given instead of
EOF, like the following?  I dunno.

Will queue as-is.

Thanks.

 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git c/add-patch.c w/add-patch.c
index cd71a0359a..f201ead08e 100644
--- c/add-patch.c
+++ w/add-patch.c
@@ -1558,8 +1558,8 @@ static int patch_update_file(struct add_p_state *s,
 			fputs(s->s.reset_color_interactive, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF) {
-			quit = 1;
-			break;
+			puts("q");
+			strbuf_addch(&s->answer, 'q');
 		}
 
 		if (!s->answer.len)

Re: [PATCH 2/2] add-patch: quit on EOF

[René Scharfe]

On 10/25/25 6:20 PM, Junio C Hamano wrote:
> 
> I wonder if we want to 'echo" something in this case, though.  If I
> say 'q', whether interactive.singlekey is active or not, I see
> 
>     (1/1) Stage this hunk [y,n,q,a,d,s,e,p,P,?]? q
> 
> on the last line before getting the shell prompt back.  With this
> change, I won't see anything after the prompt.  Perhaps it is OK?  I
> dunno.  Perhaps we want to pretend as if 'q' were given instead of
> EOF, like the following?  I dunno.
I'm used to no feedback when writing to a file using cat and finishing
with ctrl-D to signal end-of-file.  So I don't need a q "echoed", and
would actually be slightly surprised.  But that's just me.

René

Re: [PATCH 2/2] add-patch: quit on EOF

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> On 10/25/25 6:20 PM, Junio C Hamano wrote:
>> 
>> I wonder if we want to 'echo" something in this case, though.  If I
>> say 'q', whether interactive.singlekey is active or not, I see
>> 
>>     (1/1) Stage this hunk [y,n,q,a,d,s,e,p,P,?]? q
>> 
>> on the last line before getting the shell prompt back.  With this
>> change, I won't see anything after the prompt.  Perhaps it is OK?  I
>> dunno.  Perhaps we want to pretend as if 'q' were given instead of
>> EOF, like the following?  I dunno.
> I'm used to no feedback when writing to a file using cat and finishing
> with ctrl-D to signal end-of-file.  So I don't need a q "echoed", and
> would actually be slightly surprised.  But that's just me.

I do not have a strong preference either way myself. It just looked
a bit abrupt the way the session transcript ends, when it gets shut
down with ctrl-D, but after all it is a shutdown, so it may be more
natural that way ;-).

I am kind of surprised that this EOF behaviour has not been brought
up until now, and your patch did not have to touch expected output
of existing tests (certainly they are taking prepackaged series of
commands but I would not imagine all the previous test authors are
careful enough to end their tests with 'q').  Perhaps we do not have
enough multi-hunk and/or multi-file tests on "git add -p" and when
the tests react to EOF they were already at the "final" hunk of the
"final" file and nobody noticed the unnecessary output to skip all
the remaining hunks and files, perhaps.

Re: [PATCH 2/2] add-patch: quit on EOF

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 851ca6dd91..071b78c355 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -1431,4 +1431,15 @@ test_expect_success 'invalid option s is rejected' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'EOF quits' '
> +	echo a >file &&
> +	echo a >file2 &&
> +	git add file file2 &&
> +	echo X >file &&
> +	echo X >file2 &&
> +	git add -p </dev/null >out &&
> +	grep file out &&
> +	! grep file2 out
> +'
> +
>  test_done

Let's do this squashed in.

 t/t3701-add-interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh
index 071b78c355..4285314f35 100755
--- c/t/t3701-add-interactive.sh
+++ w/t/t3701-add-interactive.sh
@@ -1438,8 +1438,8 @@ test_expect_success 'EOF quits' '
 	echo X >file &&
 	echo X >file2 &&
 	git add -p </dev/null >out &&
-	grep file out &&
-	! grep file2 out
+	test_grep file out &&
+	test_grep ! file2 out
 '
 
 test_done

Re: [PATCH 2/2] add-patch: quit on EOF

[René Scharfe]

On 10/26/25 2:46 AM, Junio C Hamano wrote:
> 
> Let's do this squashed in.
> 
>  t/t3701-add-interactive.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh
> index 071b78c355..4285314f35 100755
> --- c/t/t3701-add-interactive.sh
> +++ w/t/t3701-add-interactive.sh
> @@ -1438,8 +1438,8 @@ test_expect_success 'EOF quits' '
>  	echo X >file &&
>  	echo X >file2 &&
>  	git add -p </dev/null >out &&
> -	grep file out &&
> -	! grep file2 out
> +	test_grep file out &&
> +	test_grep ! file2 out
>  '
>  
>  test_done

Good idea, thank you!

René

Re: [PATCH 1/2] add-patch: quit without skipping undecided hunks

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Option q implies d, i.e., it marks any undecided hunks towards the
> bottom of the hunk array as skipped.  This is unnecessary; later code
> treats undecided and skipped hunks the same: The only functions that
> use UNDECIDED_HUNK and SKIP_HUNK are patch_update_file() itself (but
> not after its big for loop) and its helpers get_first_undecided() and
> display_hunks().
>
> Streamline the handling of option q by quitting immediately.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  add-patch.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

You are really into "add -p" for the past few days, aren't you?

I thought I knew this code fairly well (after all, I wrote the
original version before it got ported to C), and cannot believe an
idiotic mistake like this one remained in the code X-<.

I very much appreciate your careful reading.  Will queue.

>
> diff --git a/add-patch.c b/add-patch.c
> index ae9a20d8f2..a70def1f81 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1601,7 +1601,7 @@ static int patch_update_file(struct add_p_state *s,
>  			} else if (hunk->use == UNDECIDED_HUNK) {
>  				hunk->use = USE_HUNK;
>  			}
> -		} else if (ch == 'd' || ch == 'q') {
> +		} else if (ch == 'd') {
>  			if (file_diff->hunk_nr) {
>  				for (; hunk_index < file_diff->hunk_nr; hunk_index++) {
>  					hunk = file_diff->hunk + hunk_index;
> @@ -1613,10 +1613,9 @@ static int patch_update_file(struct add_p_state *s,
>  			} else if (hunk->use == UNDECIDED_HUNK) {
>  				hunk->use = SKIP_HUNK;
>  			}
> -			if (ch == 'q') {
> -				quit = 1;
> -				break;
> -			}
> +		} else if (ch == 'q') {
> +			quit = 1;
> +			break;
>  		} else if (s->answer.buf[0] == 'K') {
>  			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
>  				hunk_index = dec_mod(hunk_index,