Hi,
On Sat, 14 Feb 2009, Felipe Contreras wrote:
> @@ -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?
> + 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.
> + 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.
> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
