Re: [PATCH v7 5/6] fast-import: add option command

From: Sverre Rabbelier <srabbelier@gmail.com>
Date: Sep 12, 2009, 19:40 UTC
Message-ID: <fabb9a1e0909121240q67dbcd11ube72dcbbd276168e@mail.gmail.com>
In-Reply-To: <20090912190416.GS1033@spearce.org>

Heya,

On Sat, Sep 12, 2009 at 21:04, Shawn O. Pearce <spearce@spearce.org> wrote:

> The more I think about this, I may have to agree with Ian, I'm not
> sure option makes much sense.

Perhaps instead we can replace the option series with a few extra features? That is 'feature git-quiet' (or maybe just 'feature quiet') and 'feature git-marks' (or just 'feature marks', since its' fairly generic)?

Show 6 quoted lines

> But what should happen if "option import-marks=bleh" isn't
> understood by fast-import?  Wouldn't the stream be useless anyway,
> because the marks it assumes aren't present?  Or worse, "option
> export-marks=bleh" isn't recognized.  The stream imports, but any
> marks it was supposed to store for the frontend to reuse later
> are gone.

This was why originally we aborted when we see an unrecognized option, I agree that due to how the series has evolved this is not such a good idea anymore.

Show 25 quoted lines

> @@ -2485,6 +2518,27 @@ static int git_pack_config(const char *k, const char *v, void *cb)
>>  static const char fast_import_usage[] =
>>  "git fast-import [--date-format=f] [--max-pack-size=n] [--depth=n] [--active-branches=n] [--export-marks=marks.file]";
>>
>> +static void parse_argv(void)
>> +{
>> +     unsigned int i;
>> +
>> +     for (i = 1; i < global_argc; i++) {
>> +             const char *a = global_argv[i];
>> +
>> +             if (*a != '-' || !strcmp(a, "--"))
>> +                     break;
>> +
>> +             /* error on unknown options */
>> +             parse_one_option(a + 2, 0);
>> +     }
>> +     if (i != global_argc)
>> +             usage(fast_import_usage);
>> +
>> +     seen_non_option_command = 1;
>
> So if I pass a single command line option, like --export-marks,
> we die if we see an "option git " inside of the stream?  That's not
> what we wanted to do.

Nope, parse_argv isn't called until after we encounter a non-git option. I think there's a test that makes sure this works too.

> I thought on fast-import list we agreed that the syntax of option was:

Right.

> So what is this parse_nongit_option() for, other than to obfuscate
> the code?  Can't we handle all of this in parse_option, have it
> check the VCS tag, and return early there?

I wanted to make it obvious that we ignore non-git options; depending on whether we want to keep the option part of this series in the first place I'll either handle it all in parse_option or drop those patches.

-- 
Cheers,

Sverre Rabbelier