add p in C tweaks #702
Closed
add p in C tweaks #702
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This simplifies the code slightly, especially the third case where hunk_nr was incremented a few lines before ALLOC_GROW(). Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
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@dunelm.org.uk>
dscho
approved these changes
Aug 14, 2020
| ALLOW_SEARCH_AND_GOTO = 1 << 4, | ||
| ALLOW_SPLIT = 1 << 5, | ||
| ALLOW_EDIT = 1 << 6 | ||
| } permitted = 0; |
There was a problem hiding this comment.
Nice idea.
It might make sense to split this commit up into two commits, though: one that avoids duplicating the permission checking, and then the one that fixes the e case (by adding ALLOW_EDIT).
There was a problem hiding this comment.
It might make sense to split this commit up into two commits, though: one that avoids duplicating the permission checking, and then the one that fixes the
ecase (by addingALLOW_EDIT).
I know what you mean - the bug fix gets a bit lost in all the other changes, but it seems a bit strange to convert everything apart from edit first - I'm not sure ...
|
/submit |
|
Submitted as pull.702.git.1597670589.gitgitgadget@gmail.com |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| const char *deleted = NULL, *added = NULL, *mode_change = NULL; | ||
|
|
||
| if (!eol) | ||
| eol = pend; |
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This simplifies the code slightly, especially the third case where
> hunk_nr was incremented a few lines before ALLOC_GROW().
OK, we lose memset()s that appear to be separate but is a part of
adding more elements. Makes sense.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> add-patch.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index f899389e2c..a15fa407be 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -457,11 +457,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> eol = pend;
>
> if (starts_with(p, "diff ")) {
> - s->file_diff_nr++;
> - ALLOC_GROW(s->file_diff, s->file_diff_nr,
> + ALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,
> file_diff_alloc);
> file_diff = s->file_diff + s->file_diff_nr - 1;
> - memset(file_diff, 0, sizeof(*file_diff));
> hunk = &file_diff->head;
> hunk->start = p - plain->buf;
> if (colored_p)
> @@ -483,11 +481,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> */
> hunk->splittable_into++;
>
> - file_diff->hunk_nr++;
> - ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr,
> + ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
> file_diff->hunk_alloc);
> hunk = file_diff->hunk + file_diff->hunk_nr - 1;
> - memset(hunk, 0, sizeof(*hunk));
>
> hunk->start = p - plain->buf;
> if (colored)
> @@ -511,7 +507,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> if (file_diff->mode_change)
> BUG("double mode change?\n\n%.*s",
> (int)(eol - plain->buf), plain->buf);
> - if (file_diff->hunk_nr++)
> + if (file_diff->hunk_nr)
> BUG("mode change in the middle?\n\n%.*s",
> (int)(eol - plain->buf), plain->buf);
>
> @@ -520,9 +516,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> * is _part of_ the header "hunk".
> */
> file_diff->mode_change = 1;
> - ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr,
> + ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
> file_diff->hunk_alloc);
> - memset(file_diff->hunk, 0, sizeof(struct hunk));
> file_diff->hunk->start = p - plain->buf;
> if (colored_p)
> file_diff->hunk->colored_start =
|
This branch is now known as |
|
This patch series was integrated into seen via git@36c9a77. |
|
This patch series was integrated into seen via git@62669af. |
| const char *deleted = NULL, *added = NULL, *mode_change = NULL; | ||
|
|
||
| if (!eol) | ||
| eol = pend; |
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Phillip,
On Mon, 17 Aug 2020, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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@dunelm.org.uk>
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
>
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
This patch series was integrated into seen via git@57410d4. |
|
This patch series was integrated into seen via git@36b9d8f. |
|
This patch series was integrated into seen via git@3697d0b. |
|
This patch series was integrated into seen via git@10ba38b. |
|
This patch series was integrated into seen via git@4f5a462. |
|
This patch series was integrated into seen via git@fb7ada4. |
|
This patch series was integrated into next via git@6cd6275. |
|
This patch series was integrated into seen via git@4d50462. |
|
This patch series was integrated into seen via git@dfa7cf8. |
|
This patch series was integrated into seen via git@e13eeca. |
|
This patch series was integrated into seen via git@cce5178. |
|
This patch series was integrated into next via git@cce5178. |
|
This patch series was integrated into master via git@cce5178. |
|
Closed via cce5178. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A code cleanup and small bug fix for the C version of add -p
dscho has pointed out that the bug fix in the second patch gets lost in the other changes and suggested adding the last member of the enum (which fixes the bug with handling 'e') as a separate patch. I'm unsure as it feels odd to split up the introduction of the flags - I'd be interested to hear what others think.
Cc: Johannes Schindelin Johannes.Schindelin@gmx.de