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