On 18/08/2020 20:44, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
