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

[David Turner <dturner@twopensource.com>]

On Tue, 2015-07-07 at 15:13 -0700, Junio C Hamano wrote:
> 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.

We did, but we did it earlier.  But I can just rearrange the code.

> 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

revs->diffopt.pathspec isn't set up yet then. But prune_data is, so I
can use that. 

Will send a v3.