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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Feb 16, 2017, 16:48 UTC
Message-ID: <vpqwpcqm69k.fsf@anie.imag.fr>
In-Reply-To: <1487258054-32292-2-git-send-email-kannan.siddharth12@gmail.com>

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

Show 11 quoted lines

> 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/