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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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
>