Re: [PATCH v2] config: Use parseopt.
- From
Felipe Contreras <felipe.contreras@gmail.com>
- Date
- Feb 14, 2009, 20:31 UTC
- Message-ID
- <94a0d4530902141231t143067e5n872558a4e515be4a@mail.gmail.com>
- In-Reply-To
- <alpine.DEB.1.00.0902142041370.10279@pacific.mpi-cbg.de>
On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 16 quoted lines
> 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,
Show 8 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.Yeap.
Show 12 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.True.
I tried to minimize changes, so that code is exactly the same as before (except vi didn't preserve the indent).
Show 5 quoted lines
>> + if (actions & ACTION_LIST) {
>> + if (git_config(show_all_config, NULL) < 0 &&
>> + file && errno)
>
> Should this not be config_exclusive_filename?Looks like that.
Show 7 quoted lines
>> + 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?
Show 8 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?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?
Show 8 quoted lines
>> + 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