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

From: Junio C Hamano <gitster@pobox.com>
Date: May 10, 2013, 04:49 UTC
Message-ID: <7v4neb5tj5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAMP44s1GAeu5v5xqLu8BgisprmWN=C8zoFYNidoyBPXmu-cvJQ@mail.gmail.com>

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

Show 7 quoted lines

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