[PATCH] Fix "git log --diff-filter" bug

[PATCH] Fix "git log --diff-filter" bug

[Arjen Laarhoven]

In commit b7bb760d5ed4881422673d32f869d140221d3564 an optimization
was made to avoid unnecessary diff generation.  This was partly fixed
in 99516e35d096f41e7133cacde8fbed8ee9a3ecd0, but obviously the
'--diff-filter' option also needs the diff machinery in action.

Signed-off-by: Arjen Laarhoven <arjen@yaph.org>
---
 revision.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 7e2f4f1..6e85aaa 100644
--- a/revision.c
+++ b/revision.c
@@ -1290,8 +1290,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 	if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
 		revs->diff = 1;
 
-	/* Pickaxe and rename following needs diffs */
-	if (revs->diffopt.pickaxe || DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))
+	/* Pickaxe, diff-filter and rename following need diffs */
+	if (revs->diffopt.pickaxe ||
+	    revs->diffopt.filter ||
+	    DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))
 		revs->diff = 1;
 
 	if (revs->topo_order)
-- 
1.5.4.rc1.21.g0e545

Re: [PATCH] Fix "git log --diff-filter" bug

[Jakub Narebski]

Arjen Laarhoven <arjen@yaph.org> writes:

> In commit b7bb760d5ed4881422673d32f869d140221d3564 an optimization
> was made to avoid unnecessary diff generation.  This was partly fixed
> in 99516e35d096f41e7133cacde8fbed8ee9a3ecd0, but obviously the
> '--diff-filter' option also needs the diff machinery in action.

Thanks a lot! I was wondering why 'git log --diff-filter=M' didn't
find anything...
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] Fix "git log --diff-filter" bug

[Junio C Hamano]

Thanks.  Some tests?

[PATCH] Test "git log --diff-filter"

[Jakub Narebski]

Add test to check "git log --diff-filter" works correctly with and
without diff generation by git-log; the main purpose of this test is
to check if "git log --diff-filter" filters revisions correctly.

This is a companion test to commit 0faf2da7e5ee5c2f472d8a7afaf8616101f34e80
(Fix "git log --diff-filter" bug) by Arjen Laarhoven.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Junio C Hamano wrote:

> Thanks.  Some tests?

So there it is...


First, either I don't understand what referred to commit was supposed
to fix, or my test is wrong, or the patch doesn't fix the bug.

Second, I have a few questions about the test itself. I'm not that
sure about it's name: t/README tells us to use 4 as a first digit of
test number for testing diff commands, and 8 for commands concerning
forensics. "git log --diff-filter" is a forensics concerning diff
output. Second digit is for command itself: 0 is used for diff, 2 for
log, so perhaps the test should be named t/t4203-log-diff-filter.sh

The style of writing test is not very consistent across git test
suite. I think that the 'setup' step style is all right, and only
perhas the style of those two-liner tests could be changed.

I use "git diff --exit-code expected current" instead of "diff" or
"cmp" utilities; should all (new) test use this... well of course
except ones testing diff output itself?

 t/t4025-diff-filter.sh |  240 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 240 insertions(+), 0 deletions(-)
 create mode 100755 t/t4025-diff-filter.sh

diff --git a/t/t4025-diff-filter.sh b/t/t4025-diff-filter.sh
new file mode 100755
index 0000000..3113786
--- /dev/null
+++ b/t/t4025-diff-filter.sh
@@ -0,0 +1,240 @@
+#!/bin/sh
+
+test_description='git log --diff-filter option
+
+Test --diff-filter option with git-log with and without diff output,
+checking both diff output filtering and revision list filtering.
+'
+
+. ./test-lib.sh
+. ../diff-lib.sh ;# test-lib chdir's into trash
+
+# ----------------------------------------------------------------------
+
+test_expect_success setup '
+
+	rm -f foo &&
+	cat ../../COPYING >foo &&
+	git add foo &&
+	git commit -a -m "1st commit: A" &&
+
+	cp foo bar &&
+	git add bar &&
+	git commit -a -m "2nd commit: C" &&
+
+	git rm foo &&
+	git commit -a -m "3rd commit: D" &&
+
+	echo "First added line" >> bar &&
+	git commit -a -m "4th commit: M" &&
+
+	git mv bar foo &&
+	git commit -a -m "5th commit: R" &&
+
+	rm -f foo &&
+	cat ../../Makefile >foo &&
+	git commit -a -m "6th commit: B" &&
+
+	rm -f bar &&
+	echo "bar" > bar &&
+	git add bar &&
+	echo "Second added line" >> foo &&
+	git commit -a -m "7th commit: AM"
+
+	rm -f bar &&
+	ln -s foo bar &&
+	git commit -a -m "8th commit: T"
+
+'
+
+# ----------------------------------------------------------------------
+
+cat >expected <<\EOF
+7th commit: AM
+:000000 100644 0000000000000000000000000000000000000000 5716ca5987cbf97d6bb54920bea6adde242d87e6 A	bar
+
+1st commit: A
+:000000 100644 0000000000000000000000000000000000000000 6ff87c4664981e4397625791c8ea3bbb5f2279a3 A	foo
+EOF
+
+test_expect_success 'git log --raw --diff-filter=A' '
+	git log --raw --no-abbrev --pretty=format:%s -B -C -C --diff-filter=A >current &&
+	compare_diff_raw expected current
+'
+
+
+cat >expected <<\EOF
+3rd commit: D
+:100644 000000 6ff87c4664981e4397625791c8ea3bbb5f2279a3 0000000000000000000000000000000000000000 D	foo
+EOF
+
+test_expect_success 'git log --raw --diff-filter=D' '
+	git log --raw --no-abbrev --pretty=format:%s -B -C -C --diff-filter=D >current &&
+	compare_diff_raw expected current
+'
+
+
+cat >expected <<\EOF
+7th commit: AM
+:100644 100644 21c80e6bf73163b9770cba5331cd48172fa6d43e a892bacce2a80efc14eef1c316e827575a96e5c9 M	foo
+
+4th commit: M
+:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 915b225a6c9984e645a8061e05002f8cbd2ce46c M	bar
+EOF
+
+test_expect_success 'git log --raw --diff-filter=M' '
+	git log --raw --no-abbrev --pretty=format:%s -B -C -C --diff-filter=M >current &&
+	compare_diff_raw expected current
+'
+
+
+cat >expected <<\EOF
+5th commit: R
+:100644 100644 915b225a6c9984e645a8061e05002f8cbd2ce46c 915b225a6c9984e645a8061e05002f8cbd2ce46c R100	bar	foo
+EOF
+
+test_expect_success 'git log --raw --diff-filter=R' '
+	git log --raw --no-abbrev --pretty=format:%s -B -C -C --diff-filter=R >current &&
+	compare_diff_raw expected current
+'
+
+
+cat >expected <<\EOF
+2nd commit: C
+:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 6ff87c4664981e4397625791c8ea3bbb5f2279a3 C100	foo	bar
+EOF
+
+test_expect_success 'git log --raw --diff-filter=C' '
+	git log --raw --no-abbrev --pretty=format:%s -B -C -C --diff-filter=C >current &&
+	compare_diff_raw expected current
+'
+
+
+cat >expected <<\EOF
+6th commit: B
+:100644 100644 915b225a6c9984e645a8061e05002f8cbd2ce46c 21c80e6bf73163b9770cba5331cd48172fa6d43e M098	foo
+EOF
+
+test_expect_success 'git log --raw --diff-filter=B' '
+	git log --raw --no-abbrev --pretty=format:%s -B -C -C --diff-filter=B >current &&
+	compare_diff_raw expected current
+'
+
+
+cat >expected <<\EOF
+8th commit: T
+:100644 120000 5716ca5987cbf97d6bb54920bea6adde242d87e6 19102815663d23f8b75a47e7a01965dcdc96468c T	bar
+EOF
+
+test_expect_success 'git log --raw --diff-filter=T' '
+	git log --raw --no-abbrev --pretty=format:%s -B -C -C --diff-filter=T >current &&
+	compare_diff_raw expected current
+'
+
+cat >expected <<\EOF
+7th commit: AM
+:000000 100644 0000000000000000000000000000000000000000 5716ca5987cbf97d6bb54920bea6adde242d87e6 A	bar
+:100644 100644 21c80e6bf73163b9770cba5331cd48172fa6d43e a892bacce2a80efc14eef1c316e827575a96e5c9 M	foo
+
+1st commit: A
+:000000 100644 0000000000000000000000000000000000000000 6ff87c4664981e4397625791c8ea3bbb5f2279a3 A	foo
+EOF
+
+test_expect_success 'git log --raw --diff-filter=A*' '
+	git log --raw --no-abbrev --pretty=format:%s -B -C -C --diff-filter=A* >current &&
+	compare_diff_raw expected current
+'
+
+# ----------------------------------------------------------------------
+
+cat >expected <<\EOF
+8th commit: T
+7th commit: AM
+6th commit: B
+5th commit: R
+4th commit: M
+3rd commit: D
+2nd commit: C
+1st commit: A
+EOF
+
+test_expect_success 'git log (no filter)' '
+	git log --pretty=format:%s -B -C -C >current &&
+	git diff --exit-code expected current
+'
+
+
+cat >expected <<\EOF
+7th commit: AM
+1st commit: A
+EOF
+
+test_expect_success 'git log --diff-filter=A' '
+	git log --pretty=format:%s -B -C -C --diff-filter=A >current &&
+	git diff --exit-code expected current
+'
+
+
+cat >expected <<\EOF
+3rd commit: D
+EOF
+
+test_expect_success 'git log --diff-filter=D' '
+	git log --pretty=format:%s -B -C -C --diff-filter=D >current &&
+	git diff --exit-code expected current
+'
+
+
+cat >expected <<\EOF
+7th commit: AM
+4th commit: M
+EOF
+
+test_expect_success 'git log --diff-filter=M' '
+	git log --pretty=format:%s -B -C -C --diff-filter=M >current &&
+	git diff --exit-code expected current
+'
+
+
+cat >expected <<\EOF
+5th commit: R
+EOF
+
+test_expect_success 'git log --diff-filter=R' '
+	git log --pretty=format:%s -B -C -C --diff-filter=R >current &&
+	git diff --exit-code expected current
+'
+
+
+cat >expected <<\EOF
+2nd commit: C
+EOF
+
+test_expect_success 'git log --diff-filter=C' '
+	git log --pretty=format:%s -B -C -C --diff-filter=C >current &&
+	git diff --exit-code expected current
+'
+
+
+cat >expected <<\EOF
+6th commit: B
+EOF
+
+test_expect_success 'git log --diff-filter=B' '
+	git log --pretty=format:%s -B -C -C --diff-filter=A >current &&
+	git diff --exit-code expected current
+'
+
+
+cat >expected <<\EOF
+8th commit: T
+EOF
+
+test_expect_success 'git log --diff-filter=T' '
+	git log --pretty=format:%s -B -C -C --diff-filter=T >current &&
+	git diff --exit-code expected current
+'
+
+# ----------------------------------------------------------------------
+
+test_done
-- 
1.5.3.7

Re: [PATCH] Test "git log --diff-filter"

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Add test to check "git log --diff-filter" works correctly with and
> without diff generation by git-log; the main purpose of this test is
> to check if "git log --diff-filter" filters revisions correctly.
>
> This is a companion test to commit 0faf2da7e5ee5c2f472d8a7afaf8616101f34e80
> (Fix "git log --diff-filter" bug) by Arjen Laarhoven.

If you look at the commit, you'd notice that I've added
necessary test when I accepted the patch from Arjen already ;-).

Does this new set of tests check something new?

Re: [PATCH] Test "git log --diff-filter"

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Add test to check "git log --diff-filter" works correctly with and
> > without diff generation by git-log; the main purpose of this test is
> > to check if "git log --diff-filter" filters revisions correctly.
> >
> > This is a companion test to commit 0faf2da7e5ee5c2f472d8a7afaf8616101f34e80
> > (Fix "git log --diff-filter" bug) by Arjen Laarhoven.
> 
> If you look at the commit, you'd notice that I've added
> necessary test when I accepted the patch from Arjen already ;-).

Sorry for the noise, then.

> Does this new set of tests check something new?

My test checks all --diff-filter filters relevant to git-diff-tree,
i.e. ADMRCBT, and not only AMD.

Also it checks if the diff is shown correctly for --diff-filter=M and
for --diff-filter=M*, but I think this should be a separate test, and
use only git-diff-something, and not git-log.


P.S. By the way, it is IMHO a bit strange that --pretty=oneline uses
newline as a terminator (it means that there is a newline at the end of
"git log --pretty=oneline), while --pretty="format:%s" uses newline as
a separator (meaning that there is no newline at the end) when redirected
to file.

 # git log --pretty="format:%s" -B -C -C >current

The 'current' file doesn't end with newline (with --pretty=oneline it
does), when log ends at root commit. Strange.

-- 
Jakub Narebski
Poland

Re: [PATCH] Test "git log --diff-filter"

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> My test checks all --diff-filter filters relevant to git-diff-tree,
> i.e. ADMRCBT, and not only AMD.

Ah, I see.  Thanks --- that could have been stated in the log
message.  Maybe we would want to add them to existing test
script, instead of adding a whole new one?

> P.S. By the way, it is IMHO a bit strange that --pretty=oneline uses
> newline as a terminator (it means that there is a newline at the end of
> "git log --pretty=oneline), while --pretty="format:%s" uses newline as
> a separator...

Yeah, I tend to agree, although I learned to live with it long
time ago.

Re: [PATCH] Test "git log --diff-filter"

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > My test checks all --diff-filter filters relevant to git-diff-tree,
> > i.e. ADMRCBT, and not only AMD.
> 
> Ah, I see.  Thanks --- that could have been stated in the log
> message.  Maybe we would want to add them to existing test
> script, instead of adding a whole new one?

The test as it stands now checks if --diff-filter select appropriate
revisions, even without patch output. I think it is enough, as I don't
see how we could screw up to filter AMD correctly, and not all others...
...perhaps with exception of pair breaking, and how they are filtered
using --diff-filter=M and --diff-filter=B; but this impression might
be caused by the fact that pair breaking is the only one which doesn't
use symbol ('B') in raw diff format output.

> > P.S. By the way, it is IMHO a bit strange that --pretty=oneline uses
> > newline as a terminator (it means that there is a newline at the end of
> > "git log --pretty=oneline), while --pretty="format:%s" uses newline as
> > a separator...
> 
> Yeah, I tend to agree, although I learned to live with it long
> time ago.

IMHO that is design bug. Perhaps it should be changed? This way, at least
conceptually oneline, short, medium, full, fuller, email formats might be
considered simply pre-defined format:<sth> formats.

Am I mistaken in thinking that the rest of git always use terminators,
and not separators for records output?
-- 
Jakub Narebski
Poland

Re: [PATCH] Test "git log --diff-filter"

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Am I mistaken in thinking that the rest of git always use terminators,
> and not separators for records output?

Actually, for normal "git log", separator semantics is the right
thing to use.  You do not want to add an additional trailing
newline when you show only one.  Compare these two to see what I
mean:

	$ git log -1
        $ git log -2

The problem is with --pretty=format.