Re: [RFC/PATCH 07/18] revert: put option information in an option struct

Christian Couder <chriscool@tuxfamily.org> writes:

This probably is not _needed_ but something you thought would be a good thing to do while at it.

You forgot to mention something more important, though. You added two extra arguments to revert_or_cherry_pick, neither of which I agree with.

 * it is a regression to call the first extra argument "int revert";
   (action == REVERT) was more readable.

 * "int edit" is ill thought out; it is about giving the default of "edit"
   to revert_or_cherry_pick() depending on what action it is going to
   take.  In this particular case, the logic for the default of "edit" is
   trivial and localized (it is 0 unless we are interactive revert), so I
   would drop the argument and have default logic immediately after
   "memset(&info, 0, sizeof(info))".

   If there were many such args-info elements whose default have to be
   different depending on the action, the caller should be passing an
   instance of "struct args_info" with the default, and have the parser
   to update the default supplied.  I don't think it is warranted in this
   case.

By the way, "infos" is an eyesore at least to me. Any data you work on is information, naming a variable "struct args_info info", unless it primarily works on that "args_info" and not any other kinds of info (like "struct rev_info revs"), is like calling a variable "var", adding _no_ useful information.