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

Matthieu Moy [off-list ref] writes:

Siddharth Kannan [off-list ref] writes:

quoted

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.

Actually, I think you misread the patch and explanation.
handle_revision_opt() used to be responsible for stuffing unknown
ones to unkv[] array passed from the caller even when it returns 0
(i.e. "I do not know what they are" case, as opposed to "I know what
they are, I am not handling them here and leaving them in unkv[]"
case--the latter returns non-zero).  The first hunk makes the
function stop doing so, and to compensate, the second hunk, which is
in setup_revisions() that calls the function, now makes the caller
do the equivalent "argv[left++] = arg" there after it receives 0.

So "Changes have been made" to setup_revisions() to compensate for
the change of behaviour in the called function.

The enumerated point 2. (not in your response) explains why such a
corresponding compensatory change is not there for the other caller
of this function whose behaviour has changed.

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.

That is a very good point to stress, but 1. is exactly to avoid
breakage in this individual step (and 2. is an explanation why the
change does not break the other caller).