* [PATCH v2] log: add log.follow config option
@ 2015-07-07 18:40 David Turner
2015-07-07 21:42 ` Matthieu Moy
2015-07-07 22:13 ` Junio C Hamano
0 siblings, 2 replies; 7+ messages in thread
From: David Turner @ 2015-07-07 18:40 UTC (permalink / raw
To: git; +Cc: David Turner
From: David Turner <dturner@twitter.com>
Many users prefer to always use --follow with logs. Rather than
aliasing the command, an option might be more convenient for some.
---
Documentation/git-log.txt | 7 +++++++
builtin/log.c | 7 +++++++
diff.c | 5 +++--
diff.h | 1 +
revision.c | 15 ++++++++++++---
t/t4206-log-follow-harder-copies.sh | 35 +++++++++++++++++++++++++++++++++++
6 files changed, 65 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 5692945..5a23085 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -184,6 +184,13 @@ log.date::
`--date` option.) Defaults to "default", which means to write
dates like `Sat May 8 19:35:34 2010 -0500`.
+log.follow::
+ If a single file argument is given to git log, it will act as
+ if the `--follow` option was also used. This adds no new
+ functionality over what --follow already provides (in other words,
+ it cannot be used to follow multiple files). It's just a
+ convenience.
+
log.showRoot::
If `false`, `git log` and related commands will not treat the
initial commit as a big creation event. Any root commits in
diff --git a/builtin/log.c b/builtin/log.c
index 8781049..9b6abef 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -31,6 +31,7 @@ static const char *default_date_mode = NULL;
static int default_abbrev_commit;
static int default_show_root = 1;
+static int default_follow = 0;
static int decoration_style;
static int decoration_given;
static int use_mailmap_config;
@@ -102,6 +103,8 @@ static void cmd_log_init_defaults(struct rev_info *rev)
{
if (fmt_pretty)
get_commit_format(fmt_pretty, rev);
+ if (default_follow)
+ DIFF_OPT_SET(&rev->diffopt, DEFAULT_FOLLOW_RENAMES);
rev->verbose_header = 1;
DIFF_OPT_SET(&rev->diffopt, RECURSIVE);
rev->diffopt.stat_width = -1; /* use full terminal width */
@@ -390,6 +393,10 @@ static int git_log_config(const char *var, const char *value, void *cb)
default_show_root = git_config_bool(var, value);
return 0;
}
+ if (!strcmp(var, "log.follow")) {
+ default_follow = git_config_bool(var, value);
+ return 0;
+ }
if (skip_prefix(var, "color.decorate.", &slot_name))
return parse_decorate_color_config(var, slot_name, value);
if (!strcmp(var, "log.mailmap")) {
diff --git a/diff.c b/diff.c
index 87b16d5..135b222 100644
--- a/diff.c
+++ b/diff.c
@@ -3815,9 +3815,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
DIFF_OPT_SET(options, FIND_COPIES_HARDER);
else if (!strcmp(arg, "--follow"))
DIFF_OPT_SET(options, FOLLOW_RENAMES);
- else if (!strcmp(arg, "--no-follow"))
+ else if (!strcmp(arg, "--no-follow")) {
DIFF_OPT_CLR(options, FOLLOW_RENAMES);
- else if (!strcmp(arg, "--color"))
+ DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
+ } else if (!strcmp(arg, "--color"))
options->use_color = 1;
else if (skip_prefix(arg, "--color=", &arg)) {
int value = git_config_colorbool(NULL, arg);
diff --git a/diff.h b/diff.h
index c7ad42a..f7208ad 100644
--- a/diff.h
+++ b/diff.h
@@ -91,6 +91,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
#define DIFF_OPT_DIRSTAT_BY_LINE (1 << 28)
#define DIFF_OPT_FUNCCONTEXT (1 << 29)
#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
+#define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1 << 31)
#define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)
#define DIFF_OPT_TOUCHED(opts, flag) ((opts)->touched_flags & DIFF_OPT_##flag)
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;
diff --git a/t/t4206-log-follow-harder-copies.sh b/t/t4206-log-follow-harder-copies.sh
index ad29e65..5355398 100755
--- a/t/t4206-log-follow-harder-copies.sh
+++ b/t/t4206-log-follow-harder-copies.sh
@@ -53,4 +53,39 @@ test_expect_success \
'validate the output.' \
'compare_diff_patch current expected'
+test_expect_success \
+ 'git config log.follow works like --follow' \
+ 'test_config log.follow true &&
+ git log --name-status --pretty="format:%s" path1 > current'
+
+test_expect_success \
+ 'validate the output.' \
+ 'compare_diff_patch current expected'
+
+test_expect_success \
+ 'git config log.follow does not die with multiple paths' \
+ 'test_config log.follow true &&
+ git log path0 path1 > current &&
+ wc -l current'
+
+test_expect_success \
+ 'git config log.follow does not die with no paths' \
+ 'test_config log.follow true &&
+ git log -- > current &&
+ wc -l current'
+
+test_expect_success \
+ 'git config log.follow is overridden by --no-follow' \
+ 'test_config log.follow true &&
+ git log --no-follow --name-status --pretty="format:%s" path1 > current'
+
+cat >expected <<\EOF
+Copy path1 from path0
+A path1
+EOF
+
+test_expect_success \
+ 'validate the output.' \
+ 'compare_diff_patch current expected'
+
test_done
--
2.0.5.499.g01f6352.dirty-twtrsrc
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] log: add log.follow config option
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
1 sibling, 1 reply; 7+ messages in thread
From: Matthieu Moy @ 2015-07-07 21:42 UTC (permalink / raw
To: David Turner; +Cc: git, David Turner
Hi,
Thanks for your patch. Below are some comments. Some of them are just me
thinking out loudly (don't take it badly if I'm being negative), some
are more serious, but all are fixable.
David Turner <dturner@twopensource.com> writes:
> From: David Turner <dturner@twitter.com>
If you configure your commiter id and your From field to the same value,
you can avoid this distracting "From:" header.
You're lacking a Signed-off-by trailer.
> +log.follow::
> + If a single file argument is given to git log, it will act as
> + if the `--follow` option was also used. This adds no new
> + functionality over what --follow already provides (in other words,
> + it cannot be used to follow multiple files). It's just a
> + convenience.
Missing `...` around the second --follow.
I would write this as:
This has the same limitations as --follow, i.e. it cannot be
used to follow multiple files and does not work well on
non-linear history.
and drop the "it's just a convenience" part.
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -31,6 +31,7 @@ static const char *default_date_mode = NULL;
>
> static int default_abbrev_commit;
> static int default_show_root = 1;
> +static int default_follow = 0;
I tend to disagree with this rule, but in Git we usually omit the "= 0"
for static variables, which are already initialized to 0 by default.
> + /* 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;
> + }
Useless braces.
> + git log --name-status --pretty="format:%s" path1 > current'
Whitespace: here an elsewhere, you have two spaces before path1, and we
usually stick the > to the filename like >current.
> --- a/t/t4206-log-follow-harder-copies.sh
> +++ b/t/t4206-log-follow-harder-copies.sh
> @@ -53,4 +53,39 @@ test_expect_success \
> 'validate the output.' \
> 'compare_diff_patch current expected'
>
> +test_expect_success \
> + 'git config log.follow works like --follow' \
> + 'test_config log.follow true &&
> + git log --name-status --pretty="format:%s" path1 > current'
> +
> +test_expect_success \
> + 'validate the output.' \
> + 'compare_diff_patch current expected'
I would squash these two tests. As-is, the first one doesn't really test
anything (well, just that git log doesn't crash with log.follow).
I think you meant test_cmp, not compare_diff_patch, as you don't need
the "similarity index" and "index ..." filtering that compare_diff_patch
does before test_cmp.
That said, I see that you are mimicking surrounding code, which is a
good thing for consistancy. I think the best would be to write tests in
t4202-log.sh, which already tests the --follow option, and uses a more
modern coding style which you can mimick.
t4206-log-follow-harder-copies.sh is really about finding copies in
--follow. Another option is to start your series with a patch like
"t4206: modernize style".
Or you can just accept that the current style in this file is subpar and
continue with it.
> +test_expect_success \
> + 'git config log.follow does not die with multiple paths' \
> + 'test_config log.follow true &&
> + git log path0 path1 > current &&
You are creating 'current' but not using it.
> + wc -l current'
What is the intent of this "wc -l current"? You're not checking its
output ...
> +test_expect_success \
> + 'git config log.follow does not die with no paths' \
> + 'test_config log.follow true &&
> + git log -- > current &&
One more creation of current which isn't used ...
> + wc -l current'
> +
> +test_expect_success \
> + 'git config log.follow is overridden by --no-follow' \
> + 'test_config log.follow true &&
> + git log --no-follow --name-status --pretty="format:%s" path1 > current'
... because you're overwriting it here.
> +cat >expected <<\EOF
> +Copy path1 from path0
> +A path1
> +EOF
Put everything in test_expect_..., including creation of expected file.
For more info, read t/README and its Do's, don'ts & things to keep in
mind section.
> +test_expect_success \
> + 'validate the output.' \
> + 'compare_diff_patch current expected'
> +
> test_done
Cheers,
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] log: add log.follow config option
2015-07-07 21:42 ` Matthieu Moy
@ 2015-07-07 22:11 ` David Turner
0 siblings, 0 replies; 7+ messages in thread
From: David Turner @ 2015-07-07 22:11 UTC (permalink / raw
To: Matthieu Moy; +Cc: git, David Turner
On Tue, 2015-07-07 at 23:42 +0200, Matthieu Moy wrote:
> Hi,
>
> Thanks for your patch. Below are some comments. Some of them are just me
> thinking out loudly (don't take it badly if I'm being negative), some
> are more serious, but all are fixable.
Thanks for the feedback!
> David Turner <dturner@twopensource.com> writes:
>
> > From: David Turner <dturner@twitter.com>
>
> If you configure your commiter id and your From field to the same value,
> you can avoid this distracting "From:" header.
>
> You're lacking a Signed-off-by trailer.
Oops. Cherry-picked over from internal repo. Will fix.
<snip> (suggestions applied)
> > + git log --name-status --pretty="format:%s" path1 > current'
>
> Whitespace: here an elsewhere, you have two spaces before path1, and we
> usually stick the > to the filename like >current.
>
> > --- a/t/t4206-log-follow-harder-copies.sh
> > +++ b/t/t4206-log-follow-harder-copies.sh
> > @@ -53,4 +53,39 @@ test_expect_success \
> > 'validate the output.' \
> > 'compare_diff_patch current expected'
> >
> > +test_expect_success \
> > + 'git config log.follow works like --follow' \
> > + 'test_config log.follow true &&
> > + git log --name-status --pretty="format:%s" path1 > current'
> > +
> > +test_expect_success \
> > + 'validate the output.' \
> > + 'compare_diff_patch current expected'
>
> I would squash these two tests. As-is, the first one doesn't really test
> anything (well, just that git log doesn't crash with log.follow).
>
> I think you meant test_cmp, not compare_diff_patch, as you don't need
> the "similarity index" and "index ..." filtering that compare_diff_patch
> does before test_cmp.
>
> That said, I see that you are mimicking surrounding code, which is a
> good thing for consistancy. I think the best would be to write tests in
> t4202-log.sh, which already tests the --follow option, and uses a more
> modern coding style which you can mimick.
> t4206-log-follow-harder-copies.sh is really about finding copies in
> --follow. Another option is to start your series with a patch like
> "t4206: modernize style".
I'm going to move over to t4202. My next re-roll will include fixes for
everything you raised.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] log: add log.follow config option
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:13 ` Junio C Hamano
2015-07-08 1:28 ` David Turner
2015-07-08 16:49 ` Junio C Hamano
1 sibling, 2 replies; 7+ messages in thread
From: Junio C Hamano @ 2015-07-07 22:13 UTC (permalink / raw
To: David Turner; +Cc: git, David Turner
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] log: add log.follow config option
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
1 sibling, 1 reply; 7+ messages in thread
From: David Turner @ 2015-07-08 1:28 UTC (permalink / raw
To: Junio C Hamano; +Cc: git, David Turner
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] log: add log.follow config option
2015-07-08 1:28 ` David Turner
@ 2015-07-08 4:12 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2015-07-08 4:12 UTC (permalink / raw
To: David Turner; +Cc: git, David Turner
David Turner <dturner@twopensource.com> writes:
>> 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.
Ah, I see. You don't have to move the existing code for that then.
Just insert the "if prune has one element and DEFAULT_ is set" thing
before the first use of FOLLOW_RENAMES (i.e. "pick, filter and follow
needs diff" piece) and you are done, I think.
Thanks.
>
>> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] log: add log.follow config option
2015-07-07 22:13 ` Junio C Hamano
2015-07-08 1:28 ` David Turner
@ 2015-07-08 16:49 ` Junio C Hamano
1 sibling, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2015-07-08 16:49 UTC (permalink / raw
To: David Turner; +Cc: git, David Turner
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);
}
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-07-08 16:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.