sequencer: rewrite update-refs as user edits todo list

An interactive rebase provides opportunities for the user to edit the
todo list. The --update-refs option initializes the list with some
'update-ref <ref>' steps, but the user could add these manually.
Further, the user could add or remove these steps during pauses in the
interactive rebase.

Add a new method, todo_list_filter_update_refs(), that scans a todo_list
and compares it to the stored update-refs file. There are two actions
that can happen at this point:

1. If a '<ref>/<before>/<after>' triple in the update-refs file does not
   have a matching 'update-ref <ref>' command in the todo-list _and_ the
   <after> value is the null OID, then remove that triple. Here, the
   user removed the 'update-ref <ref>' command before it was executed,
   since if it was executed then the <after> value would store the
   commit at that position.

2. If a 'update-ref <ref>' command in the todo-list does not have a
   matching '<ref>/<before>/<after>' triple in the update-refs file,
   then insert a new one. Store the <before> value to be the current
   OID pointed at by <ref>. This is handled inside of the
   init_update_ref_record() helper method.

We can test that this works by rewriting the todo-list several times in
the course of a rebase. Check that each ref is locked or unlocked for
updates after each todo-list update. We can also verify that the ref
update fails if a concurrent process updates one of the refs after the
rebase process records the "locked" ref location.

To help these tests, add a new 'set_replace_editor' helper that will
replace the todo-list with an exact file.

Reported-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/rebase-interactive.c b/rebase-interactive.c
index 1ff07647af3472f827be1c5614a8638160f7788d..7407c593191679c757573e567a28f35c65c9cb33 100644 (file)
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -146,6 +146,12 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
                return -4;
        }
 
+       /*
+        * See if branches need to be added or removed from the update-refs
+        * file based on the new todo list.
+        */
+       todo_list_filter_update_refs(r, new_todo);
+
        return 0;
 }
 

diff --git a/sequencer.c b/sequencer.c
index 98111e394d51cd0ecd9cacf606e07a68b2d08570..20ca7b44353e00016d004173db9e910803713201 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -4168,6 +4168,102 @@ cleanup:
        return result;
 }
 
+/*
+ * Parse the update-refs file for the current rebase, then remove the
+ * refs that do not appear in the todo_list (and have not had updated
+ * values stored) and add refs that are in the todo_list but not
+ * represented in the update-refs file.
+ *
+ * If there are changes to the update-refs list, then write the new state
+ * to disk.
+ */
+void todo_list_filter_update_refs(struct repository *r,
+                                 struct todo_list *todo_list)
+{
+       int i;
+       int updated = 0;
+       struct string_list update_refs = STRING_LIST_INIT_DUP;
+
+       sequencer_get_update_refs_state(r->gitdir, &update_refs);
+
+       /*
+        * For each item in the update_refs list, if it has no updated
+        * value and does not appear in the todo_list, then remove it
+        * from the update_refs list.
+        */
+       for (i = 0; i < update_refs.nr; i++) {
+               int j;
+               int found = 0;
+               const char *ref = update_refs.items[i].string;
+               size_t reflen = strlen(ref);
+               struct update_ref_record *rec = update_refs.items[i].util;
+
+               /* OID already stored as updated. */
+               if (!is_null_oid(&rec->after))
+                       continue;
+
+               for (j = 0; !found && j < todo_list->total_nr; j++) {
+                       struct todo_item *item = &todo_list->items[j];
+                       const char *arg = todo_list->buf.buf + item->arg_offset;
+
+                       if (item->command != TODO_UPDATE_REF)
+                               continue;
+
+                       if (item->arg_len != reflen ||
+                           strncmp(arg, ref, reflen))
+                               continue;
+
+                       found = 1;
+               }
+
+               if (!found) {
+                       free(update_refs.items[i].string);
+                       free(update_refs.items[i].util);
+
+                       update_refs.nr--;
+                       MOVE_ARRAY(update_refs.items + i, update_refs.items + i + 1, update_refs.nr - i);
+
+                       updated = 1;
+                       i--;
+               }
+       }
+
+       /*
+        * For each todo_item, check if its ref is in the update_refs list.
+        * If not, then add it as an un-updated ref.
+        */
+       for (i = 0; i < todo_list->total_nr; i++) {
+               struct todo_item *item = &todo_list->items[i];
+               const char *arg = todo_list->buf.buf + item->arg_offset;
+               int j, found = 0;
+
+               if (item->command != TODO_UPDATE_REF)
+                       continue;
+
+               for (j = 0; !found && j < update_refs.nr; j++) {
+                       const char *ref = update_refs.items[j].string;
+
+                       found = strlen(ref) == item->arg_len &&
+                               !strncmp(ref, arg, item->arg_len);
+               }
+
+               if (!found) {
+                       struct string_list_item *inserted;
+                       struct strbuf argref = STRBUF_INIT;
+
+                       strbuf_add(&argref, arg, item->arg_len);
+                       inserted = string_list_insert(&update_refs, argref.buf);
+                       inserted->util = init_update_ref_record(argref.buf);
+                       strbuf_release(&argref);
+                       updated = 1;
+               }
+       }
+
+       if (updated)
+               write_update_refs_state(&update_refs);
+       string_list_clear(&update_refs, 1);
+}
+
 static int do_update_ref(struct repository *r, const char *refname)
 {
        struct string_list_item *item;

diff --git a/sequencer.h b/sequencer.h
index 8216b62f47e12a0d900257465516fe985eb1dd98..563fe5993340a0d0a18b148d4280c1d5a3f8d307 100644 (file)
--- a/sequencer.h
+++ b/sequencer.h
@@ -133,6 +133,18 @@ void todo_list_release(struct todo_list *todo_list);
 const char *todo_item_get_arg(struct todo_list *todo_list,
                              struct todo_item *item);
 
+/*
+ * Parse the update-refs file for the current rebase, then remove the
+ * refs that do not appear in the todo_list (and have not had updated
+ * values stored) and add refs that are in the todo_list but not
+ * represented in the update-refs file.
+ *
+ * If there are changes to the update-refs list, then write the new state
+ * to disk.
+ */
+void todo_list_filter_update_refs(struct repository *r,
+                                 struct todo_list *todo_list);
+
 /* Call this to setup defaults before parsing command line options */
 void sequencer_init_config(struct replay_opts *opts);
 int sequencer_pick_revisions(struct repository *repo,

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index ec6b9b107da4eaab70e221b07c25c9f949f09b14..b57541356bd03d139334a144339b01a92be2c70a 100644 (file)
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -207,3 +207,18 @@ check_reworded_commits () {
                >reword-log &&
        test_cmp reword-expected reword-log
 }
+
+# usage: set_replace_editor <file>
+#
+# Replace the todo file with the exact contents of the given file.
+set_replace_editor () {
+       cat >script <<-\EOF &&
+       cat FILENAME >"$1"
+
+       echo 'rebase -i script after editing:'
+       cat "$1"
+       EOF
+
+       sed -e "s/FILENAME/$1/g" <script | write_script fake-editor.sh &&
+       test_set_editor "$(pwd)/fake-editor.sh"
+}

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 2fffcf5d5fbebbfa78f7d506001e89c48ecba187..bebf9b4def7393ca8d9ab6a459eb001e28fee1a5 100755 (executable)
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1834,6 +1834,145 @@ test_expect_success '--update-refs updates refs correctly' '
        test_cmp_rev HEAD refs/heads/no-conflict-branch
 '
 
+test_expect_success 'respect user edits to update-ref steps' '
+       git checkout -B update-refs-break no-conflict-branch &&
+       git branch -f base HEAD~4 &&
+       git branch -f first HEAD~3 &&
+       git branch -f second HEAD~3 &&
+       git branch -f third HEAD~1 &&
+       git branch -f unseen base &&
+
+       # First, we will add breaks to the expected todo file
+       cat >fake-todo-1 <<-EOF &&
+       pick $(git rev-parse HEAD~3)
+       break
+       update-ref refs/heads/second
+       update-ref refs/heads/first
+
+       pick $(git rev-parse HEAD~2)
+       pick $(git rev-parse HEAD~1)
+       update-ref refs/heads/third
+
+       pick $(git rev-parse HEAD)
+       update-ref refs/heads/no-conflict-branch
+       EOF
+
+       # Second, we will drop some update-refs commands (and move one)
+       cat >fake-todo-2 <<-EOF &&
+       update-ref refs/heads/second
+
+       pick $(git rev-parse HEAD~2)
+       update-ref refs/heads/third
+       pick $(git rev-parse HEAD~1)
+       break
+
+       pick $(git rev-parse HEAD)
+       EOF
+
+       # Third, we will:
+       # * insert a new one (new-branch),
+       # * re-add an old one (first), and
+       # * add a second instance of a previously-stored one (second)
+       cat >fake-todo-3 <<-EOF &&
+       update-ref refs/heads/unseen
+       update-ref refs/heads/new-branch
+       pick $(git rev-parse HEAD)
+       update-ref refs/heads/first
+       update-ref refs/heads/second
+       EOF
+
+       (
+               set_replace_editor fake-todo-1 &&
+               git rebase -i --update-refs primary &&
+
+               # These branches are currently locked.
+               for b in first second third no-conflict-branch
+               do
+                       test_must_fail git branch -f $b base || return 1
+               done &&
+
+               set_replace_editor fake-todo-2 &&
+               git rebase --edit-todo &&
+
+               # These branches are currently locked.
+               for b in second third
+               do
+                       test_must_fail git branch -f $b base || return 1
+               done &&
+
+               # These branches are currently unlocked for checkout.
+               for b in first no-conflict-branch
+               do
+                       git worktree add wt-$b $b &&
+                       git worktree remove wt-$b || return 1
+               done &&
+
+               git rebase --continue &&
+
+               set_replace_editor fake-todo-3 &&
+               git rebase --edit-todo &&
+
+               # These branches are currently locked.
+               for b in second third first unseen
+               do
+                       test_must_fail git branch -f $b base || return 1
+               done &&
+
+               # These branches are currently unlocked for checkout.
+               for b in no-conflict-branch
+               do
+                       git worktree add wt-$b $b &&
+                       git worktree remove wt-$b || return 1
+               done &&
+
+               git rebase --continue
+       ) &&
+
+       test_cmp_rev HEAD~2 refs/heads/third &&
+       test_cmp_rev HEAD~1 refs/heads/unseen &&
+       test_cmp_rev HEAD~1 refs/heads/new-branch &&
+       test_cmp_rev HEAD refs/heads/first &&
+       test_cmp_rev HEAD refs/heads/second &&
+       test_cmp_rev HEAD refs/heads/no-conflict-branch
+'
+
+test_expect_success '--update-refs: check failed ref update' '
+       git checkout -B update-refs-error no-conflict-branch &&
+       git branch -f base HEAD~4 &&
+       git branch -f first HEAD~3 &&
+       git branch -f second HEAD~2 &&
+       git branch -f third HEAD~1 &&
+
+       cat >fake-todo <<-EOF &&
+       pick $(git rev-parse HEAD~3)
+       break
+       update-ref refs/heads/first
+
+       pick $(git rev-parse HEAD~2)
+       update-ref refs/heads/second
+
+       pick $(git rev-parse HEAD~1)
+       update-ref refs/heads/third
+
+       pick $(git rev-parse HEAD)
+       update-ref refs/heads/no-conflict-branch
+       EOF
+
+       (
+               set_replace_editor fake-todo &&
+               git rebase -i --update-refs base
+       ) &&
+
+       # At this point, the values of first, second, and third are
+       # recorded in the update-refs file. We will force-update the
+       # "second" ref, but "git branch -f" will not work because of
+       # the lock in the update-refs file.
+       git rev-parse third >.git/refs/heads/second &&
+
+       git rebase --continue 2>err &&
+       grep "update_ref failed for ref '\''refs/heads/second'\''" err
+'
+
 # This must be the last test in this file
 test_expect_success '$EDITOR and friends are unchanged' '
        test_editor_unchanged