Re: [PATCH v2] config: Use parseopt.

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 14, 2009, 19:59 UTC
Message-ID: <alpine.DEB.1.00.0902142041370.10279@pacific.mpi-cbg.de>
In-Reply-To: <1234612989-32297-1-git-send-email-felipe.contreras@gmail.com>

Hi,

On Sat, 14 Feb 2009, Felipe Contreras wrote:

Show 9 quoted lines

> @@ -231,7 +264,7 @@ static int get_diff_color_found;
>  static int git_get_colorbool_config(const char *var, const char *value,
>  		void *cb)
>  {
> -	if (!strcmp(var, get_color_slot)) {
> +	if (!strcmp(var, get_colorbool_slot)) {
>  		get_colorbool_found =
>  			git_config_colorbool(var, value, stdout_is_tty);
>  	}

Name changes like this make it harder to read the patch; can you separate that change out into its own patch?

Show 5 quoted lines

> +	if (use_global_config) {
> +		char *home = getenv("HOME");
> +		if (home) {
> +			char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
> +			config_exclusive_filename = user_config;

In a subsequent patch, you might add a check only one of --global, --system or --file was given.

Show 7 quoted lines

> +	else if (given_config_file) {
> +		if (!is_absolute_path(given_config_file) && file)
> +			file = prefix_filename(file, strlen(file),
> +					       given_config_file);
> +		else
> +			file = given_config_file;
> +		config_exclusive_filename = file;

It took me a considerable amount of time to figure out that "file" is actually the "prefix"! That cleanup would be nice to have before the parseopt patch, methinks, especially since the code is reindented, and thus hard to follow in the diff.

> +	if (actions & ACTION_LIST) {
> +		if (git_config(show_all_config, NULL) < 0 &&
> +		    file && errno)

Should this not be config_exclusive_filename?

> +			die("unable to read config file %s: %s", file,
> +			    strerror(errno));

Do we really only want to die() in case we know the file name? AFAICT at this point we have no idea in which of the possibly three files the error occurred. And there need not be any errno set, for example when there was a parse error.

Show 6 quoted lines

> +	else if (actions & ACTION_EDIT) {
> +		const char *config_filename;
> +		if (config_exclusive_filename)
> +			config_filename = config_exclusive_filename;
> +		else
> +			config_filename = git_path("config");

Why not reuse config_exclusive_filename here?

> +	else if (actions & ACTION_ADD) {
> +		check_argc(argc, 2, 2);

BTW what about check_argc() in the previous two cases?

> +		return git_config_set_multivar(argv[0], value, "^$", 0);

Now that I see this, there is another idea for a possible cleanup after the parseoptification: cmd_config() should not return -1, as that will be turned into the exit status. So maybe prefix the return value with "!!"?

Or maybe even better: set a variable "ret" and at the end of cmd_config(), "return !!ret;"?

The rest looks good to me.
 
Thanks,
Dscho