Re: [PATCH 10/14] revert: Persist data for continuation
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- Jul 6, 2011, 11:55 UTC
- Message-ID
- <CALkWK0kztvKjfEdx-FTAvU7rNBO2oke8b-Wp3u=yxE9K0WvOCA@mail.gmail.com>
- In-Reply-To
- <20110706100119.GG15682@elie>
Hi,
Jonathan Nieder writes:
Show 12 quoted lines
> 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.
Show 12 quoted lines
>> --- 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.
Show 22 quoted lines
>> @@ -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.
Show 24 quoted lines
>> @@ -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.
Show 25 quoted lines
> 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.
Show 11 quoted lines
>> +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.
Show 5 quoted lines
>> 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.
Show 9 quoted lines
>> +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