git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 4, 2024, 17:12 UTC
Message-ID
<xmqqy17kws2k.fsf@gitster.g>
In-Reply-To
<75a3cc89-4d23-4eae-b0ad-e52e2c8ba550@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
Show 6 quoted lines
> @@ -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?

Show 5 quoted lines
> @@ -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.
Show 35 quoted lines
>  	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))
Show 7 quoted lines
> @@ -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?

Show 7 quoted lines
> +				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.

Show 147 quoted lines
> +			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
>  '
Previous: Rubén JustoNext: Dragan Simic
Message 89 of 113 in “use the pager in 'add -p'”
  1. 0/5 use the pager in 'add -p'Rubén Justo, May 19, 2024
  2. 1/5 add-patch: test for 'p' commandRubén Justo, May 19, 2024
  3. 2/5 pager: do not close fd 2 unnecessarilyRubén Justo, May 19, 2024
  4. Junio C HamanoMay 20, 2024
  5. Rubén JustoMay 20, 2024
  6. Junio C HamanoMay 21, 2024
  7. Rubén JustoMay 21, 2024
  8. Junio C HamanoMay 21, 2024
  9. Rubén JustoMay 22, 2024
  10. Junio C HamanoMay 22, 2024
  11. Rubén JustoMay 26, 2024
  12. Junio C HamanoMay 26, 2024
  13. 3/5 pager: introduce wait_for_pagerRubén Justo, May 19, 2024
  14. 4/5 test-terminal: introduce --no-stdin-ptyRubén Justo, May 19, 2024
  15. 5/5 add-patch: render hunks through the pagerRubén Justo, May 19, 2024
  16. Junio C HamanoMay 20, 2024
  17. Dragan SimicMay 20, 2024
  18. Rubén JustoMay 20, 2024
  19. Dragan SimicMay 20, 2024
  20. Rubén JustoMay 21, 2024
  21. Jeff KingMay 21, 2024
  22. Rubén JustoMay 21, 2024
  23. Jeff KingMay 23, 2024
  24. Junio C HamanoMay 23, 2024
  25. Dragan SimicMay 23, 2024
  26. Junio C HamanoMay 23, 2024
  27. Dragan SimicMay 23, 2024
  28. Dragan SimicMay 23, 2024
  29. Junio C HamanoMay 23, 2024
  30. Dragan SimicMay 23, 2024
  31. Jeff KingMay 25, 2024
  32. Rubén JustoMay 23, 2024
  33. Dragan SimicMay 23, 2024
  34. Rubén JustoMay 20, 2024
  35. Junio C HamanoMay 20, 2024
  36. Rubén JustoMay 20, 2024
  37. 0/5 use the pager in 'add -p'Rubén Justo, May 21, 2024
  38. 1/5 add-patch: test for 'p' commandRubén Justo, May 21, 2024
  39. 2/5 pager: do not close fd 2 unnecessarilyRubén Justo, May 21, 2024
  40. 3/5 pager: introduce wait_for_pagerRubén Justo, May 21, 2024
  41. 4/5 test-terminal: introduce --no-stdin-ptyRubén Justo, May 21, 2024
  42. 5/5 add-patch: render hunks through the pagerRubén Justo, May 21, 2024
  43. Dragan SimicMay 22, 2024
  44. Junio C HamanoMay 22, 2024
  45. Rubén JustoMay 22, 2024
  46. Dragan SimicMay 22, 2024
  47. 0/6 use the pager in 'add -p'Rubén Justo, Jun 2, 2024
  48. 1/6 add-patch: test for 'p' commandRubén Justo, Jun 2, 2024
  49. 2/6 pager: do not close fd 2 unnecessarilyRubén Justo, Jun 2, 2024
  50. 3/6 pager: introduce wait_for_pagerRubén Justo, Jun 2, 2024
  51. 4/6 pager: introduce setup_custom_pagerRubén Justo, Jun 2, 2024
  52. 5/6 test-terminal: introduce --no-stdin-ptyRubén Justo, Jun 2, 2024
  53. 6/6 add-patch: introduce the command '|'Rubén Justo, Jun 2, 2024
  54. Junio C HamanoJun 2, 2024
  55. Junio C HamanoJun 2, 2024
  56. Rubén JustoJun 2, 2024
  57. Rubén JustoJun 2, 2024
  58. Dragan SimicJun 2, 2024
  59. Junio C HamanoJun 3, 2024
  60. Dragan SimicJun 3, 2024
  61. Junio C HamanoJun 3, 2024
  62. Dragan SimicJun 4, 2024
  63. Dragan SimicJun 2, 2024
  64. Junio C HamanoJun 3, 2024
  65. Dragan SimicJun 4, 2024
  66. Dragan SimicJun 4, 2024
  67. Rubén JustoJun 3, 2024
  68. Dragan SimicJun 4, 2024
  69. 0/6 use the pager in 'add -p'Rubén Justo, Jun 3, 2024
  70. 1/6 add-patch: test for 'p' commandRubén Justo, Jun 3, 2024
  71. 2/6 pager: do not close fd 2 unnecessarilyRubén Justo, Jun 3, 2024
  72. Junio C HamanoJun 4, 2024
  73. 3/6 pager: introduce wait_for_pagerRubén Justo, Jun 3, 2024
  74. Phillip WoodJun 4, 2024
  75. Junio C HamanoJun 4, 2024
  76. Rubén JustoJun 5, 2024
  77. Junio C HamanoJun 4, 2024
  78. 4/6 pager: introduce setup_custom_pagerRubén Justo, Jun 3, 2024
  79. Junio C HamanoJun 4, 2024
  80. 5/6 test-terminal: introduce --no-stdin-ptyRubén Justo, Jun 3, 2024
  81. Phillip WoodJun 4, 2024
  82. Jeff KingJun 4, 2024
  83. Junio C HamanoJun 5, 2024
  84. Jeff KingJun 6, 2024
  85. Rubén JustoJun 5, 2024
  86. Jeff KingJun 6, 2024
  87. Rubén JustoJun 9, 2024
  88. 6/6 add-patch: introduce the command '|'Rubén Justo, Jun 3, 2024
  89. Junio C HamanoJun 4, 2024
  90. Dragan SimicJun 4, 2024
  91. Junio C HamanoJun 5, 2024
  92. Jeff KingJun 4, 2024
  93. Junio C HamanoJun 4, 2024
  94. Jeff KingJun 5, 2024
  95. Phillip WoodJun 5, 2024
  96. Dragan SimicJun 8, 2024
  97. Rubén JustoJun 9, 2024
  98. Dragan SimicJun 9, 2024
  99. Rubén JustoJun 10, 2024
  100. Dragan SimicJun 10, 2024
  101. Phillip WoodJun 10, 2024
  102. Junio C HamanoJun 10, 2024
  103. Rubén JustoJun 10, 2024
  104. Junio C HamanoJun 10, 2024
  105. Dragan SimicJun 10, 2024
  106. Dragan SimicJun 10, 2024
  107. Junio C HamanoJun 10, 2024
  108. Dragan SimicJun 10, 2024
  109. phillip.wood123@gmail.comJun 9, 2024
  110. Dragan SimicJun 9, 2024
  111. Phillip WoodJun 10, 2024
  112. Dragan SimicJun 10, 2024
  113. Junio C HamanoJun 5, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.