On Wed, Oct 31, 2018 at 01:35:38PM +0100, Miklos Szeredi wrote: > On Wed, Oct 31, 2018 at 1:26 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Wed, Oct 31, 2018 at 2:19 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > > My 'strict' patch did not try to deal with implicitly enabling redirect_dir > > Vivek posted a different patch to deal with that. > > What I mean, is that sections of your patch dealing with the > redirect_dir conflict and sections of Vivek's patch dealing with > _enforce are completely unnecessary if we just define the rules of > implicit enable/disable between metacopy and redirect_dir. > > >> config->redirect_mode = match_strdup(&args[0]); > >> if (!config->redirect_mode) > >> return -ENOMEM; > >> + > >> + /* "redirect_dir=off" implies "metacopy=off" */ > >> + if (strcmp(config->redirect_mode, "off") == 0 || > >> + strcmp(config->redirect_mode, "nofollow") == 0) > >> + config->metacopy = false; > > > > This look completely counter to the 'strict' behavior definition. > > If user specified metacopy=on, we are not allowed to end up with metacopy=off. > > What about "metacopy=on,metacopy=off"? I would think that ordering will matter. metacopy=off will override, metacopy=on (because its later in the order). > > What I'm saying is that "metacopy=on,redirect_dir=off" should be > exactly equivalent to the above, if taking the implicit disable rule > into account. I am wondering why do we have to accept nonsense options possibilities. Why not simply reject them with -EINVAL. IMHO, that behavior is much simpler to deal with than implicitly disabling metacopy because redirect_dir=off. If some options don't make sense together, then it makes sense to return error and let user understand it and fix it. Thanks Vivek
