From: Ramkumar Ramachandra Date: Wed, 06 Jul 2011 11:55:33 GMT Subject: Re: [PATCH 10/14] revert: Persist data for continuation Message-ID: In-Reply-To: <20110706100119.GG15682@elie> Hi, Jonathan Nieder writes: > Ramkumar Ramachandra wrote: >> 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. >> --- a/builtin/revert.c >> +++ b/builtin/revert.c > [...] >> @@ -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. >> @@ -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. >> @@ -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. >> +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. >> 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. >> +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