Re: [PATCH v2 02/19] parse-options-cb: implement parse_opt_pass_argv_array()

Paul Tan [off-list ref] writes:

I don't see how it feels iffy.

That is largely a taste thing.  And a good taste matters.

What is iffy is to use strbuf as an external interface between the
implementation of the parse_opt_pass() API function and its users.

I would expect that no users of the parse_opt_pass() would ever take
advantage of the fact that the value they are using is implemented
as a strbuf, allowing them to use the family of functions in the
strbuf API to further manipulate it.  All users you added wants to
use a plain vanilla string, and only because you used strbuf as the
interface element, they have to say "var.buf" to get to the value
they want to use.

The purpose of using strbufs (and
argv_arrays) is to avoid error-prone manual memory management.

It is a good idea to use strbuf as an implementation detail of
parse_opt_pass() function.  After all, safer, easier and efficient
manipulation of strings is why we added the strbuf API to the system
in the first place.

But it is a different matter to expose that as an API element when
the recipient is not going to benefit.

In other words, callers of the API designed with a better taste
would look more like this:

	static const char *opt_diffstat;

	static struct option pull_options[] = {
        	...
                { OPTION_CALLBACK, 'n', NULL, &opt_diffstat, NULL,
		  N_("do not show a diffstat at the end of the merge"),
		  PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_pass
        	},
		...
	};

	static int run_merge(void)
        {
        	...
                if (opt_diffstat)
                	argv_array_push(&args, opt_diffstat);
		...
	}

That way, they do not have to define strbuf variables and
dereference the value as opt_diffstat.buf.

And the implementation of the API element is not all that different
from what you wrote:

	int parse_opt_pass(const struct options *opt, const char *arg, int unset)
	{
		struct strbuf sb = STRBUF_INIT;
		char **value = opt->value;

		if (*value)
                	free(*value);
		if (opt->long_name) {
                	strbuf_addf(&sb, "--%s%s",
                        	    unset ? "no-" : "",
                                    opt->long_name);
			...
                }
                ...
                *value = strbuf_detach(&sb, NULL);
		return 0;
	}