Hi Rubén
On 15/09/2024 12:38, Rubén Justo wrote:
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.
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.
@@ -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"
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
