Re: [PATCH 1/3] fast-export: improve argument parsing

[Junio C Hamano <gitster@pobox.com>]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, May 9, 2013 at 6:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> That is the kind of thing that needs to be said, not in the
>> discussion but in the history, either in the log or in a new test,
>> or both.
>
> If only I had known that when I wrote the commit message.

This is exactly what we review patches on the list for.  There is no
need to feel bad.  Nobody expects anybody to be perfect on the first
try.

Other people who may not know what the thought process of the author
was when the patch was written (or those who may not be as familiar
with the area in question as the author of the patch) can sometimes
spot a gap in the thought that is recorded in the patch.

Regarding the text of the patch in question, I am of two minds.  It
may solve the "--import-marks foo" to swap the orders, but I suspect
that may merely be shifting the problem and break rev-list options
(e.g. "--since 3.days") for the same reason.

In the longer term, setup_revisions() may need to allow its callers
to tell it what to do when it sees a parameter that it does not
understand.

There already is a similar cascading mechanism in place from
revision argument parsing to diff argument parsing (look for the
place where diff_opt_parse() is called in revision.c).  It may be
just the matter of adding a "struct option *" to rev_info and
calling parse_options_step() after diff_opt_parse() says "I dunno
about this option".

For the mechanism to be more flexible, we may want to give the
custom option table the caller of setup_revisions() gives us
precedence over revision/diff options, though.