On Tue, 2015-07-07 at 15:13 -0700, Junio C Hamano wrote:
> David Turner <dtur...@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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
