Pavan Kumar Sunkara <pavan.sss1991@xxxxxxxxx> writes: > The right one is given below. > http://mid.gmane.org/1275573356-21466-3-git-send-email-pavan.sss1991@xxxxxxxxx Oh, I do have three issues with that one. #1. Command line should be able to override configuration You currently have: . By default without command line override nor configuration, old behaviour of "refresh the gitweb.conf every time" stands; . You can tell it to keep the configuration with a command line option; . If you are tired of having to give the command line option every time, you can set a configuration variable instead. The first bullet is fine; it is called "backward compatibility." The second one is also good. "We allow a non-default behaviour in a rare case with a command line option" is a good change. But the third one makes the story quite different. Adding a configuration needs to be done a lot more carefully. If somebody has configured instaweb.overwrite to false, you must give him a way to override that from the command line. IOW, you must at least support "git instaweb --no-reuse-config" in [PATCH 2/3]. If you anticipate that some people may get tired of having to give that option all the time, it is possible that the choice of the original default behaviour was wrong, at least for some people. We might even want to make "instaweb.overwrite" default to "false" in later versions. Having command line override of configured default becomes even more important for that transition to happen smoothly. If a user has to use older and newer versions of git across that default flip, it would give him a reliable behaviour to say --[no-]reuse-config explicitly from the command line. #2. The subject of the patch should spell the name of the new variable on it. But this is an artifact of a larger design issue; see below. #3. Naming. If you are going to make an configuration variable, its name should be consistent with the command line option (I think Pasky said something similar). Your command line is '--reuse-config' but your configuration is '(instaweb.)overwrite'. Do you think these click with each other for people other than you? Wouldn't it be much more consistent if the above were: . By default without command line override nor configuration, old behaviour of "refresh the gitweb.conf every time" stands; . You can tell it to keep the configuration by --no-overwrite-config option; to regenerate the config, say --overwrite-config (which is the current default but we _might_ change the default in the future if "no overwrite" is found to be more sensible by larger audience). . If you are tired of having to give --no-overwrite-config every time, you can say "[instaweb] overwriteConfig = false". I know "overwrite-config" is a bit too long, and you would need to come up with a shorter name (perhaps "reconfig"???) but the point is that we should make it easy to guess the corresponding configuration variable name given the command line option and vice-versa. > It's not acked but it's not commented. So I am guessing nobody has > problem with it. That is not the right way to interpret lack of responses. "Nobody is interested in that patch" is the default interpretation. -- 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