From: Junio C Hamano <gitster@pobox•com>
To: David Turner <dturner@twopensource•com>
Cc: git@vger•kernel.org, David Turner <dturner@twitter•com>
Subject: Re: [PATCH v2] log: add log.follow config option
Date: Wed, 08 Jul 2015 09:49:06 -0700 [thread overview]
Message-ID: <xmqqzj36y4ql.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <xmqqsi8z1urz.fsf@gitster.dls.corp.google.com> (Junio C. Hamano's message of "Tue, 07 Jul 2015 15:13:04 -0700")
Junio C Hamano <gitster@pobox•com> writes:
> 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);
}
prev parent reply other threads:[~2015-07-08 16:49 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-07 18:40 [PATCH v2] log: add log.follow config option David Turner
2015-07-07 21:42 ` Matthieu Moy
2015-07-07 22:11 ` David Turner
2015-07-07 22:13 ` Junio C Hamano
2015-07-08 1:28 ` David Turner
2015-07-08 4:12 ` Junio C Hamano
2015-07-08 16:49 ` Junio C Hamano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqzj36y4ql.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=dturner@twitter$(echo .)com \
--cc=dturner@twopensource$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox