Re: [PATCH 10/14] revert: Persist data for continuation

Hi,

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than
one commit, 2010-06-02), a single invocation of "git cherry-pick" or
"git revert" can perform picks of several individual commits.  To
implement features like "--continue" to continue the whole operation,
we will need to store some information about the state and the plan at
the beginning.  Introduce a ".git/sequencer/head" file to store this
state, and ".git/sequencer/todo" file to store the plan.

I think I remember Junio being curious about which commit is stored in
"head"; this might be a good place to put a reminder so future readers
don't have to be confused.

Oops, I totally forgot -- sorry Junio.
He suggested that we store the corresponding ref also somewhere.  Have
to think about this some more before the next iteration.

quoted

--- a/builtin/revert.c
+++ b/builtin/revert.c

[...]

quoted

@@ -25,6 +26,10 @@

  * Copyright (c) 2005 Junio C Hamano
  */

+#define SEQ_DIR              "sequencer"
+#define SEQ_HEAD_FILE        "sequencer/head"
+#define SEQ_TODO_FILE        "sequencer/todo"

Yay. :)

Sorry it took me so long to understand this.  Your elaborate
explanation last time drove the point home.

quoted

@@ -417,7 +422,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)

                      return error(_("Your index file is unmerged."));
      } else {
              if (get_sha1("HEAD", head))
-                     return error(_("You do not have a valid HEAD"));
+                     return error(_("Can't %s on an unborn branch"), me);

Remember that "me" is an untranslated command name, and see also
http://thread.gmane.org/gmane.comp.version-control.git/153026

Perhaps it would make sense to do something like

       if (get_sha1("HEAD", head)) {
               if (opts->action == REVERT)
                       return error(_("can't revert as initial commit"));
               return error(_("cherry-pick into empty head not supported yet"));
       }

In a way they feel like different operations, anyway.  On the other
hand, there's no reason I can think of not to allow reverting a patch
that only removes files as the initial commit other than not having
implemented it.

Okay.  That would be unrelated to this patch though -- I'll make it a
separate patch and move it to the beginning of the series.

quoted

@@ -578,10 +583,106 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)

      rollback_lock_file(&index_lock);
 }

-static int pick_commits(struct replay_opts *opts)
+static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
+                     struct replay_opts *opts)
+{
+     struct commit_list *cur = NULL;
+     struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+     const char *sha1_abbrev = NULL;
+     const char *action;
+
+     action = (opts->action == REVERT ? "revert" : "pick");
+     for (cur = todo_list; cur; cur = cur->next) {
+             sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
+             if (get_message(cur->item, &msg))
+                     die(_("Cannot get commit message for %s"), sha1_abbrev);
+             strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);

Maybe some word like "command", "insn", or "keyword" would be more
suggestive than "action".  It also might be worth mentioning somewhere
(in the commit message?) that this format is inspired by
rebase--interactive's insn sheet.

Okay, will do.

The operation that could be exposed does not include get_revision,
does it?

 /*
 * Example:
 *
 *     struct commit_list *list;
 *     struct commit_list **next = &list;
 *
 *     next = commit_list_append(c1, next);
 *     next = commit_list_append(c2, next);
 *     *next = NULL;
 *     assert(commit_list_count(list) == 2);
 *     return list;
 *
 * Don't forget to NULL-terminate!
 */
 struct commit_list **commit_list_append(struct commit *commit,
                                       struct commit_list **next)
 {
       struct commit_list *new = xmalloc(sizeof(*new_list));
       new->item = commit;
       *next = new;
       return &new->next;
 }

I would have done this, but I was worried about what the NULL
termination would mean API-wise.  In retrospect, a lot of APIs
described in Documentation/technical are pretty non-trivial, and it's
not obvious how to use it without the documentation.  Would it be okay
to expose this in commit.c and write some documentation?  I already
have two callers.

quoted

+static void create_seq_dir(void)
+{
+     if (file_exists(git_path(SEQ_DIR))) {
+             if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
+                     die(_("Could not remove %s"), git_path(SEQ_DIR));
+     } else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
+             die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
+}

A local variable to cache the git_path result would make this much
easier to read.

Okay.

quoted

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
new file mode 100644

Should "chmod +x" so the test can be run directly (I forget to do that
all the time).

Good catch.

quoted

+test_expect_success 'cherry-pick cleans up sequencer directory upon success' '
+     pristine_detach initial &&
+     git cherry-pick initial..picked &&
+     test_path_is_missing .git/sequencer
+'

Thanks for thinking about these things.  Maybe another test
demonstrating that the .git/sequencer directory is left behind on
failure would help put this in context.

Good suggestion.

-- Ram