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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 3, 2009, 02:41 UTC
Message-ID: <7vskf4px6j.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1251914223-31435-6-git-send-email-srabbelier@gmail.com>

Sverre Rabbelier <srabbelier@gmail.com> writes:

>   Main difference with v5 is that the syntax is now 'option git ...'
>   as per a discussion with the other fast-import devs. Other options,
>   e.g. 'option hg' are ignored. Also fixed the docs to say that
>   feature commands are allowed before git option commands.

Perhaps the other people have discussed and thought about this much deeper than I have after seeing the above description, but what should the semantics be when you see unknown options?

If "option git something-unknown" is given, it is clear that the tool that generated the stream assumed that such an option exists in the importer; it might appear prudent to abort the operation.

But what about "option hg something"?

It is an indication that the stream is meant to be used with the named option if fed to Hg, but it does not say anything about what should happen when used with other systems. If older versions of Hg that do not grok the given option is expected to abort because they won't be able to change the behaviour to obey the optional semantics demanded by the "option hg something", what should the other VCS do?

Worrying about the above would be unnecessary, if you declare that it is *entirely* optional to understand and obey "option", and ignoring them does not result in a corrupt import at all. I think that is the intent behind "option", as opposed to "feature", and is consistent with the fact that the command line options can override the in-stream settings. In other words, any in-stream instruction that changes the semantics of stream to render it dangerous to be processed by older version of tools would be expressed with "feature", not with "option".

If that is the sensible thing to do, then we obviously should ignore "option hg anything", but at the same time we should ignore "option git we-do-not-know-what-it-does".

But then, the call to die("Unsupported option: %s", option) at the end of parse_one_option() is wrong, isn't it?

I think at least the function should be made conditional to die() if it was called from parse_argv() but simply ignore unknown if it was called from the input stream.

Show 18 quoted lines

> diff --git a/fast-import.c b/fast-import.c
> index 9bf06a4..bad93dc 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2456,11 +2468,31 @@ static void parse_feature(void)
>  
>  	if (!prefixcmp(feature, "date-format=")) {
>  		option_date_format(feature + 12);
> +	} else if (!strcmp("git-options", feature)) {
> +		options_enabled = 1;
>  	} else {
>  		die("This version of fast-import does not support feature %s.", feature);
>  	}
>  }
>  
> +static void parse_option(void)
> +{
> +	char* option = command_buf.buf + 11;

ERROR: "foo* bar" should be "foo *bar"

Show 13 quoted lines

> +
> +	if (!options_enabled)
> +		die("Got option command '%s' before options feature'", option);
> +
> +	if (seen_non_option_command)
> +		die("Got option command '%s' after non-option command", option);
> +
> +	parse_one_option(option);
> +}
> +
> +static void parse_nongit_option(void)
> +{
> +  // do nothing

ERROR: do not use C99 // comments

Show 13 quoted lines

> @@ -2539,9 +2581,18 @@ int main(int argc, const char **argv)
>  			parse_progress();
>  		else if (!prefixcmp(command_buf.buf, "feature "))
>  			parse_feature();
> +		else if (!prefixcmp(command_buf.buf, "option git "))
> +			parse_option();
> +    else if (!prefixcmp(command_buf.buf, "option "))
> +      parse_nongit_option();
>  		else
>  			die("Unsupported command: %s", command_buf.buf);
>  	}
> +
> +	// argv hasn't been parsed yet, do so

ERROR: do not use C99 // comments