| Message ID | 4dd5a2c7-26a8-470f-b651-e1fe2d1dbcec@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] add-patch: edit the hunk again | expand |
Hi Rubén Thanks for the re-roll. I'm still not convinced that changing this without keeping an easy way to get the current behavior is a good idea. On 18/09/2024 18:51, 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. It's still not clear how an inexperienced user is meant to do that. > If they really want to start over with a fresh patch they still can > say "no" to cancel the "edit" and start anew [*]. This is not very obvious to the user, it would be much better to give them the choice when we prompt them about editing the hunk again. We've been giving the user the original hunk for the last six and a half years so I think it's a bit late to unilaterally change that now. > diff --git a/add-patch.c b/add-patch.c > index 557903310d..75b5129281 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, I would add const struct hunk *backup, here > + 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); then we can restore the back up here with *hunk = *backup; That would make it clear that we're resetting the hunk and would continue to work if we change struct hunk in the future. > /* 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); > + This means we're now forking an external process when there is no hunk to color. It would be better to avoid that by leaving this code where it was and restoring the backup hunk above. > if (hunk->end == hunk->start) > /* The user aborted editing by deleting everything */ > return 0; > > - recolor_hunk(s, hunk); > - > > /* > * TRANSLATORS: do not translate [y/n] > @@ -1289,8 +1290,14 @@ static int edit_hunk_loop(struct add_p_state *s, > "Edit again (saying \"no\" discards!) " > "[y/n]? ")); I think we should make this a three-way choice so the user can choose to keep their changes or start from a valid hunk. > if (res < 1) > - return -1; > + break; > } > + > + /* Drop a possible edit */ > + strbuf_setlen(&s->plain, plain_len); > + strbuf_setlen(&s->colored, colored_len); > + *hunk = backup; > + 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..f3206a317b 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 | GIT_TRACE=1 git add -p This is still missing "n q". Apart from that the test is looking good. Best Wishes Phillip > + ) && > + test_grep been-here output > +' > + > test_expect_success 'setup patch' ' > cat >patch <<-\EOF > @@ -1,1 +1,4 @@ > > Range-diff: > 1: bcf32d0979 ! 1: 2b55a759d5 add-patch: edit the hunk again > @@ add-patch.c: static int edit_hunk_manually(struct add_p_state *s, struct hunk *h > /* strip out commented lines */ > hunk->start = s->plain.len; > for (i = 0; i < s->buf.len; ) { > +@@ add-patch.c: 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 > @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s, > backup = *hunk; > > @@ t/t3701-add-interactive.sh: 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 > -+ echo been-here > "$1" > -+ EOF > -+ test_set_editor "$(pwd)/fake_editor.sh" > -+' > -+ > +test_expect_success 'editing again works' ' > + git reset && > -+ test_write_lines e y | GIT_TRACE=1 git add -p && > -+ grep found output > ++ 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 | GIT_TRACE=1 git add -p > ++ ) && > ++ test_grep been-here output > +' > + > test_expect_success 'setup patch' '
phillip.wood123@gmail.com writes: > Thanks for the re-roll. I'm still not convinced that changing this > without keeping an easy way to get the current behavior is a good > idea. > > This is not very obvious to the user, it would be much better to give > them the choice when we prompt them about editing the hunk > again. We've been giving the user the original hunk for the last six > and a half years so I think it's a bit late to unilaterally change > that now. I almost never use the (e)dit in "add -p", but after trying it and deliberately screwing up the edit, I tend to agree with you. It is very easy to lose what the original change was, what you wanted it to say after the edit in the end state, and how the patch for the current state should look like, and being able to easily start over (and more importantly, knowing that I'd get the version that has none of my screw-ups) was the only thing that convinced me that I might in the future try to use the (e)dit mode again when I find an applicable situation. Thanks for review.
On Mon, Sep 23, 2024 at 10:07:08AM +0100, phillip.wood123@gmail.com wrote: > Thanks for the re-roll. I'm still not convinced that changing this without > keeping an easy way to get the current behavior is a good idea. Thank you for maintaining an open attitude and helping make the change reasonable. > > On 18/09/2024 18:51, 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. > > It's still not clear how an inexperienced user is meant to do that. If inexperience refers to the user not being familiar with the patch format, I don't think it's something we should worry about. It is very likely that few users will attempt to (e)dit, and those who do, inexperienced users experimenting, the normal (and sensible) path they will follow at the slightest difficulty will probably be to abandon patch editing and try a more accessible option, such as: cancel the `add -p` session, edit normally, and restart the session. If inexperience refers to users unfamiliar with the (e)dit option but knowledgeable about the patch format, I believe they will have enough experience to know that reconstructing a patch is a tricky task. Again, the most likely path they will follow when encountering difficulties will be to restart the edit (Junio's message in the thread is an example of this). At any rate, although I understand the concern, this series doesn't aim to improve the mechanisms that help identify the problems in a faulty patch. The goal of this series is to offer (actually to recover) the possibility for the user to make corrections. Something I haven't mentioned before in this thread is that currently, if the user makes a mistake while editing a patch, we don't give them the opportunity to review their error. We leave them in doubt. This happened recently to me editing a patch with context lines containing whitespace errors and "whitespace=fix". But that's another story that I still need to work on [1]. So, to me, it seems sensible to let the user review the faulty patch, even if it's only to discard it. > > > If they really want to start over with a fresh patch they still can > > say "no" to cancel the "edit" and start anew [*]. > > This is not very obvious to the user, It has been so for a decade... Keep in mind that this message will probably only be shown very _very_ rarely to users who are most likely very familiar with (e)dit. > it would be much better to give them > the choice when we prompt them about editing the hunk again. That's an option I explored but abandoned. I didn't come up with any message that I liked that wasn't literally a long paragraph. In the end, I gave up in favor of what I believe is a better option, which is recovering the original intention of "no". I'm not against the option you propose, I'm just not convinced that what we already have, since ac083c47ea (git-add--interactive: manual hunk editing mode, 2008-07-03), isn't intuitive enough for the users of (e)dit. > We've been > giving the user the original hunk for the last six and a half years so I > think it's a bit late to unilaterally change that now. For me, this isn't a reason not to make the change. > > > diff --git a/add-patch.c b/add-patch.c > > index 557903310d..75b5129281 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, > > I would add > const struct hunk *backup, > > here > > > + 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); > > then we can restore the back up here with > > *hunk = *backup; > > That would make it clear that we're resetting the hunk and would continue to > work if we change struct hunk in the future. > > > /* 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); > > + > > This means we're now forking an external process when there is no hunk to > color. It would be better to avoid that by leaving this code where it was > and restoring the backup hunk above. I don't see that external process. ¿? After reviewing the code in the previous iteration, based on your comments, I concluded that `recolor_hunk()` makes more sense before the "return 0". Even if we end up discarding this series. I think. > > > if (hunk->end == hunk->start) > > /* The user aborted editing by deleting everything */ > > return 0; > > - recolor_hunk(s, hunk); > > - > > > > /* > > * TRANSLATORS: do not translate [y/n] > > @@ -1289,8 +1290,14 @@ static int edit_hunk_loop(struct add_p_state *s, > > "Edit again (saying \"no\" discards!) " > > "[y/n]? ")); > > I think we should make this a three-way choice so the user can choose to > keep their changes or start from a valid hunk. As I said above, I don't object to this, but I'm not convinced it's necessary. > > > if (res < 1) > > - return -1; > > + break; > > } > > + > > + /* Drop a possible edit */ > > + strbuf_setlen(&s->plain, plain_len); > > + strbuf_setlen(&s->colored, colored_len); > > + *hunk = backup; > > + 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..f3206a317b 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 | GIT_TRACE=1 git add -p > > This is still missing "n q". Apart from that the test is looking good. I've been resisting the idea of "completeness", because I think "e y" should also be fine. But I'm not going to resist anymore here :-), since I don't think the test has much more value without "n q". So I'll add it.
Hi Rubén On 24/09/2024 23:54, Rubén Justo wrote: > On Mon, Sep 23, 2024 at 10:07:08AM +0100, phillip.wood123@gmail.com wrote: > > So, to me, it seems sensible to let the user review the faulty patch, > even if it's only to discard it. I agree we could add that option but not as the default >>> If they really want to start over with a fresh patch they still can >>> say "no" to cancel the "edit" and start anew [*]. >> >> This is not very obvious to the user, > > It has been so for a decade... That does not make it obvious though > Keep in mind that this message will probably only be shown very _very_ > rarely to users who are most likely very familiar with (e)dit. I'd argue that users who are not familiar with (e)dit are more likely to make mistakes when editing hunks and are less likely to be able to fix them. >>> + >>> + recolor_hunk(s, hunk); >>> + >> >> This means we're now forking an external process when there is no hunk to >> color. It would be better to avoid that by leaving this code where it was >> and restoring the backup hunk above. > > I don't see that external process. ¿? Oh sorry, I thought we ran interactive.diffFilter on the edited hunk, but we don't. I'll try and find time to fix that. >> This is still missing "n q". Apart from that the test is looking good. > > I've been resisting the idea of "completeness", because I think "e y" > should also be fine. But I'm not going to resist anymore here :-), > since I don't think the test has much more value without "n q". So > I'll add it. The reason I think we should have it is that the tests ought to be testing realistic user input and not rely on getting EOF which is unlikely to happen in real life. Best Wishes Phillip
On Tue, Oct 01, 2024 at 11:02:53AM +0100, Phillip Wood wrote: > I'd argue that users who are not familiar with (e)dit are more likely to > make mistakes when editing hunks and are less likely to be able to fix them. This series is not about making errors more descriptive or making (e)dit more (or less) accessible. Editing the original hunk is already quite challenging and prone to errors. This series is about regaining the possibility for the user to see and correct their mistakes. > > > This is still missing "n q". Apart from that the test is looking good. > > > > I've been resisting the idea of "completeness", because I think "e y" > > should also be fine. But I'm not going to resist anymore here :-), > > since I don't think the test has much more value without "n q". So > > I'll add it. > > The reason I think we should have it is that the tests ought to be testing > realistic user input and not rely on getting EOF which is unlikely to happen > in real life. Sometimes, I use ctrl+d instead of 'q'. So, some of my interactive "add -p" sessions can be described better with "y" than with "y q" or "y n... q". And I'm real ;-) Of course, non-interactively: "e y" or "y" work as expected as the test in this series and others in t3701 demonstrate. Since we already have other tests, I didn't mind adding "n q" in an attempt to move the series forward.
diff --git a/add-patch.c b/add-patch.c index 557903310d..75b5129281 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] @@ -1289,8 +1290,14 @@ static int edit_hunk_loop(struct add_p_state *s, "Edit again (saying \"no\" discards!) " "[y/n]? ")); if (res < 1) - return -1; + break; } + + /* Drop a possible edit */ + strbuf_setlen(&s->plain, plain_len); + strbuf_setlen(&s->colored, colored_len); + *hunk = backup; + 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..f3206a317b 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 | GIT_TRACE=1 git add -p + ) && + test_grep been-here output +' + test_expect_success 'setup patch' ' cat >patch <<-\EOF @@ -1,1 +1,4 @@
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 hunk when the user requests to discard their edits. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- This iteration addresses Phillip's comments: - Ensure that `edit_hunk_manually()` exits with sane values in `hunk`. - Merge the tests into a single one and use a subshell to prevent leaking `EDITOR`. Thanks. add-patch.c | 31 +++++++++++++++++++------------ t/t3701-add-interactive.sh | 13 +++++++++++++ 2 files changed, 32 insertions(+), 12 deletions(-) Range-diff: 1: bcf32d0979 ! 1: 2b55a759d5 add-patch: edit the hunk again @@ add-patch.c: static int edit_hunk_manually(struct add_p_state *s, struct hunk *h /* strip out commented lines */ hunk->start = s->plain.len; for (i = 0; i < s->buf.len; ) { +@@ add-patch.c: 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 @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s, backup = *hunk; @@ t/t3701-add-interactive.sh: 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 -+ echo been-here > "$1" -+ EOF -+ test_set_editor "$(pwd)/fake_editor.sh" -+' -+ +test_expect_success 'editing again works' ' + git reset && -+ test_write_lines e y | GIT_TRACE=1 git add -p && -+ grep found output ++ 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 | GIT_TRACE=1 git add -p ++ ) && ++ test_grep been-here output +' + test_expect_success 'setup patch' '