Re: [PATCH] worktree: add per-worktree config files

From: Duy Nguyen <pclouds@gmail.com>
Date: Sep 29, 2018, 13:53 UTC
Message-ID: <CACsJy8AqX+ckqxLpf5HtpfSJi=gkfodTNQP_mQb=LC-Dgvih=Q@mail.gmail.com>
In-Reply-To: <20180924142143.GD68796@syl>

On Mon, Sep 24, 2018 at 4:21 PM Taylor Blau <me@ttaylorr.com> wrote:

Show 19 quoted lines

> > @@ -602,6 +605,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> >                            PARSE_OPT_STOP_AT_NON_OPTION);
> >
> >       if (use_global_config + use_system_config + use_local_config +
> > +         use_worktree_config +
> >           !!given_config_source.file + !!given_config_source.blob > 1) {
>
> I feel like we're getting into "let's extract a function" territory,
> here, since this line is growing in width. Perhaps:
>
>   static int config_files_count()
>   {
>     return use_global_config + use_system_config + use_local_config +
>                 use_worktree_config +
>                 !!given_config_source.file +
>                 !!given_config_source.blob;
>   }
>
> Simplifying the call to:

I think there's a bigger problem here because we have no good way to describe that a bunch of options are mutually exclusive. We get by ok if the number of options is two, but when it gets bigger (and this is not the only place), it gets messier. Besides the long "if" condition, I consider the error message "only one config file at a time" inadequate. We should tell the user what exact options are conflicting.

So, I'm going to bury my head in the sand this time and ignore the problem, including adding config_files_count(). It could be a interesting mini project if someone wants to jump in. Meanwhile it'll go to the very bottom of my long long backlog.

-- 
Duy