Re: [PATCH v2] log: add log.follow config option

> Hi,
> 
> Thanks for your patch. Below are some comments. Some of them are just me
> thinking out loudly (don't take it badly if I'm being negative), some
> are more serious, but all are fixable.

> David Turner <dturner@twopensource.com> writes:
> 
> > From: David Turner <dturner@twitter.com>
> 
> If you configure your commiter id and your From field to the same value,
> you can avoid this distracting "From:" header.
> 
> You're lacking a Signed-off-by trailer.

> > +     git log --name-status --pretty="format:%s"  path1 > current'
> 
> Whitespace: here an elsewhere, you have two spaces before path1, and we
> usually stick the > to the filename like >current.
>
> > --- a/t/t4206-log-follow-harder-copies.sh
> > +++ b/t/t4206-log-follow-harder-copies.sh
> > @@ -53,4 +53,39 @@ test_expect_success \
> >      'validate the output.' \
> >      'compare_diff_patch current expected'
> >  
> > +test_expect_success \
> > +    'git config log.follow works like --follow' \
> > +    'test_config log.follow true &&
> > +     git log --name-status --pretty="format:%s"  path1 > current'
> > +
> > +test_expect_success \
> > +    'validate the output.' \
> > +    'compare_diff_patch current expected'
> 
> I would squash these two tests. As-is, the first one doesn't really test
> anything (well, just that git log doesn't crash with log.follow).
> 
> I think you meant test_cmp, not compare_diff_patch, as you don't need
> the "similarity index" and "index ..." filtering that compare_diff_patch
> does before test_cmp.
> 
> That said, I see that you are mimicking surrounding code, which is a
> good thing for consistancy. I think the best would be to write tests in
> t4202-log.sh, which already tests the --follow option, and uses a more
> modern coding style which you can mimick.
> t4206-log-follow-harder-copies.sh is really about finding copies in
> --follow. Another option is to start your series with a patch like
> "t4206: modernize style".