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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 29, 2013, 12:27 UTC
Message-ID: <CALkWK0m02XCVnrnFQ+mF8FFZEMD36_J3Tyjh-E4SuZ++xdcXHQ@mail.gmail.com>
In-Reply-To: <1369799788-24803-9-git-send-email-felipe.contreras@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?

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

Show 9 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?

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

Show 5 quoted lines

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