Re: [PATCH v2 8/8] revert/cherry-pick: add --skip option

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 29, 2013, 13:27 UTC
Message-ID: <CAMP44s1i9H9cp_RoOo-rDWL6b76=zxbwC19=DnUayK_Ti2CXFA@mail.gmail.com>
In-Reply-To: <CALkWK0m02XCVnrnFQ+mF8FFZEMD36_J3Tyjh-E4SuZ++xdcXHQ@mail.gmail.com>

On Wed, May 29, 2013 at 7:27 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 7 quoted lines

> 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.

Show 12 quoted lines

>> 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.

Show 11 quoted lines

>> @@ -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.

Show 6 quoted lines

>> +       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