diff mbox series

[2/2] add -p: fix checking of user input

Message ID 752e13fb9fd9fd7930d83a0915dbbc0274a99908.1597670589.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit ce910287e727e5b534d806df725c68a8dfc4790a
Headers show
Series add p in C tweaks | expand

Commit Message

Jean-Noël Avila via GitGitGadget Aug. 17, 2020, 1:23 p.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

When a file has been deleted the C version of add -p allows the user
to edit a hunk even though 'e' is not in the list of allowed
responses. (I think 'e' is disallowed because if the file is edited it
is no longer a deletion and we're not set up to rewrite the diff
header).

The invalid response was allowed because the test that determines
whether to display 'e' was not duplicated correctly in the code that
processes the user's choice. Fix this by using flags that are set when
constructing the prompt and checked when processing the user's choice
rather than repeating the check itself.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 add-patch.c | 54 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 16 deletions(-)

Comments

Johannes Schindelin Aug. 18, 2020, 6:38 a.m. UTC | #1 
Hi Phillip,

On Mon, 17 Aug 2020, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When a file has been deleted the C version of add -p allows the user
> to edit a hunk even though 'e' is not in the list of allowed
> responses. (I think 'e' is disallowed because if the file is edited it
> is no longer a deletion and we're not set up to rewrite the diff
> header).
>
> The invalid response was allowed because the test that determines
> whether to display 'e' was not duplicated correctly in the code that
> processes the user's choice. Fix this by using flags that are set when
> constructing the prompt and checked when processing the user's choice
> rather than repeating the check itself.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>

As I had mentioned on the PR, I would much rather have a bug-for-bug
conversion to use the `enum`, and the fix for the `edit` case as a
separate patch on top.

Thank you,
Dscho

> ---
>  add-patch.c | 54 +++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index a15fa407be..907c05b3c1 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1352,6 +1352,15 @@ 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;
> +	enum {
> +		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
> +		ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
> +		ALLOW_GOTO_NEXT_HUNK = 1 << 2,
> +		ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
> +		ALLOW_SEARCH_AND_GOTO = 1 << 4,
> +		ALLOW_SPLIT = 1 << 5,
> +		ALLOW_EDIT = 1 << 6
> +	} permitted = 0;
>
>  	if (!file_diff->hunk_nr)
>  		return 0;
> @@ -1388,22 +1397,35 @@ static int patch_update_file(struct add_p_state *s,
>  		fputs(s->buf.buf, stdout);
>
>  		strbuf_reset(&s->buf);
> -		if (undecided_previous >= 0)
> +		if (undecided_previous >= 0) {
> +			permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
>  			strbuf_addstr(&s->buf, ",k");
> -		if (hunk_index)
> +		}
> +		if (hunk_index) {
> +			permitted |= ALLOW_GOTO_PREVIOUS_HUNK;
>  			strbuf_addstr(&s->buf, ",K");
> -		if (undecided_next >= 0)
> +		}
> +		if (undecided_next >= 0) {
> +			permitted |= ALLOW_GOTO_NEXT_UNDECIDED_HUNK;
>  			strbuf_addstr(&s->buf, ",j");
> -		if (hunk_index + 1 < file_diff->hunk_nr)
> +		}
> +		if (hunk_index + 1 < file_diff->hunk_nr) {
> +			permitted |= ALLOW_GOTO_NEXT_HUNK;
>  			strbuf_addstr(&s->buf, ",J");
> -		if (file_diff->hunk_nr > 1)
> +		}
> +		if (file_diff->hunk_nr > 1) {
> +			permitted |= ALLOW_SEARCH_AND_GOTO;
>  			strbuf_addstr(&s->buf, ",g,/");
> -		if (hunk->splittable_into > 1)
> +		}
> +		if (hunk->splittable_into > 1) {
> +			permitted |= ALLOW_SPLIT;
>  			strbuf_addstr(&s->buf, ",s");
> +		}
>  		if (hunk_index + 1 > file_diff->mode_change &&
> -		    !file_diff->deleted)
> +		    !file_diff->deleted) {
> +			permitted |= ALLOW_EDIT;
>  			strbuf_addstr(&s->buf, ",e");
> -
> +		}
>  		if (file_diff->deleted)
>  			prompt_mode_type = PROMPT_DELETION;
>  		else if (file_diff->added)
> @@ -1452,22 +1474,22 @@ static int patch_update_file(struct add_p_state *s,
>  				break;
>  			}
>  		} else if (s->answer.buf[0] == 'K') {
> -			if (hunk_index)
> +			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
>  				hunk_index--;
>  			else
>  				err(s, _("No previous hunk"));
>  		} else if (s->answer.buf[0] == 'J') {
> -			if (hunk_index + 1 < file_diff->hunk_nr)
> +			if (permitted & ALLOW_GOTO_NEXT_HUNK)
>  				hunk_index++;
>  			else
>  				err(s, _("No next hunk"));
>  		} else if (s->answer.buf[0] == 'k') {
> -			if (undecided_previous >= 0)
> +			if (permitted & ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK)
>  				hunk_index = undecided_previous;
>  			else
>  				err(s, _("No previous hunk"));
>  		} else if (s->answer.buf[0] == 'j') {
> -			if (undecided_next >= 0)
> +			if (permitted & ALLOW_GOTO_NEXT_UNDECIDED_HUNK)
>  				hunk_index = undecided_next;
>  			else
>  				err(s, _("No next hunk"));
> @@ -1475,7 +1497,7 @@ static int patch_update_file(struct add_p_state *s,
>  			char *pend;
>  			unsigned long response;
>
> -			if (file_diff->hunk_nr < 2) {
> +			if (!(permitted & ALLOW_SEARCH_AND_GOTO)) {
>  				err(s, _("No other hunks to goto"));
>  				continue;
>  			}
> @@ -1512,7 +1534,7 @@ static int patch_update_file(struct add_p_state *s,
>  			regex_t regex;
>  			int ret;
>
> -			if (file_diff->hunk_nr < 2) {
> +			if (!(permitted & ALLOW_SEARCH_AND_GOTO)) {
>  				err(s, _("No other hunks to search"));
>  				continue;
>  			}
> @@ -1557,7 +1579,7 @@ static int patch_update_file(struct add_p_state *s,
>  			hunk_index = i;
>  		} else if (s->answer.buf[0] == 's') {
>  			size_t splittable_into = hunk->splittable_into;
> -			if (splittable_into < 2)
> +			if (!(permitted & ALLOW_SPLIT))
>  				err(s, _("Sorry, cannot split this hunk"));
>  			else if (!split_hunk(s, file_diff,
>  					     hunk - file_diff->hunk))
> @@ -1565,7 +1587,7 @@ static int patch_update_file(struct add_p_state *s,
>  						 _("Split into %d hunks."),
>  						 (int)splittable_into);
>  		} else if (s->answer.buf[0] == 'e') {
> -			if (hunk_index + 1 == file_diff->mode_change)
> +			if (!(permitted & ALLOW_EDIT))
>  				err(s, _("Sorry, cannot edit this hunk"));
>  			else if (edit_hunk_loop(s, file_diff, hunk) >= 0) {
>  				hunk->use = USE_HUNK;
> --
> gitgitgadget
>
diff mbox series

Patch

diff --git a/add-patch.c b/add-patch.c
index a15fa407be..907c05b3c1 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1352,6 +1352,15 @@  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;
+	enum {
+		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
+		ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
+		ALLOW_GOTO_NEXT_HUNK = 1 << 2,
+		ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
+		ALLOW_SEARCH_AND_GOTO = 1 << 4,
+		ALLOW_SPLIT = 1 << 5,
+		ALLOW_EDIT = 1 << 6
+	} permitted = 0;
 
 	if (!file_diff->hunk_nr)
 		return 0;
@@ -1388,22 +1397,35 @@  static int patch_update_file(struct add_p_state *s,
 		fputs(s->buf.buf, stdout);
 
 		strbuf_reset(&s->buf);
-		if (undecided_previous >= 0)
+		if (undecided_previous >= 0) {
+			permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
 			strbuf_addstr(&s->buf, ",k");
-		if (hunk_index)
+		}
+		if (hunk_index) {
+			permitted |= ALLOW_GOTO_PREVIOUS_HUNK;
 			strbuf_addstr(&s->buf, ",K");
-		if (undecided_next >= 0)
+		}
+		if (undecided_next >= 0) {
+			permitted |= ALLOW_GOTO_NEXT_UNDECIDED_HUNK;
 			strbuf_addstr(&s->buf, ",j");
-		if (hunk_index + 1 < file_diff->hunk_nr)
+		}
+		if (hunk_index + 1 < file_diff->hunk_nr) {
+			permitted |= ALLOW_GOTO_NEXT_HUNK;
 			strbuf_addstr(&s->buf, ",J");
-		if (file_diff->hunk_nr > 1)
+		}
+		if (file_diff->hunk_nr > 1) {
+			permitted |= ALLOW_SEARCH_AND_GOTO;
 			strbuf_addstr(&s->buf, ",g,/");
-		if (hunk->splittable_into > 1)
+		}
+		if (hunk->splittable_into > 1) {
+			permitted |= ALLOW_SPLIT;
 			strbuf_addstr(&s->buf, ",s");
+		}
 		if (hunk_index + 1 > file_diff->mode_change &&
-		    !file_diff->deleted)
+		    !file_diff->deleted) {
+			permitted |= ALLOW_EDIT;
 			strbuf_addstr(&s->buf, ",e");
-
+		}
 		if (file_diff->deleted)
 			prompt_mode_type = PROMPT_DELETION;
 		else if (file_diff->added)
@@ -1452,22 +1474,22 @@  static int patch_update_file(struct add_p_state *s,
 				break;
 			}
 		} else if (s->answer.buf[0] == 'K') {
-			if (hunk_index)
+			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
 				hunk_index--;
 			else
 				err(s, _("No previous hunk"));
 		} else if (s->answer.buf[0] == 'J') {
-			if (hunk_index + 1 < file_diff->hunk_nr)
+			if (permitted & ALLOW_GOTO_NEXT_HUNK)
 				hunk_index++;
 			else
 				err(s, _("No next hunk"));
 		} else if (s->answer.buf[0] == 'k') {
-			if (undecided_previous >= 0)
+			if (permitted & ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK)
 				hunk_index = undecided_previous;
 			else
 				err(s, _("No previous hunk"));
 		} else if (s->answer.buf[0] == 'j') {
-			if (undecided_next >= 0)
+			if (permitted & ALLOW_GOTO_NEXT_UNDECIDED_HUNK)
 				hunk_index = undecided_next;
 			else
 				err(s, _("No next hunk"));
@@ -1475,7 +1497,7 @@  static int patch_update_file(struct add_p_state *s,
 			char *pend;
 			unsigned long response;
 
-			if (file_diff->hunk_nr < 2) {
+			if (!(permitted & ALLOW_SEARCH_AND_GOTO)) {
 				err(s, _("No other hunks to goto"));
 				continue;
 			}
@@ -1512,7 +1534,7 @@  static int patch_update_file(struct add_p_state *s,
 			regex_t regex;
 			int ret;
 
-			if (file_diff->hunk_nr < 2) {
+			if (!(permitted & ALLOW_SEARCH_AND_GOTO)) {
 				err(s, _("No other hunks to search"));
 				continue;
 			}
@@ -1557,7 +1579,7 @@  static int patch_update_file(struct add_p_state *s,
 			hunk_index = i;
 		} else if (s->answer.buf[0] == 's') {
 			size_t splittable_into = hunk->splittable_into;
-			if (splittable_into < 2)
+			if (!(permitted & ALLOW_SPLIT))
 				err(s, _("Sorry, cannot split this hunk"));
 			else if (!split_hunk(s, file_diff,
 					     hunk - file_diff->hunk))
@@ -1565,7 +1587,7 @@  static int patch_update_file(struct add_p_state *s,
 						 _("Split into %d hunks."),
 						 (int)splittable_into);
 		} else if (s->answer.buf[0] == 'e') {
-			if (hunk_index + 1 == file_diff->mode_change)
+			if (!(permitted & ALLOW_EDIT))
 				err(s, _("Sorry, cannot edit this hunk"));
 			else if (edit_hunk_loop(s, file_diff, hunk) >= 0) {
 				hunk->use = USE_HUNK;