On Tue, 2015-07-07 at 23:42 +0200, Matthieu Moy wrote: > 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. Thanks for the feedback! > David Turner <dturner@xxxxxxxxxxxxxxxx> writes: > > > From: David Turner <dturner@xxxxxxxxxxx> > > 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. Oops. Cherry-picked over from internal repo. Will fix. <snip> (suggestions applied) > > + 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". I'm going to move over to t4202. My next re-roll will include fixes for everything you raised. -- 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