Re: [PATCH 7/7] fast-import: use struct option for usage string

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> +	unsigned long pack_size_limit, big_file_threshold, depth, active_branches;
> +	char *edges, *signed_commits, *signed_tags, *date_format, *import_marks;
> +	char *import_marks_if_exists, *export_marks, *submodules_from, *submodules_to;
> +	int opt_quiet, opt_show_stats, opt_relative_marks, opt_force, opt_done;
> +	int opt_allow_unsafe;
> +	int cat_blob;
>  
> -	fast_import_state_init(&state, argc, argv, prefix);
> +	/*
> +	 * NEEDSWORK: For now this is used only to render
> +	 * `-h`/`--help-all` usage messages. The actual parsing is
> +	 * done by parse_one_option()/parse_one_feature().
> +	 */

OK, I am a bit torn on this.  On one hand:

 (1) I do agree that it would be nice to eventually have
     fast_import_state_init() (or some other helper that groks
     argc/argv) use this options array to parse the command line
     arguments.

 (2) I am sympathetic to the position that doing so is a bit
     outside the scope of this series, whose focus is strictly on
     "git fast-import -h" and nothing else.

 (3) I suspect that when fast_import_state_init() does start using
     the options array to initialize the state, the parsed results
     will not be stored in the variables this caller currently holds,
     but will instead live inside the fast_import_state structure.

So in that sense, the huge list of unused function-local variables
above are merely throw-away placeholders.  When the real code is
written, they will disappear, and the references to them in the
fast_import_options[] array will have to be updated to point to
members of the structure (or global variables).

Still, seeing all of those variables left uninitialized leaves a
slightly sour taste.  And because of (3), it would be a clear waste
of time to go through the motions of initializing these throw-away
locals.

Perhaps we would end up in a better position if we bent (2) a bit.
After all, my hesitation likely stems from the feeling that this
series stops short at a slightly awkward spot, having already
completed 90% of the journey.

For example, instead of inventing a local, throw-away
"pack_size_limit" variable, wouldn't it make more sense to refer to
the existing global "max_packsize" variable from the options[]
array below?

> +	struct option fast_import_options[] = {
> +		OPT_GROUP(N_("Common")),
> +		OPT_STRING_F(0, "date-format", &date_format, N_("fmt"),
> +			   N_("format of the commit/tag dates"), PARSE_OPT_NONEG),
> +		OPT_BOOL_F(0, "stats", &opt_show_stats,
> +			   N_("display some basic statistics (objects, packfiles and memory)"),
> +			   PARSE_OPT_NONEG),
> +		OPT_BOOL_F(0, "quiet", &opt_quiet,
> +			   N_("disable the output shown by --stats"), PARSE_OPT_NONEG),
> +		OPT_BOOL_F(0, "force", &opt_force,
> +			   N_("force updating modified existing branches"), PARSE_OPT_NONEG),
> +		OPT_BOOL_F(0, "done", &opt_done,
> +			   N_("require a terminating 'done' command"), PARSE_OPT_NONEG),
> +		OPT_UNSIGNED(0, "max-pack-size", &pack_size_limit,
> +			     N_("maximum size of each output pack file")),
> +		OPT_UNSIGNED(0, "big-file-threshold", &big_file_threshold,

Thanks.