From: Felipe Contreras Date: Wed, 29 May 2013 13:27:28 GMT Subject: Re: [PATCH v2 8/8] revert/cherry-pick: add --skip option Message-ID: In-Reply-To: On Wed, May 29, 2013 at 7:27 AM, Ramkumar Ramachandra wrote: > Felipe Contreras wrote: >> Akin to 'am --skip' and 'rebase --skip'. > > This ranged-cherry-pick can be useful for small ranges. As pointed > out by others on the list, it hemorrhages memory quite horribly (and > this problem is non-trivial to fix). Perhaps we should document this > in limitations or bugs if we intend to make it more useful? Is there a test-case that triggers this? I don't see why we shouldn't try to fix this. >> diff --git a/builtin/revert.c b/builtin/revert.c >> index d63b4a6..6afd990 100644 >> --- a/builtin/revert.c >> +++ b/builtin/revert.c >> @@ -99,11 +99,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) >> int remove_state = 0; >> int contin = 0; >> int rollback = 0; >> + int skip = 0; > > Ugh, one more integer. Can't we use an OPT_BIT and store the action > in one variable? No hurry ofcourse: just asking. I thought of switching to bit fields, but then I opened builtin/checkout.c to see how many options were implemented, and I saw more ints, so I left it like that. >> @@ -1201,7 +1203,7 @@ static int sequencer_continue(struct replay_opts *opts) >> } >> if (index_differs_from("HEAD", 0)) >> return error_dirty_index(opts); >> - { >> + if (!skip) { >> unsigned char to[20]; >> if (!read_ref("HEAD", to)) >> add_rewritten(todo_list->item->object.sha1, to); > > Couldn't you just say if (skip) todo_list = todo_list -> next? We are already doing that. And after that we need to continue with the rest of the commits. >> + if (setup_rerere(&merge_rr, 0) >= 0) { >> + rerere_clear(&merge_rr); >> + string_list_clear(&merge_rr, 1); >> + } > > Why exactly? Why doesn't rebase --skip 'rerere clear'? It does, and so does 'git am', so why shouldn't 'git cherry-pick'. Also, I think the same should happen in --abort. -- Felipe Contreras