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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 16, 2017, 18:11 UTC
Message-ID: <xmqqwpcqxay0.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <vpqwpcqm69k.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 16 quoted lines

> Siddharth Kannan <kannan.siddharth12@gmail.com> 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.

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).