Re: [PATCH v6 14/16] rebase-interactive: rewrite edit_todo_list() to handle the initial edit

Hi Alban

On 06/02/2019 21:11, Alban Gruin wrote:

Hi Phillip,

I’ve just reread this message and have a couple of additionnal comments.

Le 01/02/2019 à 12:03, Phillip Wood a écrit :

quoted

Hi Alban

This looks good apart from some missing error handling.

On 29/01/2019 15:01, Alban Gruin wrote:

quoted

edit_todo_list() is changed to work on a todo_list, and to handle the
initial edition of the todo list (ie. making a backup of the todo
list).

It does not check for dropped commits yet, as todo_list_check() does not
take the commits that have already been processed by the rebase (ie. the
todo list is edited in the middle of a rebase session).

Signed-off-by: Alban Gruin <redacted>
---
  builtin/rebase--interactive.c | 24 +++++++++++++++++-
  rebase-interactive.c          | 48 ++++++++++++++++++-----------------
  rebase-interactive.h          |  4 ++-
  sequencer.c                   |  3 +--
  sequencer.h                   |  1 +
  5 files changed, 53 insertions(+), 27 deletions(-)

diff --git a/builtin/rebase--interactive.c

b/builtin/rebase--interactive.c
index 2dbf8fc08b..645ac587f7 100644

--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c

@@ -13,6 +13,28 @@ static GIT_PATH_FUNC(path_state_dir, "rebase-merge/")

  static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
  static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive")
  +static int edit_todo_file(unsigned flags)
+{
+    const char *todo_file = rebase_path_todo();
+    struct todo_list todo_list = TODO_LIST_INIT,
+        new_todo = TODO_LIST_INIT;
+
+    if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
+        return error_errno(_("could not read '%s'."), todo_file);
+
+    strbuf_stripspace(&todo_list.buf, 1);
+    if (!edit_todo_list(the_repository, &todo_list,
+                &new_todo, NULL, NULL, flags) &&
+        todo_list_write_to_file(the_repository, &new_todo, todo_file,
NULL, NULL,
+                    -1, flags & ~(TODO_LIST_SHORTEN_IDS)) < 0)
+        return error_errno(_("could not write '%s'"), todo_file);

If edit_todo_list() fails then the function returns 0. I think you need
to do

if (edit_todo_list() || todo_list_write_file())
    return error...

todo_list_write_file() forwards the return value of write_message()
which is 0/-1 so there is no need for the '< 0'

With your proposed condition, if edit_todo_list() fails, the error
"could not write '%s'" will be shown, if I’m not mistaken.

Yes, you're right but as edit_todo_list() will have already printed an
error I decided it didn't matter too much, but it would be better to
avoid it as you suggest.

But in my
version, if edit_todo_list() fails, the return value is 0.  Perhaps I
should write something like this instead:

    int res = 0;
    …
    res = edit_todo_list();
    if (!res && todo_list_write_to_file())
        return error;

If you did
	ret = error...;
instead then we always free the todo lists before exiting the function.

    …
    return res;

quoted

quoted

+
+    todo_list_release(&todo_list);
+    todo_list_release(&new_todo);
+
+    return 0;
+}
+
  static int get_revision_ranges(const char *upstream, const char *onto,
                     const char **head_hash,
                     char **revisions, char **shortrevisions)

@@ -242,7 +264,7 @@ int cmd_rebase__interactive(int argc, const char

**argv, const char *prefix)
          break;
      }
      case EDIT_TODO:
-        ret = edit_todo_list(the_repository, flags);
+        ret = edit_todo_file(flags);
          break;
      case SHOW_CURRENT_PATCH: {
          struct child_process cmd = CHILD_PROCESS_INIT;

diff --git a/rebase-interactive.c b/rebase-interactive.c
index 807f8370db..3301efbe52 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c

@@ -87,35 +87,37 @@ void append_todo_help(unsigned keep_empty, int

command_count,
      }
  }
  -int edit_todo_list(struct repository *r, unsigned flags)
+int edit_todo_list(struct repository *r, struct todo_list *todo_list,
+           struct todo_list *new_todo, const char *shortrevisions,
+           const char *shortonto, unsigned flags)
  {
      const char *todo_file = rebase_path_todo();
-    struct todo_list todo_list = TODO_LIST_INIT;
-    int res = 0;
-
-    if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
-        return error_errno(_("could not read '%s'."), todo_file);
-
-    strbuf_stripspace(&todo_list.buf, 1);
-    todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list);
-    if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL,
-1,
-                    flags | TODO_LIST_SHORTEN_IDS |
TODO_LIST_APPEND_TODO_HELP)) {
-        todo_list_release(&todo_list);
-        return -1;
+    unsigned initial = shortrevisions && shortonto;
+
+    if (initial) {
+        todo_list_write_to_file(r, todo_list, todo_file,
shortrevisions, shortonto,
+                    -1, flags | TODO_LIST_SHORTEN_IDS |
TODO_LIST_APPEND_TODO_HELP);

This has lost the error handling when we cannot write the file

quoted

+
+        if (copy_file(rebase_path_todo_backup(), todo_file, 0666))
+            return error(_("could not copy '%s' to '%s'."), todo_file,
+                     rebase_path_todo_backup());
+    } else {
+        todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
+        todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1,
+                    flags | TODO_LIST_SHORTEN_IDS |
TODO_LIST_APPEND_TODO_HELP);

error handling again

I agree for todo_list_write_to_file(), but todo_list_parse_insn_buffer()
already shows an error, and here it should not return -- we want to edit
the todo list to remove an error, but git would fail because the todo
list has an error.

Ah yes, that is what the original was doing


Best Wishes

Phillip

-- Alban