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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 8, 2015, 16:49 UTC
Message-ID: <xmqqzj36y4ql.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqsi8z1urz.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 38 quoted lines

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

Whoa, wait.

What is this opt->tweak thing doing here?

    int setup_revisions(int argc, const char **argv, ...
    {
    ...
            /* Second, deal with arguments and options */
    ...
            if (prune_data.nr) {
    ...
                    parse_pathspec(&revs->prune_data, 0, 0,
                                   revs->prefix, prune_data.path);
            }

            if (revs->def == NULL)
                    revs->def = opt ? opt->def : NULL;
            if (opt && opt->tweak)
                    opt->tweak(revs, opt);
    ...
            if (revs->prune_data.nr) {
                    copy_pathspec(&revs->pruning.pathspec, &revs->prune_data);
                    /* Can't prune commits with rename following...
                    if (!DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))
                            revs->prune = 1;
                    if (!revs->full_diff)
                            copy_pathspec(&revs->diffopt.pathspec,
                                          &revs->prune_data);
            }
    ...
    }

It seems that my earlier "setup_revisions() does not have a way to let you flip the settings depending on the number of pathspecs specified" is false. We had to solve a similar issue when we did b4490059 (show -c: show patch text, 2010-03-08), and the mechanism for doing such tweaking of the effect by command line options is already there.

Now I am wondering if the following suffices---instead of adding another special case to setup_revisions(), we can tweak with an existing mechanism.

You may want to rename show_rev_tweak_rev() and call the new default_follow_tweak() function at the end of that existing tweak function, if you want "git show" to also pay attention to the log.follow configuration. I don't think it is necessary.

diff --git a/builtin/log.c b/builtin/log.c
index 6a068f7..d06248a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -625,6 +625,14 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 	return cmd_log_walk(&rev);
 }
 
+static void default_follow_tweak(struct rev_info *rev,
+				 struct setup_revision_opt *opt)
+{
+	if (DIFF_OPT_TST(&rev->diffopt, DEFAULT_FOLLOW_RENAMES) &&
+	    rev->prune_data.nr == 1)
+		DIFF_OPT_SET(&rev->diffopt, FOLLOW_RENAMES);
+}
+
 int cmd_log(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info rev;
@@ -638,6 +646,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";
 	opt.revarg_opt = REVARG_COMMITTISH;
+	opt.tweak = default_follow_tweak;
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
 	return cmd_log_walk(&rev);
 }