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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Aug 14, 2009, 15:37 UTC
Message-ID: <20090814153707.GT1033@spearce.org>
In-Reply-To: <1250190156-4752-3-git-send-email-srabbelier@gmail.com>

Sverre Rabbelier <srabbelier@gmail.com> wrote:

Show 6 quoted lines

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

> +static void read_marks(void)
> +{
> +	char line[512];
> +	FILE *f = fopen(input_file, "r");

...

> +static void option_import_marks(const char *marks)
>  {
> -	char line[512];
> -	FILE *f = fopen(input_file, "r");

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

Show 13 quoted lines

> @@ -2517,9 +2556,16 @@ int main(int argc, const char **argv)
>  			parse_checkpoint();
>  		else if (!prefixcmp(command_buf.buf, "progress "))
>  			parse_progress();
> +		else if (!prefixcmp(command_buf.buf, "option "))
> +			parse_option();
>  		else
>  			die("Unsupported command: %s", command_buf.buf);
>  	}
> +
> +	// argv hasn't been parsed yet, do so
> +	if (!seen_non_option_command)
> +		parse_argv();

This is too late. Options like --date-format or --max-pack-size or --depth or --active-branches all influence the command processing above. Parsing these at the end means they have no affect on the import, which is wrong.

Oh, and --active-branches or --max-pack-size or --depth are really good examples of things that maybe you do want to override on the command line. They have impact only on memory usage of the running import process, and the local disk usage. Maybe the frontend has given too many active branches for your physical memory, and you want a lower threshold.

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

-- 
Shawn.