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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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.

> +log.follow::
> +	If a single file argument is given to git log, it will act as
> +	if the `--follow` option was also used.  This adds no new
> +	functionality over what --follow already provides (in other words,
> +	it cannot be used to follow multiple files).  It's just a
> +	convenience.

Missing `...` around the second --follow.

I would write this as:

	This has the same limitations as --follow, i.e. it cannot be
	used to follow multiple files and does not work well on
	non-linear history.

and drop the "it's just a convenience" part.

> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -31,6 +31,7 @@ static const char *default_date_mode = NULL;
>  
>  static int default_abbrev_commit;
>  static int default_show_root = 1;
> +static int default_follow = 0;

I tend to disagree with this rule, but in Git we usually omit the "= 0"
for static variables, which are already initialized to 0 by default.

> +		/* Can't prune commits with rename following: the paths change.. */
> +		if (!DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES)) {
> +			revs->prune = 1;
> +		} else {
> +			revs->diff = 1;
> +		}

Useless braces.

> +     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".

Or you can just accept that the current style in this file is subpar and
continue with it.

> +test_expect_success \
> +    'git config log.follow does not die with multiple paths' \
> +    'test_config log.follow true &&
> +     git log path0 path1 > current &&

You are creating 'current' but not using it.

> +     wc -l current'

What is the intent of this "wc -l current"? You're not checking its
output ...

> +test_expect_success \
> +    'git config log.follow does not die with no paths' \
> +    'test_config log.follow true &&
> +     git log -- > current &&

One more creation of current which isn't used ...

> +     wc -l current'
> +
> +test_expect_success \
> +    'git config log.follow is overridden by --no-follow' \
> +    'test_config log.follow true &&
> +     git log --no-follow --name-status --pretty="format:%s"  path1 > current'

... because you're overwriting it here.

> +cat >expected <<\EOF
> +Copy path1 from path0
> +A	path1
> +EOF

Put everything in test_expect_..., including creation of expected file.
For more info, read t/README and its Do's, don'ts & things to keep in
mind section.

> +test_expect_success \
> +    'validate the output.' \
> +    'compare_diff_patch current expected'
> +
>  test_done

Cheers,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/