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

[Phillip Wood <phillip.wood123@xxxxxxxxx>]

Hi Rubén

On 28/09/2024 15:30, Rubén Justo wrote:
> The "edit" option allows the user to directly modify the hunk to be
> applied.
>
> If the modified hunk returned by the user is not an applicable patch,
> they will be given the opportunity to try again.
>
> For this new attempt we give them the original hunk;  they have to
> repeat the modification from scratch.
>
> Instead, let's give them the modified patch back, so they can identify
> and fix the problem.
>
> If they really want to start over with a fresh patch they still can
> say 'no', to cancel the "edit" and start anew [*].
>
>      * In the old script-based version of "add -p", this "no" meant
>        discarding the hunk and moving on to the next one.
>
>        This changed, probably unintentionally, during its conversion to
>        C in bcdd297b78 (built-in add -p: implement hunk editing,
>        2019-12-13).
>
>        It now makes more sense not to move to the next block when the
>        user requests to discard their edits.
>
> Signed-off-by: Rubén Justo <rjusto@xxxxxxxxx>
> ---
>
> In this iteration, I'm modifying the message 'saying no discards' to
> make its meaning more explicit, perhaps also gaining some clarity
> along the way for the user who wants to restart editing from the
> original patch.
>
> In the test, I'm adding "n q" to the script as suggested by Phillip.
>
> And, just a bit of mental peace by restoring the hunk from the backup
> before trimming the two strbuf.

I hoped that change would be in edit_hunk_manually() but it isn't.

I'm afraid I still don't think that changing the default is a good idea 
as it is often very difficult to correct a badly edited hunk. Can we 
offer the user a choice of

    (e) edit the original hunk again
    (f) fix the edited hunk
    (d) discard the edit

In [1] you say you discarded that idea because the wording was too 
verbose but something along like the above should be succinct enough.

Best Wishes

Phillip

[1] 
https://lore.kernel.org/git/6f392446-10b4-4074-a993-97ac444275f8@xxxxxxxxx

> Thanks.
>
>   add-patch.c                | 33 ++++++++++++++++++++-------------
>   t/t3701-add-interactive.sh | 13 +++++++++++++
>   2 files changed, 33 insertions(+), 13 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 557903310d..c847b4a59d 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1111,7 +1111,8 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
>   	hunk->colored_end = s->colored.len;
>   }
>   
> -static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
> +static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk,
> +			      size_t plain_len, size_t colored_len)
>   {
>   	size_t i;
>   
> @@ -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);
> +
>   	/* strip out commented lines */
>   	hunk->start = s->plain.len;
>   	for (i = 0; i < s->buf.len; ) {
> @@ -1157,12 +1162,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
>   	}
>   
>   	hunk->end = s->plain.len;
> +
> +	recolor_hunk(s, hunk);
> +
>   	if (hunk->end == hunk->start)
>   		/* The user aborted editing by deleting everything */
>   		return 0;
>   
> -	recolor_hunk(s, hunk);
> -
>   	/*
>   	 * If the hunk header is intact, parse it, otherwise simply use the
>   	 * hunk header prior to editing (which will adjust `hunk->start` to
> @@ -1257,15 +1263,14 @@ static int edit_hunk_loop(struct add_p_state *s,
>   	backup = *hunk;
>   
>   	for (;;) {
> -		int res = edit_hunk_manually(s, hunk);
> +		int res = edit_hunk_manually(s, hunk, plain_len, colored_len);
>   		if (res == 0) {
>   			/* abandoned */
> -			*hunk = backup;
> -			return -1;
> +			break;
>   		}
>   
>   		if (res > 0) {
> -			hunk->delta +=
> +			hunk->delta = backup.delta +
>   				recount_edited_hunk(s, hunk,
>   						    backup.header.old_count,
>   						    backup.header.new_count);
> @@ -1273,10 +1278,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;
>   
>   		/*
>   		 * TRANSLATORS: do not translate [y/n]
> @@ -1286,11 +1287,17 @@ static int edit_hunk_loop(struct add_p_state *s,
>   		 * of the word "no" does not start with n.
>   		 */
>   		res = prompt_yesno(s, _("Your edited hunk does not apply. "
> -					"Edit again (saying \"no\" discards!) "
> +					"Edit again (saying \"no\" discards your edits!) "
>   					"[y/n]? "));
>   		if (res < 1)
> -			return -1;
> +			break;
>   	}
> +
> +	/* Drop a possible edit */
> +	*hunk = backup;
> +	strbuf_setlen(&s->plain, plain_len);
> +	strbuf_setlen(&s->colored, colored_len);
> +	return -1;
>   }
>   
>   static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 718438ffc7..1ceefd96e6 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -165,6 +165,19 @@ test_expect_success 'dummy edit works' '
>   	diff_cmp expected diff
>   '
>   
> +test_expect_success 'editing again works' '
> +	git reset &&
> +	write_script "fake_editor.sh" <<-\EOF &&
> +	grep been-here "$1" >output
> +	echo been-here >"$1"
> +	EOF
> +	(
> +		test_set_editor "$(pwd)/fake_editor.sh" &&
> +		test_write_lines e y n q | GIT_TRACE=1 git add -p
> +	) &&
> +	test_grep been-here output
> +'
> +
>   test_expect_success 'setup patch' '
>   	cat >patch <<-\EOF
>   	@@ -1,1 +1,4 @@
>
> Range-diff against v2:
> 1:  2b55a759d5 ! 1:  7e76606751 add-patch: edit the hunk again
>      @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,
>        		/*
>        		 * TRANSLATORS: do not translate [y/n]
>       @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,
>      - 					"Edit again (saying \"no\" discards!) "
>      + 		 * of the word "no" does not start with n.
>      + 		 */
>      + 		res = prompt_yesno(s, _("Your edited hunk does not apply. "
>      +-					"Edit again (saying \"no\" discards!) "
>      ++					"Edit again (saying \"no\" discards your edits!) "
>        					"[y/n]? "));
>        		if (res < 1)
>       -			return -1;
>      @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,
>        	}
>       +
>       +	/* Drop a possible edit */
>      ++	*hunk = backup;
>       +	strbuf_setlen(&s->plain, plain_len);
>       +	strbuf_setlen(&s->colored, colored_len);
>      -+	*hunk = backup;
>       +	return -1;
>        }
>        
>      @@ t/t3701-add-interactive.sh: test_expect_success 'dummy edit works' '
>       +	EOF
>       +	(
>       +		test_set_editor "$(pwd)/fake_editor.sh" &&
>      -+		test_write_lines e y | GIT_TRACE=1 git add -p
>      ++		test_write_lines e y n q | GIT_TRACE=1 git add -p
>       +	) &&
>       +	test_grep been-here output
>       +'