Junio C Hamano <gitster@xxxxxxxxx> writes:
> This certainly is good, but I wonder if a new variant of OPT_STRING
> and OPTION_STRING that does the strdup for you, something along the
> lines of ...
> ... may make it even more pleasant to use? Only for two fields in
> this patch that may probably be an overkill, but we may eventually
> benefit from such an approach when we audit and plug leaks in
> parse-options users. I dunno.
After sleeping on it, I do think this is an overkill. The pattern I
would expect for the most normal (boring) code to use is rather:
struct app_specific app;
const char *opt_x = NULL;
struct option options[] = {
...
OPT_STRING(0, "xopt", &opt_x, N_("x option"), ...),
...
OPT_END()
};
parse_options(ac, av, prefix, options, ...);
app.x_field = xstrdup_or_null(opt_x);
... other values set to app's field based on
... not just command line options but from
... other sources.
The only reason why the OPT_STRDUP appeared convenient was because
options[] element happened to use a field in the structure directly.
The patch under discussion does an equivalent of
app.x_field = xstrdup_or_null(opt_x);
but the "opt_x" happens to be the same "app.x_field" in this case,
so in that sense, it follows the normal and boring pattern.
The "struct app_specific" may not even exist in the same scope as
the caller of parse_options(), but may have to be initialized in a
function that is three-level deep in the callchain, with opt_x
variable passed through as a parameter. So OPT_STRDUP may not be a
bad or horrible idea, but it is not such a great one, either.