Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx> writes: > Siddharth Kannan <kannan.siddharth12@xxxxxxxxx> 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).