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

Junio C Hamano [off-list ref] writes:

Matthieu Moy [off-list ref] writes:

quoted

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

Indeed, I misread the patch. The explanation could be a little bit more
"tired-reviewer-proof" by not using a past tone, perhaps

1. setup_revision, which is changed to ...

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

Hey Matthieu,

On 16 February 2017 at 23:52, Matthieu Moy [off-list ref] wrote:

Indeed, I misread the patch. The explanation could be a little bit more
"tired-reviewer-proof" by not using a past tone, perhaps

1. setup_revision, which is changed to ...

Oh, okay! Sorry about the confusion!

Yes, I used the past perfect tense to refer to changes that were made
in this particular patch!

I will change the message in the next version to something that's in
present tense.

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

-- 

Best Regards,

- Siddharth.