Re: [PATCH] add-patch: edit the hunk again

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 16, 2024, 13:33 UTC
Message-ID: <cba63486-2186-4e8e-aad4-ed7f54606ec7@gmail.com>
In-Reply-To: <21ddf64f-10c2-4087-a778-0bd2e82aef42@gmail.com>

Hi Rubén

On 15/09/2024 12:38, Rubén Justo wrote:

Show 8 quoted lines

> The "edit" option allows the user to directly modify the hunk to be
> applied.
> 
> If the modified hunk returned is not an applicable patch, we give the
> opportunity to try again.
> 
> For this new attempt we provide, again, the original hunk;  the user
> has to repeat the modification from scratch.

As you say below it looks like we started doing this by accident with 2b8ea7f3c7 (add -p: calculate offset delta for edited patches, 2018-03-05). I think that although the change was accidental it was actually a move in the right direction for several reasons.

  - The error message from "git apply" makes it is virtually impossible
    to tell what is wrong with the edited patch. The line numbers in the
    error message refer to the complete patch but the user is editing a
    single hunk so the user has no idea which line of the hunk the error
    message applies to.

  - If the user uses a terminal based editor then they cannot see the
    error messages while they're re-editing the hunk.

  - If the user has deleted a pre-image line then they need to somehow
    magic it back before the hunk will apply.

> Instead, let's give them the faulty modified patch back, so they can
> identify and fix the problem.

The problem is how do they identify the problem? I have some unfinished patches [1] that annotate the edited patch with comments explaining what's wrong. Because we know what the unedited patch looked like and that the pre-image lines should be unchanged it is possible to provide much better error messages than we get from trying to apply the whole patch with "git apply". It also makes it possible to restore deleted pre-image lines.

[1] https://github.com/phillipwood/git/tree/wip/add-p-editing-improvements
     Note that the later patches do not even compile at the moment. I've
     been meaning to split out the first eight patches and clean them up
     as they're mostly functional and just need the commit messages
     cleaning up.

Show 12 quoted lines

> diff --git a/add-patch.c b/add-patch.c
> index 557903310d..125e79a5ae 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
>   				      "addp-hunk-edit.diff", NULL) < 0)
>   		return -1;
>   
> +	/* Drop possible previous edits */
> +	strbuf_setlen(&s->plain, plain_len);
> +	strbuf_setlen(&s->colored, colored_len);
> +

At this point hunk->end points past s->plain.len. If the user has deleted all the lines then we return with hunk->end in this invalid state. I think that turns out not to matter as we end up restoring hunk->end from the backup we make at the beginning of edit_hunk_loop() but it is not straight forward to reason about.

Show 8 quoted lines

> @@ -1273,10 +1277,6 @@ static int edit_hunk_loop(struct add_p_state *s,
>   				return 0;
>   		}
>   
> -		/* Drop edits (they were appended to s->plain) */
> -		strbuf_setlen(&s->plain, plain_len);
> -		strbuf_setlen(&s->colored, colored_len);
> -		*hunk = backup;

In the old version we always restore the hunk from the backup when we trim the edited patch which maintains the invariant "hunk->end <= s->plain->end"

Show 11 quoted lines

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 718438ffc7..6af5636221 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -165,6 +165,20 @@ test_expect_success 'dummy edit works' '
>   	diff_cmp expected diff
>   '
>   
> +test_expect_success 'setup re-edit editor' '
> +	write_script "fake_editor.sh" <<-\EOF &&
> +	grep been-here "$1" && echo found >output

'grep been-here "$1" >output' should be sufficient I think

> +	echo been-here > "$1"
> +	EOF
> +	test_set_editor "$(pwd)/fake_editor.sh"
> +'

I don't think we need to write the fake editor in a separate test. Also it would be better to call test_set_editor in a subshell so that it does not affect later tests.

> +test_expect_success 'editing again works' '
> +	git reset &&
> +	test_write_lines e y | GIT_TRACE=1 git add -p &&

It would be nice to add "n q" to the input to make it complete.

> +	grep found output

Using test_grep makes it easier to debug test failures.

Best Wishes

Phillip