Hi,
On Sat, 14 Feb 2009, Felipe Contreras wrote:
> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
> <Johannes.Schindelin@xxxxxx> wrote:
>
> > 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,
What I meant was: have a patch renaming get_color_slot to
get_colorbool_slot _before_ the (already large) parseoptification.
> >> + 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?
I think we should die() in all cases, not just one, and we might want to
skip the "file" parameter (and the "errno") parameter, as the file
containing the error could be different from "file".
> >> + 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");
Yes.
> >> + 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?
Yes.
> >> + 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?
AFAICT an exit status is supposed to be between 0 and 127.
Ciao,
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
