2017-05-12 17:48 GMT-06:00 Jonathan Nieder <jrnie...@gmail.com>: > Hi, > > brian m. carlson wrote: > >> Does anyone else have views on whether this is good thing to test for? > > I know you don't mean to be rude, but this comes across as a bit of > a dismissive question.
The question sounded neutral to me. >> On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote: >>> brian m. carlson wrote: > >>>> The recent change that introduced autodecorating of refs accidentally >>>> broke the ability of users to set log.decorate = false to override it. >>> >>> Yikes. It sounds to me like we need a test to ensure we don't regress >>> it again later. >> >> I can add one, but it's going to be a bit odd. The issue is that as far >> as I can tell, the option is honored only if it's the last one read, so >> it necessarily has to be in the per-repository configuration. >> >> I'm not sure it makes that much sense to add a test for this case. Do >> we generally want to write such tests for all config options? I don't >> suppose it's that common a mistake to make. > > In my humble opinion, the bug being subtle makes it especially useful > to have a test that describes that bug. That way, if someone is > refactoring this code later then they know what to watch out for not > reintroducing. > > I'm happy to hear other opinions, especially if they come with data or > a principle attached that I can use when writing future patches of my > own. When I saw Brian's email today, my first thought was "What was I thinking?" My mistake was pretty obvious. Then I remembered that when I wrote the original patch, I wasn't sure where to set the default value, because there were no clear examples in this file. Now that we've established a clear precedent for setting the log.decorate default (and other defaults like it) in init_log_defaults, I don't expect any more problems with log.decorate. And since it's not practical to add tests for similar bugs for every command and configuration option in Git, we'll just have to be a little more vigilant about code review. Again, I apologize for the trouble. -Alex
