patch, 2 partsadd p in C tweaks

>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  add-patch.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index f899389e2c..a15fa407be 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -457,11 +457,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  			eol = pend;
>  
>  		if (starts_with(p, "diff ")) {
> -			s->file_diff_nr++;
> -			ALLOC_GROW(s->file_diff, s->file_diff_nr,
> +			ALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,
>  				   file_diff_alloc);
>  			file_diff = s->file_diff + s->file_diff_nr - 1;
> -			memset(file_diff, 0, sizeof(*file_diff));
>  			hunk = &file_diff->head;
>  			hunk->start = p - plain->buf;
>  			if (colored_p)
> @@ -483,11 +481,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  				 */
>  				hunk->splittable_into++;
>  
> -			file_diff->hunk_nr++;
> -			ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr,
> +			ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
>  				   file_diff->hunk_alloc);
>  			hunk = file_diff->hunk + file_diff->hunk_nr - 1;
> -			memset(hunk, 0, sizeof(*hunk));
>  
>  			hunk->start = p - plain->buf;
>  			if (colored)
> @@ -511,7 +507,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  			if (file_diff->mode_change)
>  				BUG("double mode change?\n\n%.*s",
>  				    (int)(eol - plain->buf), plain->buf);
> -			if (file_diff->hunk_nr++)
> +			if (file_diff->hunk_nr)
>  				BUG("mode change in the middle?\n\n%.*s",
>  				    (int)(eol - plain->buf), plain->buf);
>  
> @@ -520,9 +516,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  			 * is _part of_ the header "hunk".
>  			 */
>  			file_diff->mode_change = 1;
> -			ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr,
> +			ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
>  				   file_diff->hunk_alloc);
> -			memset(file_diff->hunk, 0, sizeof(struct hunk));
>  			file_diff->hunk->start = p - plain->buf;
>  			if (colored_p)
>  				file_diff->hunk->colored_start =

> 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(-)
>
> 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
>

> A code cleanup and small bug fix for the C version of add -p
>
> dscho has pointed out that the bug fix in the second patch gets lost in the
> other changes and suggested adding the last member of the enum (which fixes
> the bug with handling 'e') as a separate patch. I'm unsure as it feels odd
> to split up the introduction of the flags - I'd be interested to hear what
> others think.

>
> Phillip Wood (2):
>   add -p: use ALLOC_GROW_BY instead of ALLOW_GROW
>   add -p: fix checking of user input
>
>  add-patch.c | 67 +++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 42 insertions(+), 25 deletions(-)
>
>
> base-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-702%2Fphillipwood%2Fwip%2Fadd-p-fixes-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-702/phillipwood/wip/add-p-fixes-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/702
> --
> gitgitgadget
>

> A code cleanup and small bug fix for the C version of add -p
>
> dscho has pointed out that the bug fix in the second patch gets lost in the
> other changes and suggested adding the last member of the enum (which fixes
> the bug with handling 'e') as a separate patch. I'm unsure as it feels odd
> to split up the introduction of the flags - I'd be interested to hear what
> others think.

> Phillip Wood (2):
>   add -p: use ALLOC_GROW_BY instead of ALLOW_GROW
>   add -p: fix checking of user input
>
>  add-patch.c | 67 +++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 42 insertions(+), 25 deletions(-)
>
>
> base-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-702%2Fphillipwood%2Fwip%2Fadd-p-fixes-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-702/phillipwood/wip/add-p-fixes-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/702

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> A code cleanup and small bug fix for the C version of add -p
>>
>> dscho has pointed out that the bug fix in the second patch gets lost in the
>> other changes and suggested adding the last member of the enum (which fixes
>> the bug with handling 'e') as a separate patch. I'm unsure as it feels odd
>> to split up the introduction of the flags - I'd be interested to hear what
>> others think.
> 
> Essentially, the original was doing:
> 
>     - In early part of patch_update_file(), decide what option to show
>       in s->buf using "if (undecided_previous >= 0)" etc. boolean
>       expression that is tailored for each command;
> 
>     - In later part of patch_update_file(), after getting an answer
>       to the end user, try to use the same boolean expression that
>       is tailored for each command to see if the given command is
>       acceptable.
> 
> and the bug was that each pair of boolean expressions that are
> supposed to be identical were duplicated in two places, and one pair
> was not identical by mistake.
> 
> Your [2/2] fixes it by turning the above to
> 
>     - In early part of patch_update_file(), decide what option to show
>       in s->buf using "if (undecided_previous >= 0)" etc. boolean
>       expression that is tailored for each command, *AND* record the
>       fact that the command is allowed in the permitted bitmask.
> 
>     - In later part of patch_update_file(), after getting an answer
>       to the end user, consult the permitted bitmask computed
>       earlier to see if the given command is acceptable.
> 
> Since there no longer is duplicated boolean expressions that are
> supposed to be the same but different by a bug, once this conversion
> is made, it is impossible to have the bug.  For that reason, I do
> not think the suggested split makes much sense.
> 
> A much saner split, if we have to split this step into two, would be
> to first fix the bug keeping the code structure of the original,
> i.e. the later part guards the 'e' command with
> 
> 	if (hunk_index + 1 == file_diff->mode_change)
> 
> but the earlier part also required !file_diff->deleted, i.e. the
> condition should have been
> 
> 	if (hunk_index + 1 > file_diff->mode_change && !file_diff->deleted)
> 
> So without introducing enum and permitted bitmask, you can fix the
> bug in place, replacing the incorrect boolean condition in the later
> part that guards the 'e' command with a corrected one.  That would
> be a minimum fix that can become your new [2/2], whose theme is "to
> fix the bug with minumum change".
> 
> On top of that, you can convert the function again to reach the
> final shape that writes each boolean condition only once and records
> the permitted commands in the bitmask.  That can be your new [3/2],
> whose these is "to make it impossible to introduce the bug previous
> step fixed".

>> Phillip Wood (2):
>>   add -p: use ALLOC_GROW_BY instead of ALLOW_GROW
>>   add -p: fix checking of user input
>>
>>  add-patch.c | 67 +++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 42 insertions(+), 25 deletions(-)
>>
>>
>> base-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-702%2Fphillipwood%2Fwip%2Fadd-p-fixes-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-702/phillipwood/wip/add-p-fixes-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/702