Hi Phillip,
On Mon, 17 Aug 2020, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
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
>
