From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Alban Gruin <alban.gruin@gmail.com>
Cc: git@vger.kernel.org, Phillip Wood <phillip.wood@dunelm.org.uk>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v5 0/2] rebase -i: extend rebase.missingCommitsCheck
Date: Sun, 26 Jan 2020 11:04:11 +0100 (CET) [thread overview]
Message-ID: <nycvar.QRO.7.76.6.2001261043540.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20200125175436.29786-1-alban.gruin@gmail.com>
Hi Alban,
On Sat, 25 Jan 2020, Alban Gruin wrote:
> Changes since v4:
>
> - Added a test to check that no errors are raised when editing the todo
> list after pausing the rebase; it was provided by Phillip Wood.
>
> - Fixed the issue pointed out by this test by creating a backup of the
> todo list even if the edited list is incorrect. This step was moved
> before the user edits the list, so the backup can be created with
> copy_file() instead of todo_list_write_to_file().
Please note that I changed this to always use `todo_list_write_to_file()`
in `js/rebase-i-with-colliding-hash` (see e.g.
https://github.com/gitgitgadget/git/commit/26027625).
The reason is that the copy would be made with _abbreviated_ commit IDs.
And during the rebase, those abbreviated IDs could become non-unique.
I have to admit that I did not follow the evolution of your patch series
terribly closely, but from your description I gather that we want to be
careful to ensure that the `.backup` file is written with non-abbreviated
commit IDs always.
The way I read your patches, they will conflict with
`js/rebase-i-with-colliding-hash`, so maybe it would be a good idea to
base your patches on that branch?
Thanks,
Dscho
>
> - In edit_todo_list(), `incorrect' is set to 0 by default. This only
> concerns the initial edit case; instead of opening and parsing the
> backup, we use directly `old_todo' as a reference.
>
> - Don't check if the "dropped" file flag exists at the initial edit.
>
> Alban Gruin (2):
> sequencer: move check_todo_list_from_file() to rebase-interactive.c
> rebase-interactive: warn if commit is dropped with `rebase
> --edit-todo'
>
> rebase-interactive.c | 90 ++++++++++++++++++++++---
> rebase-interactive.h | 5 ++
> sequencer.c | 51 ++++----------
> sequencer.h | 2 +-
> t/t3404-rebase-interactive.sh | 121 ++++++++++++++++++++++++++++++++++
> 5 files changed, 219 insertions(+), 50 deletions(-)
>
> Range-diff against v4:
> 1: 996045a300 = 1: 996045a300 sequencer: move check_todo_list_from_file() to rebase-interactive.c
> 2: 11b0e1e78c ! 2: 6dbaa8cbe6 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
> @@ Commit message
> `edit_todo_list_advice' is removed from sequencer.c as it is no longer
> used there.
>
> - This changes when a backup of the todo list is made. Until now, it
> - was saved only once, before the initial edit. Now, it is also made if
> - after the user edited the list, if it has no errors or if no commits
> - were dropped and `rebase.missingCommitsCheck' is set. Thus, the
> + This changes when a backup of the todo list is made. Until now, it was
> + saved only once, before the initial edit. Now, it is also made if the
> + original todo list has no errors or no dropped commits. Thus, the
> backup should be error-free. Without this, sequencer_continue()
> (`rebase --continue') could only compare the current todo list against
> the original, unedited list. Before this change, this file was only
> @@ Commit message
> no errors were found at the last edition, so any missing commits here
> have already been picked.
>
> - Four tests are added to t3404. The tests for
> + Five tests are added to t3404. The tests for
> `rebase.missingCommitsCheck = warn' and `rebase.missingCommitsCheck =
> error' have a similar structure. First, we start a rebase with an
> incorrect command on the first line. Then, we edit the todo list,
> @@ Commit message
> has been dropped. Then, the actual rebase takes place. In the third
> test, it is also checked that `--continue' will refuse to resume the
> rebase if commits were dropped. The fourth test checks that no errors
> - are raised when resuming a rebase after resolving a conflict.
> + are raised when resuming a rebase after resolving a conflict, the fifth
> + checks that no errors are raised when editing the todo list after
> + pausing the rebase.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> I don't think the way I create `expect.3' files in "rebase --edit-todo
> @@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list
> + const char *todo_file = rebase_path_todo(),
> + *todo_backup = rebase_path_todo_backup();
> unsigned initial = shortrevisions && shortonto;
> -+ int incorrect = 1;
> ++ int incorrect = 0;
>
> /* If the user is editing the todo list, we first try to parse
> * it. If there is an error, we do not return, because the user
> * might want to fix it in the first place. */
> if (!initial)
> - todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
> -+ incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
> -+
> -+ incorrect |= file_exists(rebase_path_dropped());
> ++ incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
> ++ file_exists(rebase_path_dropped());
>
> if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
> -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))
> @@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list
> - if (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666))
> - return error(_("could not copy '%s' to '%s'."), todo_file,
> - rebase_path_todo_backup());
> -+ if (initial && copy_file(todo_backup, todo_file, 0666))
> -+ return error(_("could not copy '%s' to '%s'."), todo_file, todo_backup);
> ++ if (initial || !incorrect) {
> ++ if (!initial)
> ++ unlink(todo_backup);
> ++
> ++ if (copy_file(todo_backup, todo_file, 0666))
> ++ return error(_("could not copy '%s' to '%s'."), todo_file, todo_backup);
> ++ }
>
> if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
> return -2;
> @@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list
> + } else if (todo_list_check(todo_list, new_todo)) {
> + write_file(rebase_path_dropped(), "");
> + return -4;
> -+ } else {
> -+ todo_list_write_to_file(r, todo_list, todo_backup, shortrevisions, shortonto,
> -+ -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP);
> + }
>
> return 0;
> @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.mi
> + git add file1 &&
> + git rebase --continue
> +'
> ++
> ++test_expect_success 'rebase.missingCommitsCheck = error when editing for a second time' '
> ++ test_config rebase.missingCommitsCheck error &&
> ++ (
> ++ set_fake_editor &&
> ++ FAKE_LINES="1 break 2 3" git rebase -i A D &&
> ++ cp .git/rebase-merge/git-rebase-todo todo &&
> ++ test_must_fail env FAKE_LINES=2 git rebase --edit-todo &&
> ++ GIT_SEQUENCE_EDITOR="cp todo" git rebase --edit-todo &&
> ++ git rebase --continue
> ++ )
> ++'
> +
> test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
> rebase_setup_and_clean abbrevcmd &&
> --
> 2.24.1
>
>
next prev parent reply other threads:[~2020-01-26 10:04 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-17 14:39 [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co Alban Gruin
2019-07-17 14:39 ` [RFC PATCH 1/9] t3404: demonstrate that --edit-todo does not check for dropped commits Alban Gruin
2019-07-18 18:31 ` Junio C Hamano
2019-07-19 18:12 ` Alban Gruin
2019-07-19 19:51 ` Junio C Hamano
2019-07-17 14:39 ` [RFC PATCH 2/9] t3429: demonstrate that rebase exec " Alban Gruin
2019-07-17 14:39 ` [RFC PATCH 3/9] sequencer: update `total_nr' when adding an item to a todo list Alban Gruin
2019-07-18 19:52 ` Junio C Hamano
2019-07-19 18:12 ` Alban Gruin
2019-07-17 14:39 ` [RFC PATCH 4/9] sequencer: update `done_nr' when skipping commands in " Alban Gruin
2019-07-18 19:55 ` Junio C Hamano
2019-07-19 18:13 ` Alban Gruin
2019-07-17 14:39 ` [RFC PATCH 5/9] sequencer: move the code writing total_nr on the disk to a new function Alban Gruin
2019-07-18 20:04 ` Junio C Hamano
2019-07-19 18:14 ` Alban Gruin
2019-07-17 14:39 ` [RFC PATCH 6/9] sequencer: add a parameter to sequencer_continue() to accept a todo list Alban Gruin
2019-07-17 14:39 ` [RFC PATCH 7/9] rebase-interactive: todo_list_check() also uses the done list Alban Gruin
2019-07-17 14:39 ` [RFC PATCH 8/9] rebase-interactive: warn if commit is dropped with --edit-todo Alban Gruin
2019-07-17 14:39 ` [RFC PATCH 9/9] sequencer: have read_populate_todo() check for dropped commits Alban Gruin
2019-07-24 13:29 ` [RFC PATCH 0/9] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co Phillip Wood
2019-07-25 9:01 ` Johannes Schindelin
2019-07-25 20:26 ` Alban Gruin
2019-07-29 9:38 ` Phillip Wood
2019-09-24 20:15 ` Alban Gruin
2019-11-04 9:54 ` [RFC PATCH v2 0/2] rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' Alban Gruin
2019-11-04 9:54 ` [RFC PATCH v2 1/2] t3404: demonstrate that --edit-todo does not check for dropped commits Alban Gruin
2019-11-04 9:54 ` [RFC PATCH v2 2/2] rebase-interactive: warn if commit is dropped with --edit-todo Alban Gruin
2019-11-05 14:20 ` Phillip Wood
2019-12-02 23:47 ` [PATCH v3 0/2] rebase -i: extend rebase.missingCommitsCheck Alban Gruin
2019-12-02 23:47 ` [PATCH v3 1/2] sequencer: move check_todo_list_from_file() to rebase-interactive.c Alban Gruin
2019-12-06 14:38 ` Johannes Schindelin
2019-12-02 23:47 ` [PATCH v3 2/2] rebase-interactive: warn if commit is dropped with `rebase --edit-todo' Alban Gruin
2019-12-04 19:19 ` Junio C Hamano
2019-12-09 16:00 ` Phillip Wood
2020-01-09 21:13 ` Alban Gruin
2020-01-10 17:13 ` Phillip Wood
2020-01-10 21:31 ` Alban Gruin
2020-01-11 14:44 ` Phillip Wood
2019-12-09 16:08 ` Phillip Wood
2019-12-04 21:51 ` [PATCH v3 0/2] rebase -i: extend rebase.missingCommitsCheck Junio C Hamano
2019-12-05 23:15 ` Alban Gruin
2019-12-06 10:41 ` Phillip Wood
2019-12-06 14:30 ` Johannes Schindelin
2020-01-11 17:39 ` [PATCH v4 " Alban Gruin
2020-01-11 17:39 ` [PATCH v4 1/2] sequencer: move check_todo_list_from_file() to rebase-interactive.c Alban Gruin
2020-01-11 17:39 ` [PATCH v4 2/2] rebase-interactive: warn if commit is dropped with `rebase --edit-todo' Alban Gruin
2020-01-19 16:28 ` Phillip Wood
2020-01-25 15:17 ` Alban Gruin
2020-01-25 17:54 ` [PATCH v5 0/2] rebase -i: extend rebase.missingCommitsCheck Alban Gruin
2020-01-25 17:54 ` [PATCH v5 1/2] sequencer: move check_todo_list_from_file() to rebase-interactive.c Alban Gruin
2020-01-25 17:54 ` [PATCH v5 2/2] rebase-interactive: warn if commit is dropped with `rebase --edit-todo' Alban Gruin
2020-01-26 10:04 ` Johannes Schindelin [this message]
2020-01-27 21:39 ` [PATCH v5 0/2] rebase -i: extend rebase.missingCommitsCheck Alban Gruin
2020-01-28 22:46 ` Johannes Schindelin
2020-01-28 21:12 ` [PATCH v6 " Alban Gruin
2020-01-28 21:12 ` [PATCH v6 1/2] sequencer: move check_todo_list_from_file() to rebase-interactive.c Alban Gruin
2020-01-28 21:12 ` [PATCH v6 2/2] rebase-interactive: warn if commit is dropped with `rebase --edit-todo' Alban Gruin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.QRO.7.76.6.2001261043540.46@tvgsbejvaqbjf.bet \
--to=johannes.schindelin@gmx.de \
--cc=alban.gruin@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).