Re: [PATCH 0/2] add p in C tweaks

[Phillip Wood <phillip.wood123@gmail.com>]

On 18/08/2020 20:44, Junio C Hamano wrote:
> "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".

Thanks that makes much more sense to me

Best Wishes

Phillip

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