Re: [PATCH v4 6/6] add-patch: introduce the command '|'

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

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

> @@ -1389,6 +1390,7 @@ N_("j - leave this hunk undecided, see next undecided hunk\n"
>     "s - split the current hunk into smaller hunks\n"
>     "e - manually edit the current hunk\n"
>     "p - print the current hunk\n"
> +   "| - use pager to show the current hunk, or use |<program> to customize\n"
>     "? - print help\n");

"to customize" strongly hints that the customization will stick, at
least during this session.  Is that what actually happens?

> @@ -1401,6 +1403,7 @@ static int patch_update_file(struct add_p_state *s,
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	int colored = !!s->colored.len, quit = 0;
>  	enum prompt_mode_type prompt_mode_type;
> +	const char* pager = NULL;

The asterisk sticks to "pager", not its type.

>  	enum {
>  		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
>  		ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
> @@ -1449,9 +1452,15 @@ static int patch_update_file(struct add_p_state *s,
>  		strbuf_reset(&s->buf);
>  		if (file_diff->hunk_nr) {
>  			if (rendered_hunk_index != hunk_index) {
> +				if (pager)
> +					setup_custom_pager(pager);
>  				render_hunk(s, hunk, 0, colored, &s->buf);
>  				fputs(s->buf.buf, stdout);
>  				rendered_hunk_index = hunk_index;
> +				if (pager) {
> +					wait_for_pager();
> +					pager = NULL;
> +				}
>  			}
>  
>  			strbuf_reset(&s->buf);
> @@ -1485,6 +1494,7 @@ static int patch_update_file(struct add_p_state *s,
>  				strbuf_addstr(&s->buf, ",e");
>  			}
>  			strbuf_addstr(&s->buf, ",p");
> +			strbuf_addstr(&s->buf, ",|");
>  		}
>  		if (file_diff->deleted)
>  			prompt_mode_type = PROMPT_DELETION;
> @@ -1512,8 +1522,8 @@ static int patch_update_file(struct add_p_state *s,
>  			continue;
>  		ch = tolower(s->answer.buf[0]);
>  
> -		/* 'g' takes a hunk number and '/' takes a regexp */
> -		if (s->answer.len != 1 && (ch != 'g' && ch != '/')) {
> +		/* 'g' takes a hunk number, '/' takes a regexp and '|' takes a program */
> +		if (s->answer.len != 1 && (ch != 'g' && ch != '/' && ch != '|')) {

Not limited to this instance, but a good discipline is to stop and
think twice before adding the third thing to already existing two.

Perhaps

		/*
		 * 'g' takes a hunk number to go to.
		 * '/' takes a regexp to match.
		 * '|' takes a program to pipe to.
		 */
		if (s->answer.len != 1 && !strchr("g/|", ch))

> @@ -1674,6 +1684,17 @@ static int patch_update_file(struct add_p_state *s,
>  			}
>  		} else if (s->answer.buf[0] == 'p') {
>  			rendered_hunk_index = -1;
> +		} else if (ch == '|') {
> +			strbuf_remove(&s->answer, 0, 1);
> +			if (s->s.use_single_key && s->answer.len == 0) {

If you check .use_single_key, you do not need to check answer.len,
do you?  Can it ever be anything other than 0 here in the single-key
mode?

> +				printf("%s", _("program? "));
> +				fflush(stdout);
> +				strbuf_getline(&s->answer, stdin);
> +				strbuf_trim_trailing_newline(&s->answer);
> +			}
> +			strbuf_trim(&s->answer);
> +			pager = s->answer.buf;

Is it safe to peek into s->answer.buf and expect it to be live until
we have to use the pager like this?

By the way, it should be trivial to make the "custom" pager more sticky.

		} else if (ch == '|') {
			if (s->s.use_single_key) {
				... read into s->answer ...
			} else {
				strbuf_remove(&s->answer, 0, 1);
			}
			strbuf_trim_trailing_newline(&s->answer);

			/*
                         * If it is completely empty, use the last
                         * one, if it is semi-empty, reset to the default.
                         */
			if (!s->answer.len) {
				;
			} else {
				FREE_AND_NULL(pager);
				strbuf_trim(&s->answer);
                                if (!s->answer.len)
                                	pager = xstrdup(s->answer.buf);
			}

Even better, we can lift the scope of "pager" one level up, define
it as an on-stack variable in run_add_p(), add a new parameter of
type "const char **" to patch_update_file(), and use it throughout
patch_update_file(), so that a "custom" pager set here will be
carried across file boundaries.

> +			rendered_hunk_index = -1;
>  		} else if (s->answer.buf[0] == '?') {
>  			const char *p = _(help_patch_remainder), *eol = p;
>  
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 6f6d174687..7b3ebb671d 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -64,8 +64,8 @@ test_expect_success 'unknown command' '
>  	git add -N command &&
>  	git diff command >expect &&
>  	cat >>expect <<-EOF &&
> -	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> -	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
> +	(1/1) Stage addition [y,n,q,a,d,e,p,|,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> +	(1/1) Stage addition [y,n,q,a,d,e,p,|,?]?$SP
>  	EOF
>  	git add -p -- command <command >actual 2>&1 &&
>  	test_cmp expect actual
> @@ -348,9 +348,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,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,?]?
> +	(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
>  '
> @@ -537,13 +537,13 @@ test_expect_success 'split hunk setup' '
>  test_expect_success 'goto hunk 1 with "g 1"' '
>  	test_when_finished "git reset" &&
>  	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 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,p,?]?_
> +	(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 &&
> @@ -556,7 +556,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
>  	_10
>  	+15
>  	_20
> -	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
> +	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
>  	EOF
>  	test_write_lines s y g1 | git add -p >actual &&
>  	tail -n 4 <actual >actual.trimmed &&
> @@ -566,11 +566,11 @@ test_expect_success 'goto hunk 1 with "g1"' '
>  test_expect_success 'navigate to hunk via regex /pattern' '
>  	test_when_finished "git reset" &&
>  	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -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,p,?]?_
> +	(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 &&
> @@ -583,7 +583,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '
>  	_10
>  	+15
>  	_20
> -	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
> +	(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 4 <actual >actual.trimmed &&
> @@ -595,17 +595,38 @@ test_expect_success 'print again the hunk' '
>  	tr _ " " >expect <<-EOF &&
>  	+15
>  	 20
> -	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
> +	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]? @@ -1,2 +1,3 @@
>  	 10
>  	+15
>  	 20
> -	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
> +	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]?_
>  	EOF
>  	test_write_lines s y g 1 p | git add -p >actual &&
>  	tail -n 7 <actual >actual.trimmed &&
>  	test_cmp expect actual.trimmed
>  '
>  
> +test_expect_success TTY 'print again the hunk (PAGER)' '
> +	test_when_finished "git reset" &&
> +	cat >expect <<-EOF &&
> +	<GREEN>+<RESET><GREEN>15<RESET>
> +	 20<RESET>
> +	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
> +	PAGER  10<RESET>
> +	PAGER <GREEN>+<RESET><GREEN>15<RESET>
> +	PAGER  20<RESET>
> +	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]? <RESET>
> +	EOF
> +	test_write_lines s y g 1 \| |
> +	(
> +		GIT_PAGER="sed s/^/PAGER\ /" &&
> +		export GIT_PAGER &&
> +		test_terminal --no-stdin-pty git add -p >actual
> +	) &&
> +	tail -n 7 <actual | test_decode_color >actual.trimmed &&
> +	test_cmp expect actual.trimmed
> +'
> +
>  test_expect_success 'split hunk "add -p (edit)"' '
>  	# Split, say Edit and do nothing.  Then:
>  	#
> @@ -780,21 +801,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,p,?]? <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,p,?]? <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,p,?]? <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,p,?]? <RESET>
> +	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,|,?]? <RESET>
>  	EOF
>  	test_cmp expect actual
>  '