Subject: Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option

From: Matthieu Moy <[email-masked]>
Date: Thu, 16 Feb 2017 17:48:07 +0100

Siddharth Kannan <[email-masked]> writes:

> handle_revision_opt() tries to recognize and handle the given argument. If an
> option was unknown to it, it used to add the option to unkv[(*unkc)++].  This
> increment of unkc causes the variable in the caller to change.
>
> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
> This is now the responsibility of the caller.
>
> There are two callers of this function:
>
> 1. setup_revision: Changes have been made so that setup_revision will now
> update the unknown option in argv

You're writting "Changes have been made", but I did not see any up to
this point in the series.

We write patch series so that they are bisectable, i.e. each commit
should be correct (compileable, pass tests, consistent
documentation, ...). Here, it seems you are introducing a breakage to
repair it later.

Other that bisectability, this makes review harder: at this point the
reader knows it's broken, guesses that it will be repaired later, but
does not know in which patch.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/