Re: [PATCH 12/18] revert: Make pick_commits functionally act on a commit list

Hi Christian,

Christian Couder writes:

On Thursday 28 July 2011 18:52:25 Ramkumar Ramachandra wrote:

quoted

+     /*
+      * 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.

Fixed.

quoted

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

Fixed.

quoted

+             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