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

[Junio C Hamano <gitster@pobox.com>]

David Turner <dturner@twopensource.com> writes:

> diff --git a/revision.c b/revision.c
> index 3ff8723..ae6d4c3 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2322,12 +2322,21 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  
>  	if (revs->prune_data.nr) {
>  		copy_pathspec(&revs->pruning.pathspec, &revs->prune_data);
> -		/* Can't prune commits with rename following: the paths change.. */
> -		if (!DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))
> -			revs->prune = 1;
> +
>  		if (!revs->full_diff)
>  			copy_pathspec(&revs->diffopt.pathspec,
>  				      &revs->prune_data);
> +
> +		if (DIFF_OPT_TST(&revs->diffopt, DEFAULT_FOLLOW_RENAMES) &&
> +		    revs->diffopt.pathspec.nr == 1)
> +			DIFF_OPT_SET(&revs->diffopt, FOLLOW_RENAMES);
> +
> +		/* 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;
> +		}
>  	}
>  	if (revs->combine_merges)
>  		revs->ignore_merges = 0;

It is unfortunate that we have to waste one DIFF_OPT bit and touch
revision.c for something that is "just a convenience".  Because
setup_revisions() does not have a way to let you flip the settings
depending on the number of pathspecs specified, I do not think of a
solution that does not have to touch a low level foundation part of
the codebase, which I'd really want to avoid.

But I wonder why your patch needs to touch so much.

Your changes to other files make sure that diffopt has the
DEFAULT_FOLLOW_RENAMES when (1) the configuration is set and (2) the
command line did not override it with --no-follow.  And these look
very sensible.

Isn't the only thing left to do in this codepath, after the DEFAULT_
is set up correctly, to set FOLLOW_RENAMES when (1) DEFAULT_ is set
and (2) you have only one path?

And once FOLLOW_RENAMES is set or unset according to the end-user
visible semantics, i.e. "just for a convenience, setting log.follow
adds --follow to the command line if and only if there is only one
pathspec", I do not see why existing code needs to be modified in
any other way.

IOW, I'd like to know why we need more than something like this
change to this file, instead of the above?  We didn't muck with
revs->diff in the original when FOLLOW_RENAMES was set, but now it
does, for example.

diff --git a/revision.c b/revision.c
index 3ff8723..f7bd229 100644
--- a/revision.c
+++ b/revision.c
@@ -2270,6 +2270,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			got_rev_arg = 1;
 	}
 
+	if (DIFF_OPT_TST(&revs->diffopt, DEFAULT_FOLLOW_RENAMES) &&
+	    revs->diffopt.pathspec.nr == 1)
+		DIFF_OPT_SET(&revs->diffopt, FOLLOW_RENAMES);
+
 	if (prune_data.nr) {
 		/*
 		 * If we need to introduce the magic "a lone ':' means no