Re: [PATCH 12/18] revert: Make pick_commits functionally act on a commit list
Hi Christian,
Christian Couder writes:
Show 10 quoted lines
> On Thursday 28 July 2011 18:52:25 Ramkumar Ramachandra wrote:
>> + /*
>> + * Decide what to do depending on the arguments; a fresh
>> + * cherry-pick should be handled differently from an existing
>> + * one that is being continued
>> + */
>
> It is strange to me that you add this comment only here and not below in
> cmd_cherry_pick(). So I'd suggest to put it before the definition of
> pick_revisions() instead.
Show 7 quoted lines
>> + if (get_sha1("HEAD", sha1)) {
>> + if (opts->action == REVERT)
>> + die(_("Can't revert as initial commit"));
>> + die(_("Can't cherry-pick into empty head"));
>> + } else
>
> This "else" could be removed.Show 5 quoted lines
>> + save_head(sha1_to_hex(sha1));
>> + save_opts(opts);
>> + save_todo(todo_list, opts);
>
> This save_todo() could be removed too as pick_commits() already does it.
Consider what effect this will have: pick_commits() may die before it
calls the save_todo() in a couple of ways:
1. The opts->allow_ff assertion block fails. Although this can't
happen now (due to parse_args already catching it), it might happen
sometime in the future when we have a public API. The question we
should be asking is: Is it alright to persist the todo in this case?
I'd say no -- the caller needs to be fixed, and persisting the todo
really isn't something I'd expect as an end user at this point.
2. read_and_refresh_cache fails. Same question: Is it alright to
persist the todo in this case? Failing to read or refresh the index
is quite a serious error, and persisting a todo is the last thing a
user would expect at this point anyway. So, no again.
Result: Fixed.
Thanks.
-- Ram
Ramkumar Ramachandra <artagnon@gmail.com>