BUG: git log: fatal: internal error in diff-resolve-rename-copy

BUG: git log: fatal: internal error in diff-resolve-rename-copy

[Constantine Plotnikov]

Somewhere between the git 1.7.0.2 and the git 1.7.2.0 the rename
detection started to fail with fatal error on some files in our
repository. The bug could be seen on the public IntelliJ IDEA
repository (about 760M in size), but our users have reported it as
well.

To reproduce the error, run the following sequence of the commands:

git clone git://git.jetbrains.org/idea/community.git idea
cd idea
git log -M --follow --name-only --
platform/lang-api/src/com/intellij/lang/documentation/CompositeDocumentationProvider.java

As result "fatal: internal error in diff-resolve-rename-copy" is
written on stderr. This is somewhat unexpected result. Git 1.7.0.2 and
1.6.5.2 seems to work without visible problems.

Regards,
Constantine

Re: BUG: git log: fatal: internal error in diff-resolve-rename-copy

[Ævar Arnfjörð Bjarmason]

On Fri, Aug 13, 2010 at 11:25, Constantine Plotnikov
<constantine.plotnikov@gmail.com> wrote:
> Somewhere between the git 1.7.0.2 and the git 1.7.2.0 the rename
> detection started to fail with fatal error on some files in our
> repository. The bug could be seen on the public IntelliJ IDEA
> repository (about 760M in size), but our users have reported it as
> well.

>From a bisect:

    1da6175d438a9849db07a68326ee05f291510074 is the first bad commit
    commit 1da6175d438a9849db07a68326ee05f291510074
    Author: Bo Yang <struggleyb.nku@gmail.com>
    Date:   Thu May 6 21:52:28 2010 -0700

        Make diffcore_std only can run once before a diff_flush

        When file renames/copies detection is turned on, the
        second diffcore_std will degrade a 'C' pair to a 'R' pair.

        And this may happen when we run 'git log --follow' with
        hard copies finding. That is, the try_to_follow_renames()
        will run diffcore_std to find the copies, and then
        'git log' will issue another diffcore_std, which will reduce
        'src->rename_used' and recognize this copy as a rename.
        This is not what we want.

        So, I think we really don't need to run diffcore_std more
        than one time.

        Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
        Signed-off-by: Junio C Hamano <gitster@pobox.com>

Bisect script:

    #!/bin/sh

    cd ~/g/git
    make clean
    make -j 10 CC=clang CFLAGS=-O0

    ./git --git-dir=/tmp/idea/.git log -M --follow --name-only --
platform/lang-api/src/com/intellij/lang/documentation/CompositeDocumentationProvider.java
> /dev/null
    ret=$?
    test $ret -gt 127 && ret=127
    exit $ret

Re: BUG: git log: fatal: internal error in diff-resolve-rename-copy

[Junio C Hamano]

Constantine Plotnikov <constantine.plotnikov@gmail.com> writes:

> Somewhere between the git 1.7.0.2 and the git 1.7.2.0 the rename
> detection started to fail with fatal error on some files in our
> repository. The bug could be seen on the public IntelliJ IDEA
> repository (about 760M in size), but our users have reported it as
> well.
>
> To reproduce the error, run the following sequence of the commands:
>
> git clone git://git.jetbrains.org/idea/community.git idea
> cd idea
> git log -M --follow --name-only --
> platform/lang-api/src/com/intellij/lang/documentation/CompositeDocumentationProvider.java
>
> As result "fatal: internal error in diff-resolve-rename-copy" is
> written on stderr. This is somewhat unexpected result. Git 1.7.0.2 and
> 1.6.5.2 seems to work without visible problems.

This is interesting.  I actually see another potential "funny" (and that
is why Linus is CC'ed --- scroll down to "funny"), which is unrelated.

diff_tree_sha1() essentially:

 - given two trees, runs straightforward diff-tree without any rename
   detection;

 - under --follow mode, if it has only one change that deletes a path,
   runs try_to_follow_renames(), which:

   - runs the same diff-tree with deep rename detection but without any
     path limiter;

   - if we find a rename or copy that explains why we saw the deletion in
     the first step, replace the deletion record with the rename or copy.
     also set up to follow the old name from now on.

 - then return to the caller.

The diff frontends (diff-tree, diff-files and diff-index) are expected to
leave vanilla filepairs and let the diffcore backend to find renames and
copies via a call to diff_resolve_rename_copy() in diffcore_std().

We however have a special "hack" in "diff-tree --follow".  If it finds
only one diff_queue entry that creates a path, it internally runs itself
again with the rename/copy detection on, without any pathspec, to see if
the creation is an artifact of the pathspec (iow, if there is a source of
rename/copy into the created path), and replaces that creation record with
a rename record in the diff_queue.  When this is done, we do not want the
regular resolve_rename_copy() to kick in (i.e. the "follow" hack already
made a pair).

But what 1da6175 (Make diffcore_std only can run once before a diff_flush,
2010-05-06) did is clearly wrong.  Not wanting to call resolve-rename-copy
does not mean we do not want to run the rest of what diffcore_std() does
at all!  For example, "-S" and "--diff-filter=" options are processed in
that function; the exit status of the command based on the presense of
difference is computed in the function, too.

Another potential "funny" is this (unrelated to the reported issue).

The "--follow" logic is called from diff_tree_sha1() function, but the
input trees to diff_tree_sha1() are not necessarily the top-level trees
(compare_tree_entry() calls it while it recursively descends into
subtrees).  For example, with the example Constantine gave us, the first
"try-to-follow-renames" call happens with the "base" set to "platform/"
but the rename source is actually "lang-api/src/com/intellij/..."
hierarchy, so it is a wasted call.  I think we only want to run the rename
following at the very top level, i.e. like the attached patch.

Linus, what do you think?  Am I missing something obvious?

diff --git a/tree-diff.c b/tree-diff.c
index 1fb3e94..5b68c08 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -412,7 +412,7 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha
 	init_tree_desc(&t1, tree1, size1);
 	init_tree_desc(&t2, tree2, size2);
 	retval = diff_tree(&t1, &t2, base, opt);
-	if (DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) {
+	if (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) {
 		init_tree_desc(&t1, tree1, size1);
 		init_tree_desc(&t2, tree2, size2);
 		try_to_follow_renames(&t1, &t2, base, opt);

Re: BUG: git log: fatal: internal error in diff-resolve-rename-copy

[Linus Torvalds]

On Fri, Aug 13, 2010 at 10:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Another potential "funny" is this (unrelated to the reported issue).
>
> The "--follow" logic is called from diff_tree_sha1() function, but the
> input trees to diff_tree_sha1() are not necessarily the top-level trees
> (compare_tree_entry() calls it while it recursively descends into
> subtrees).  For example, with the example Constantine gave us, the first
> "try-to-follow-renames" call happens with the "base" set to "platform/"
> but the rename source is actually "lang-api/src/com/intellij/..."
> hierarchy, so it is a wasted call.  I think we only want to run the rename
> following at the very top level, i.e. like the attached patch.
>
> Linus, what do you think?  Am I missing something obvious?

I think you're right. You're certainly not missing anythng _obvious_.
Iirc the only reason I put that 'follow' hack in diff_tree_sha1() was
because that way I didn't need to worry about all the callers, but I
didn't even think about the whole recursion issue.

So ack on that.

                Linus

Re: BUG: git log: fatal: internal error in diff-resolve-rename-copy

[Junio C Hamano]

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

> Constantine Plotnikov <constantine.plotnikov@gmail.com> writes:
>
>> Somewhere between the git 1.7.0.2 and the git 1.7.2.0 the rename
>> detection started to fail with fatal error on some files in our
>> repository. The bug could be seen on the public IntelliJ IDEA
>> repository (about 760M in size), but our users have reported it as
>> well.
> ...
> But what 1da6175 (Make diffcore_std only can run once before a diff_flush,
> 2010-05-06) did is clearly wrong.  Not wanting to call resolve-rename-copy
> does not mean we do not want to run the rest of what diffcore_std() does
> at all!  For example, "-S" and "--diff-filter=" options are processed in
> that function; the exit status of the command based on the presense of
> difference is computed in the function, too.

This reverts 1da6175 (Make diffcore_std only can run once before a
diff_flush, 2010-05-06) and replaces it with an uglier looking but
hopefully correct fix.

Constantine, does it fix your issue?

 diff.c      |   27 +++++++++++++--------------
 diff.h      |    3 +++
 diffcore.h  |    2 --
 tree-diff.c |   12 ++++++++++++
 4 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/diff.c b/diff.c
index bf65892..9300492 100644
--- a/diff.c
+++ b/diff.c
@@ -4064,25 +4064,24 @@ void diffcore_fix_diff_index(struct diff_options *options)
 
 void diffcore_std(struct diff_options *options)
 {
-	/* We never run this function more than one time, because the
-	 * rename/copy detection logic can only run once.
-	 */
-	if (diff_queued_diff.run)
-		return;
-
 	if (options->skip_stat_unmatch)
 		diffcore_skip_stat_unmatch(options);
-	if (options->break_opt != -1)
-		diffcore_break(options->break_opt);
-	if (options->detect_rename)
-		diffcore_rename(options);
-	if (options->break_opt != -1)
-		diffcore_merge_broken();
+	if (!options->found_follow) {
+		/* See try_to_follow_renames() in tree-diff.c */
+		if (options->break_opt != -1)
+			diffcore_break(options->break_opt);
+		if (options->detect_rename)
+			diffcore_rename(options);
+		if (options->break_opt != -1)
+			diffcore_merge_broken();
+	}
 	if (options->pickaxe)
 		diffcore_pickaxe(options->pickaxe, options->pickaxe_opts);
 	if (options->orderfile)
 		diffcore_order(options->orderfile);
-	diff_resolve_rename_copy();
+	if (!options->found_follow)
+		/* See try_to_follow_renames() in tree-diff.c */
+		diff_resolve_rename_copy();
 	diffcore_apply_filter(options->filter);
 
 	if (diff_queued_diff.nr && !DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
@@ -4090,7 +4089,7 @@ void diffcore_std(struct diff_options *options)
 	else
 		DIFF_OPT_CLR(options, HAS_CHANGES);
 
-	diff_queued_diff.run = 1;
+	options->found_follow = 0;
 }
 
 int diff_result_code(struct diff_options *opt, int status)
diff --git a/diff.h b/diff.h
index 063d10a..6fff024 100644
--- a/diff.h
+++ b/diff.h
@@ -126,6 +126,9 @@ struct diff_options {
 	/* this is set by diffcore for DIFF_FORMAT_PATCH */
 	int found_changes;
 
+	/* to support internal diff recursion by --follow hack*/
+	int found_follow;
+
 	FILE *file;
 	int close_file;
 
diff --git a/diffcore.h b/diffcore.h
index 05ebc11..8b3241a 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -91,13 +91,11 @@ struct diff_queue_struct {
 	struct diff_filepair **queue;
 	int alloc;
 	int nr;
-	int run;
 };
 #define DIFF_QUEUE_CLEAR(q) \
 	do { \
 		(q)->queue = NULL; \
 		(q)->nr = (q)->alloc = 0; \
-		(q)->run = 0; \
 	} while (0)
 
 extern struct diff_queue_struct diff_queued_diff;
diff --git a/tree-diff.c b/tree-diff.c
index 5b68c08..293cc91 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -350,6 +350,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	diff_opts.single_follow = opt->paths[0];
 	diff_opts.break_opt = opt->break_opt;
+
 	paths[0] = NULL;
 	diff_tree_setup_paths(paths, &diff_opts);
 	if (diff_setup_done(&diff_opts) < 0)
@@ -359,6 +360,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 	diff_tree_release_paths(&diff_opts);
 
 	/* Go through the new set of filepairing, and see if we find a more interesting one */
+	opt->found_follow = 0;
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 
@@ -376,6 +378,16 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 			diff_tree_release_paths(opt);
 			opt->paths[0] = xstrdup(p->one->path);
 			diff_tree_setup_paths(opt->paths, opt);
+
+			/*
+			 * The caller expects us to return a set of vanilla
+			 * filepairs to let a later call to diffcore_std()
+			 * it makes to sort the renames out (among other
+			 * things), but we already have found renames
+			 * ourselves; signal diffcore_std() not to muck with
+			 * rename information.
+			 */
+			opt->found_follow = 1;
 			break;
 		}
 	}

[PATCH] diff --follow: do call diffcore_std() as necessary

[Junio C Hamano]

Usually, diff frontends populate the output queue with filepairs without
any rename information and call diffcore_std() to sort the renames out.
When --follow is in effect, however, diff-tree family of frontend has a
hack that looks like this:

    diff-tree frontend
    -> diff_tree_sha1()
       . populate diff_queued_diff
       . if --follow is in effect and there is only one change that
         creates the target path, then
       -> try_to_follow_renames()
	  -> diff_tree_sha1() with no pathspec but with -C
	  -> diffcore_std() to find renames
	  . if rename is found, tweak diff_queued_diff and put a
	    single filepair that records the found rename there
    -> diffcore_std()
       . tweak elements on diff_queued_diff by
       - rename detection
       - path ordering
       - pickaxe filtering

We need to skip parts of the second call to diffcore_std() that is related
to rename detection, and do so only when try_to_follow_renames() did find
a rename.  Earlier 1da6175 (Make diffcore_std only can run once before a
diff_flush, 2010-05-06) tried to deal with this issue incorrectly; it
unconditionally disabled any second call to diffcore_std().

This hopefully fixes the breakage.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This time with a proposed commit message for a change.  Still seems to
   pass the test in 0cdca13 (Make git log --follow find copies among
   unmodified files., 2010-05-06), presumably added for 1da6175.

 diff.c      |   27 +++++++++++++--------------
 diff.h      |    3 +++
 diffcore.h  |    2 --
 tree-diff.c |   11 +++++++++++
 4 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/diff.c b/diff.c
index bf65892..9300492 100644
--- a/diff.c
+++ b/diff.c
@@ -4064,25 +4064,24 @@ void diffcore_fix_diff_index(struct diff_options *options)
 
 void diffcore_std(struct diff_options *options)
 {
-	/* We never run this function more than one time, because the
-	 * rename/copy detection logic can only run once.
-	 */
-	if (diff_queued_diff.run)
-		return;
-
 	if (options->skip_stat_unmatch)
 		diffcore_skip_stat_unmatch(options);
-	if (options->break_opt != -1)
-		diffcore_break(options->break_opt);
-	if (options->detect_rename)
-		diffcore_rename(options);
-	if (options->break_opt != -1)
-		diffcore_merge_broken();
+	if (!options->found_follow) {
+		/* See try_to_follow_renames() in tree-diff.c */
+		if (options->break_opt != -1)
+			diffcore_break(options->break_opt);
+		if (options->detect_rename)
+			diffcore_rename(options);
+		if (options->break_opt != -1)
+			diffcore_merge_broken();
+	}
 	if (options->pickaxe)
 		diffcore_pickaxe(options->pickaxe, options->pickaxe_opts);
 	if (options->orderfile)
 		diffcore_order(options->orderfile);
-	diff_resolve_rename_copy();
+	if (!options->found_follow)
+		/* See try_to_follow_renames() in tree-diff.c */
+		diff_resolve_rename_copy();
 	diffcore_apply_filter(options->filter);
 
 	if (diff_queued_diff.nr && !DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
@@ -4090,7 +4089,7 @@ void diffcore_std(struct diff_options *options)
 	else
 		DIFF_OPT_CLR(options, HAS_CHANGES);
 
-	diff_queued_diff.run = 1;
+	options->found_follow = 0;
 }
 
 int diff_result_code(struct diff_options *opt, int status)
diff --git a/diff.h b/diff.h
index 063d10a..6fff024 100644
--- a/diff.h
+++ b/diff.h
@@ -126,6 +126,9 @@ struct diff_options {
 	/* this is set by diffcore for DIFF_FORMAT_PATCH */
 	int found_changes;
 
+	/* to support internal diff recursion by --follow hack*/
+	int found_follow;
+
 	FILE *file;
 	int close_file;
 
diff --git a/diffcore.h b/diffcore.h
index 05ebc11..8b3241a 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -91,13 +91,11 @@ struct diff_queue_struct {
 	struct diff_filepair **queue;
 	int alloc;
 	int nr;
-	int run;
 };
 #define DIFF_QUEUE_CLEAR(q) \
 	do { \
 		(q)->queue = NULL; \
 		(q)->nr = (q)->alloc = 0; \
-		(q)->run = 0; \
 	} while (0)
 
 extern struct diff_queue_struct diff_queued_diff;
diff --git a/tree-diff.c b/tree-diff.c
index 5b68c08..cd659c6 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -359,6 +359,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 	diff_tree_release_paths(&diff_opts);
 
 	/* Go through the new set of filepairing, and see if we find a more interesting one */
+	opt->found_follow = 0;
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 
@@ -376,6 +377,16 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 			diff_tree_release_paths(opt);
 			opt->paths[0] = xstrdup(p->one->path);
 			diff_tree_setup_paths(opt->paths, opt);
+
+			/*
+			 * The caller expects us to return a set of vanilla
+			 * filepairs to let a later call to diffcore_std()
+			 * it makes to sort the renames out (among other
+			 * things), but we already have found renames
+			 * ourselves; signal diffcore_std() not to muck with
+			 * rename information.
+			 */
+			opt->found_follow = 1;
 			break;
 		}
 	}
-- 
1.7.2.1.186.gffe84

Re: [PATCH] diff --follow: do call diffcore_std() as necessary

[Ævar Arnfjörð Bjarmason]

On Fri, Aug 13, 2010 at 19:46, Junio C Hamano <gitster@pobox.com> wrote:

> This hopefully fixes the breakage.

Hopefully. It'd also be nice if we had a regression test for this, but
I don't know how hard that would be to arrange. If it's hard to
reproduce we might get away with a filter-branch + subdir filter from
the idea repository.

Re: [PATCH] diff --follow: do call diffcore_std() as necessary

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Aug 13, 2010 at 19:46, Junio C Hamano <gitster@pobox.com> wrote:
>
>> This hopefully fixes the breakage.
>
> Hopefully. It'd also be nice if we had a regression test for this, but
> I don't know how hard that would be to arrange. If it's hard to
> reproduce we might get away with a filter-branch + subdir filter from
> the idea repository.

As I wrote, the test added by 0cdca13 breaks with just a reversion of
1da6175 but with this patch it passes.  I didn't run any other test,
though ;-).

[PATCH] log: test for regression introduced in v1.7.2-rc0~103^2~2

[Ævar Arnfjörð Bjarmason]

Add a regression test for the git log -M --follow --name-only bug
introduced in v1.7.2-rc0~103^2~2

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4202-log.sh |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 95ac3f8..ff624f4 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -441,5 +441,14 @@ test_expect_success 'log.decorate configuration' '
 
 '
 
+test_expect_success 'Regression test for v1.7.2-rc0~103^2~2' '
+	# Needs an unrelated root commit
+	test_commit README &&
+	>Foo.bar &&
+	git add Foo.bar &&
+	git commit --allow-empty-message </dev/null &&
+	git log -M --follow --name-only Foo.bar
+'
+
 test_done
 
-- 
1.7.2.1.338.ge1a5e

Re: [PATCH] log: test for regression introduced in v1.7.2-rc0~103^2~2

[Ævar Arnfjörð Bjarmason]

On Sat, Aug 14, 2010 at 01:10, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Add a regression test for the git log -M --follow --name-only bug
> introduced in v1.7.2-rc0~103^2~2

AKA "we didn't have any tests for log's --name-only *at all*".

Re: [PATCH] log: test for regression introduced in v1.7.2-rc0~103^2~2

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Sat, Aug 14, 2010 at 01:10, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> Add a regression test for the git log -M --follow --name-only bug
>> introduced in v1.7.2-rc0~103^2~2
>
> AKA "we didn't have any tests for log's --name-only *at all*".

But this is not related to --name-only at all; anything that is "diff"
related, e.g. -p, --stat, --name-status, will share the same issue.

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 95ac3f8..ff624f4 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -441,5 +441,14 @@ test_expect_success 'log.decorate configuration' '
>  
>  '
>  
> +test_expect_success 'Regression test for v1.7.2-rc0~103^2~2' '

This is uninformative and ugly at the same time.

 - Can't we describe the nature of the situation where the old bug
   triggers concisely?  Perhaps 'show added path under "--follow -M"?'

 - All others begin with lowercase.

> +	# Needs an unrelated root commit
> +	test_commit README &&

This is not a "root" commit, is it?

> +	>Foo.bar &&
> +	git add Foo.bar &&
> +	git commit --allow-empty-message </dev/null &&

Does emptiness of the message matter?

> +	git log -M --follow --name-only Foo.bar
> +'
> +
>  test_done

Re: [PATCH] log: test for regression introduced in v1.7.2-rc0~103^2~2

[Ævar Arnfjörð Bjarmason]

On Sun, Aug 15, 2010 at 09:08, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Sat, Aug 14, 2010 at 01:10, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>> Add a regression test for the git log -M --follow --name-only bug
>>> introduced in v1.7.2-rc0~103^2~2
>>
>> AKA "we didn't have any tests for log's --name-only *at all*".
>
> But this is not related to --name-only at all; anything that is "diff"
> related, e.g. -p, --stat, --name-status, will share the same issue.

I meant that as an extra benefit this is the first test for log +
--name-only.

>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 95ac3f8..ff624f4 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -441,5 +441,14 @@ test_expect_success 'log.decorate configuration' '
>>
>>  '
>>
>> +test_expect_success 'Regression test for v1.7.2-rc0~103^2~2' '
>
> This is uninformative and ugly at the same time.
>
>  - Can't we describe the nature of the situation where the old bug
>   triggers concisely?  Perhaps 'show added path under "--follow -M"?'

I didn't grok why this was happening, but yeah, that description is
better.

>> +     # Needs an unrelated root commit
>> +     test_commit README &&
>
> This is not a "root" commit, is it?

s/root/first/

>> +     >Foo.bar &&
>> +     git add Foo.bar &&
>> +     git commit --allow-empty-message </dev/null &&
>
> Does emptiness of the message matter?

No, I was just going for a minimal test case, no commit message is
more minimal than having one.

Re: [PATCH] log: test for regression introduced in v1.7.2-rc0~103^2~2

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>> +     # Needs an unrelated root commit
>>> +     test_commit README &&
>>
>> This is not a "root" commit, is it?
>
> s/root/first/

It is not even the first commit, is it?  It comes on top of whatever
commits that earlier tests left.

>>> +     >Foo.bar &&
>>> +     git add Foo.bar &&
>>> +     git commit --allow-empty-message </dev/null &&
>>
>> Does emptiness of the message matter?
>
> No, I was just going for a minimal test case, no commit message is
> more minimal than having one.

I do not think having to write "--allow-empty-message </dev/null" is
aiming for being minimal; it is doing something unusual after all.

If you do not remember why you added this test 6 months down the road,
wouldn't you be confused to think maybe the commit has to be unusual in
that it has to lack the message to trigger the bug?

Re: [PATCH] log: test for regression introduced in v1.7.2-rc0~103^2~2

[Ævar Arnfjörð Bjarmason]

On Mon, Aug 16, 2010 at 01:49, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>>> +     # Needs an unrelated root commit
>>>> +     test_commit README &&
>>>
>>> This is not a "root" commit, is it?
>>
>> s/root/first/
>
> It is not even the first commit, is it?  It comes on top of whatever
> commits that earlier tests left.
>
>>>> +     >Foo.bar &&
>>>> +     git add Foo.bar &&
>>>> +     git commit --allow-empty-message </dev/null &&
>>>
>>> Does emptiness of the message matter?
>>
>> No, I was just going for a minimal test case, no commit message is
>> more minimal than having one.
>
> I do not think having to write "--allow-empty-message </dev/null" is
> aiming for being minimal; it is doing something unusual after all.
>
> If you do not remember why you added this test 6 months down the road,
> wouldn't you be confused to think maybe the commit has to be unusual in
> that it has to lack the message to trigger the bug?

My v2 patch should address both of these issues.

[PATCH v2] log: test for regression introduced in v1.7.2-rc0~103^2~2

[Ævar Arnfjörð Bjarmason]

Add a regression test for the git log -M --follow $diff_option bug
introduced in v1.7.2-rc0~103^2~2, $diff_option being diff related
options like -p, --stat, --name-only etc.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Version two of this test case, simpler, and takes into account
commentary from Junio.

 t/t4202-log.sh |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 95ac3f8..a0be122 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -441,5 +441,18 @@ test_expect_success 'log.decorate configuration' '
 
 '
 
+test_expect_success 'show added path under "--follow -M"' '
+	# This tests for a regression introduced in v1.7.2-rc0~103^2~2
+	test_create_repo regression &&
+	(
+		cd regression &&
+		test_commit needs-another-commit &&
+		test_commit Foo.bar &&
+		git log -M --follow -p Foo.bar.t &&
+		git log -M --follow --stat Foo.bar.t &&
+		git log -M --follow --name-only Foo.bar.t
+	)
+'
+
 test_done
 
-- 
1.7.2.1.339.gfad93

Re: BUG: git log: fatal: internal error in diff-resolve-rename-copy

[Constantine Plotnikov]

On Fri, Aug 13, 2010 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Constantine Plotnikov <constantine.plotnikov@gmail.com> writes:
>>
>>> Somewhere between the git 1.7.0.2 and the git 1.7.2.0 the rename
>>> detection started to fail with fatal error on some files in our
>>> repository. The bug could be seen on the public IntelliJ IDEA
>>> repository (about 760M in size), but our users have reported it as
>>> well.
>> ...
>> But what 1da6175 (Make diffcore_std only can run once before a diff_flush,
>> 2010-05-06) did is clearly wrong.  Not wanting to call resolve-rename-copy
>> does not mean we do not want to run the rest of what diffcore_std() does
>> at all!  For example, "-S" and "--diff-filter=" options are processed in
>> that function; the exit status of the command based on the presense of
>> difference is computed in the function, too.
>
> This reverts 1da6175 (Make diffcore_std only can run once before a
> diff_flush, 2010-05-06) and replaces it with an uglier looking but
> hopefully correct fix.
>
> Constantine, does it fix your issue?
>
Yes. It fixes the issue on the known problematic files. The patch was
tested over 3235b7053 in the git.git master. Please apply it to the
next update.

Thank You,
Constantine