Hi Christian,
On Sat, 7 Jul 2018, Christian Couder wrote:
> On Fri, Jul 6, 2018 at 2:08 PM, Pratik Karki <predatoram...@gmail.com> wrote:
>
> > + switch (opts->type) {
> > + case REBASE_AM:
> > + backend = "git-rebase--am";
> > + backend_func = "git_rebase__am";
> > + break;
> > + case REBASE_INTERACTIVE:
> > + backend = "git-rebase--interactive";
> > + backend_func = "git_rebase__interactive";
> > + break;
> > + case REBASE_MERGE:
> > + backend = "git-rebase--merge";
> > + backend_func = "git_rebase__merge";
> > + break;
> > + case REBASE_PRESERVE_MERGES:
> > + backend = "git-rebase--preserve-merges";
> > + backend_func = "git_rebase__preserve_merges";
> > + break;
> > + default:
> > + BUG("Unhandled rebase type %d", opts->type);
> > + break;
>
> Nit: I think the "break;" line could be removed as the BUG() should always
> exit.
>
> A quick grep shows that there are other places where there is a
> "break;" line after a BUG() though. Maybe one of the #leftoverbits
> could be about removing those "break;" lines.
But what if there is a bug in the BUG() function? Shouldn't we then not
call `die()` directly after the `BUG()`?
Okay, sorry, I let myself loose a little, as I think that we are still
safely in the territory where the code needs to be made correct. We can
nitpick when there are no "biggies" left to comment about. Maybe focus a
little more on whether the code does what it should do, rather than
whether some stylistic guidelines are violated?
I mean, we can argue back and forth about white-space, indentation,
superfluous break statements, etc. But that way, we won't get anywhere
with the builtin rebase.
Let's help Pratik instead to complete his project, okay?
Ciao,
Dscho
