Re: [PATCH 1/2] add-patch: introduce 'p' in interactive-patch

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

> Shortly we're going make interactive-patch stop printing automatically
> the hunk under certain circumstances.
>
> Let's introduce a new option to allow the user to explicitly request
> the printing.

That is good, but ...

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  add-patch.c                |  4 ++++
>  t/t3701-add-interactive.sh | 22 +++++++++++-----------
>  2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 68f525b35c..52be1ddb15 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1388,6 +1388,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
>     "/ - search for a hunk matching the given regex\n"
>     "s - split the current hunk into smaller hunks\n"
>     "e - manually edit the current hunk\n"
> +   "p - print again the current hunk\n"
>     "? - print help\n");
>  
>  static int patch_update_file(struct add_p_state *s,
> @@ -1480,6 +1481,7 @@ static int patch_update_file(struct add_p_state *s,
>  				permitted |= ALLOW_EDIT;
>  				strbuf_addstr(&s->buf, ",e");
>  			}
> +			strbuf_addstr(&s->buf, ",p");
>  		}
>  		if (file_diff->deleted)
>  			prompt_mode_type = PROMPT_DELETION;
> @@ -1658,6 +1660,8 @@ static int patch_update_file(struct add_p_state *s,
>  				hunk->use = USE_HUNK;
>  				goto soft_increment;
>  			}
> +		} else if (s->answer.buf[0] == 'p') {
> +			/* nothing to do */

This is not good.  If we are taking a new input, why doesn't the
code already respond to it?  "Showing it again" should be a separate
feature even if some other codepaths still do show when [2/2] would
prevent them to show, no?

Also, in addition to the changes to the patch to unbreak it, we'd
need to update the git-add(1) manual page, especially its section on
the interactive mode.  I think a single-line addition should suffice,
but it has to be there, added by the same patch as the one that
starts accepting 'p' and acting on that input.

Thanks.

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 0b5339ac6c..bc55255b0a 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -325,9 +325,9 @@ test_expect_success 'different prompts for mode change/deleted' '
>  	git -c core.filemode=true add -p >actual &&
>  	sed -n "s/^\(([0-9/]*) Stage .*?\).*/\1/p" actual >actual.filtered &&
>  	cat >expect <<-\EOF &&
> -	(1/1) Stage deletion [y,n,q,a,d,?]?
> -	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,?]?
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]?
> +	(1/1) Stage deletion [y,n,q,a,d,p,?]?
> +	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?
> +	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]?
>  	EOF
>  	test_cmp expect actual.filtered
>  '
> @@ -514,13 +514,13 @@ test_expect_success 'split hunk setup' '
>  test_expect_success 'goto hunk' '
>  	test_when_finished "git reset" &&
>  	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? + 1:  -1,2 +1,3          +15
> +	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
>  	_ 2:  -2,4 +3,8          +21
>  	go to which hunk? @@ -1,2 +1,3 @@
>  	_10
>  	+15
>  	_20
> -	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_
> +	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
>  	EOF
>  	test_write_lines s y g 1 | git add -p >actual &&
>  	tail -n 7 <actual >actual.trimmed &&
> @@ -530,11 +530,11 @@ test_expect_success 'goto hunk' '
>  test_expect_success 'navigate to hunk via regex' '
>  	test_when_finished "git reset" &&
>  	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -1,2 +1,3 @@
> +	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@
>  	_10
>  	+15
>  	_20
> -	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_
> +	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
>  	EOF
>  	test_write_lines s y /1,2 | git add -p >actual &&
>  	tail -n 5 <actual >actual.trimmed &&
> @@ -715,21 +715,21 @@ test_expect_success 'colors can be overridden' '
>  	<BLUE>+<RESET><BLUE>new<RESET>
>  	<CYAN> more-context<RESET>
>  	<BLUE>+<RESET><BLUE>another-one<RESET>
> -	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
> +	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
>  	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
>  	<CYAN> context<RESET>
>  	<BOLD>-old<RESET>
>  	<BLUE>+<RESET><BLUE>new<RESET>
>  	<CYAN> more-context<RESET>
> -	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
> +	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
>  	<CYAN> more-context<RESET>
>  	<BLUE>+<RESET><BLUE>another-one<RESET>
> -	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
> +	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
>  	<CYAN> context<RESET>
>  	<BOLD>-old<RESET>
>  	<BLUE>+new<RESET>
>  	<CYAN> more-context<RESET>
> -	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET>
> +	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
>  	EOF
>  	test_cmp expect actual
>  '