Re: [PATCH v3 2/3] fast-import: add option command

From: Sverre Rabbelier <srabbelier@gmail.com>
Date: Aug 14, 2009, 16:37 UTC
Message-ID: <fabb9a1e0908140937h32a2eac7ka88f76aa417fd631@mail.gmail.com>
In-Reply-To: <20090814153707.GT1033@spearce.org>

Heya,

On Fri, Aug 14, 2009 at 08:37, Shawn O. Pearce<spearce@spearce.org> wrote:

Show 9 quoted lines

> Sverre Rabbelier <srabbelier@gmail.com> wrote:
>> +`option`
>> +~~~~~~~~
>> +Processes the specified option so that git fast-import behaves in a
>> +way that suits the frontend's needs.
>> +Note that options specified by the frontend are overridden by any
>> +options the user may specify to git fast-import itself.
>
> Wha?  This disagrees with the code.

It does? It's saying that the user my override what the frontend specifies, which is what the current version does.

> This is a nasty refactoring, I would prefer to see it done in its
> own commit, "move option_import_marks so it can be called during
> command processing".

Ok, will do.

Show 5 quoted lines

>> +     // argv hasn't been parsed yet, do so
>> +     if (!seen_non_option_command)
>> +             parse_argv();
>
> This is too late.

No it's not. Earlier in the patch, at the other 'seen_non_option_command', we call parse_argv() as well (which happens when a non-option command is issued). This statement is here to deal with options that affect an empty stream, such as 'git format-patch --import-marks=marks.old --export-marks=marks.new < /dev/null && test_cmp marks.old marks.new'.

> So yea, I really do think its a good idea for command line options
> to override stream options, despite what Dscho may think.  :-)

Which is what the current version does, yes?

-- 
Cheers,

Sverre Rabbelier