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

[Ramkumar Ramachandra <artagnon@gmail.com>]

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?

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

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

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

> +       argv[0] = "reset";
> +       argv[1] = "--hard";
> +       argv[2] = "HEAD";
> +       argv[3] = NULL;
> +       ret = run_command_v_opt(argv, RUN_GIT_CMD);

Unrelated to your patch, but any clue why reset doesn't have an api
yet?  Does it leak memory too?