On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
<Johannes.Schindelin@xxxxxx> wrote:
> 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?
In that case I couldn't use OPT_STRING to store that value; I would
have to change --get-color* to use OPT_BOOLEAN or OPT_SET_INT and
store check the argc (since OPT_STRING isn't doing that anymore) and
save argv[1] to get_color_slot,
>> + 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.
Yeap.
>> + 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.
True.
I tried to minimize changes, so that code is exactly the same as
before (except vi didn't preserve the indent).
>> + if (actions & ACTION_LIST) {
>> + if (git_config(show_all_config, NULL) < 0 &&
>> + file && errno)
>
> Should this not be config_exclusive_filename?
Looks like that.
>> + 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.
Yes, there should be an error even if 'file' is not set. But if the
file is set what's wrong with that die command?
>> + 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?
You mean:
if (!config_exclusive_filename)
config_exclusive_filename = git_path("config");
>> + else if (actions & ACTION_ADD) {
>> + check_argc(argc, 2, 2);
>
> BTW what about check_argc() in the previous two cases?
You mean fail if -e or -l have extra arguments?
>> + 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;"?
Huh? So git commands don't return negative error values?
--
Felipe Contreras
--
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
