[PATCH 0/2 v3] Make git log --follow find copies among unmodified files.

[PATCH 0/2 v3] Make git log --follow find copies among unmodified files.

[Bo Yang]

I have tried to make --follow to support finding copies among unmodified files. And the first patch is to fix a bug introduced by '--follow' and 'git log' combination.
We use the code:

    else if (--p->one->rename_used > 0)
        p->status = DIFF_STATUS_COPIED;

to detect copies and renames. So, if diffcore_std run more than one time, p->one->rename_used will be reduced to a 'R' from 'C'. And this patch will fix this by allowing diffcore_std can only run once before a diff_flush, which seems rationale for our code.

Bo Yang (2):
  Make diffcore_std only can run once before a diff_flush.
  Make git log --follow find copies among unmodified files.

 Documentation/git-log.txt           |    2 +-
 diff.c                              |   21 ++++++++-----
 diffcore-break.c                    |    6 +--
 diffcore-pickaxe.c                  |    3 +-
 diffcore-rename.c                   |    3 +-
 diffcore.h                          |    6 ++++
 t/t4205-log-follow-harder-copies.sh |   56 +++++++++++++++++++++++++++++++++++
 tree-diff.c                         |    2 +-
 8 files changed, 81 insertions(+), 18 deletions(-)
 create mode 100755 t/t4205-log-follow-harder-copies.sh

[PATCH 1/2 v3] Make diffcore_std only can run once before a diff_flush.

[Bo Yang]

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>
---
 diff.c             |   21 +++++++++++++--------
 diffcore-break.c   |    6 ++----
 diffcore-pickaxe.c |    3 +--
 diffcore-rename.c  |    3 +--
 diffcore.h         |    6 ++++++
 5 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/diff.c b/diff.c
index d0ecbc3..d32fc68 100644
--- a/diff.c
+++ b/diff.c
@@ -2544,6 +2544,7 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 void diff_setup(struct diff_options *options)
 {
 	memset(options, 0, sizeof(*options));
+	memset(&diff_queued_diff, 0, sizeof(diff_queued_diff));
 
 	options->file = stdout;
 
@@ -3462,8 +3463,7 @@ int diff_flush_patch_id(struct diff_options *options, unsigned char *sha1)
 		diff_free_filepair(q->queue[i]);
 
 	free(q->queue);
-	q->queue = NULL;
-	q->nr = q->alloc = 0;
+	DIFF_QUEUE_CLEAR(q);
 
 	return result;
 }
@@ -3591,8 +3591,7 @@ void diff_flush(struct diff_options *options)
 		diff_free_filepair(q->queue[i]);
 free_queue:
 	free(q->queue);
-	q->queue = NULL;
-	q->nr = q->alloc = 0;
+	DIFF_QUEUE_CLEAR(q);
 	if (options->close_file)
 		fclose(options->file);
 
@@ -3614,8 +3613,7 @@ static void diffcore_apply_filter(const char *filter)
 	int i;
 	struct diff_queue_struct *q = &diff_queued_diff;
 	struct diff_queue_struct outq;
-	outq.queue = NULL;
-	outq.nr = outq.alloc = 0;
+	DIFF_QUEUE_CLEAR(&outq);
 
 	if (!filter)
 		return;
@@ -3683,8 +3681,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)
 	int i;
 	struct diff_queue_struct *q = &diff_queued_diff;
 	struct diff_queue_struct outq;
-	outq.queue = NULL;
-	outq.nr = outq.alloc = 0;
+	DIFF_QUEUE_CLEAR(&outq);
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
@@ -3745,6 +3742,12 @@ 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)
@@ -3764,6 +3767,8 @@ void diffcore_std(struct diff_options *options)
 		DIFF_OPT_SET(options, HAS_CHANGES);
 	else
 		DIFF_OPT_CLR(options, HAS_CHANGES);
+
+	diff_queued_diff.run = 1;
 }
 
 int diff_result_code(struct diff_options *opt, int status)
diff --git a/diffcore-break.c b/diffcore-break.c
index 3a7b60a..44f8678 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -162,8 +162,7 @@ void diffcore_break(int break_score)
 	if (!merge_score)
 		merge_score = DEFAULT_MERGE_SCORE;
 
-	outq.nr = outq.alloc = 0;
-	outq.queue = NULL;
+	DIFF_QUEUE_CLEAR(&outq);
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
@@ -256,8 +255,7 @@ void diffcore_merge_broken(void)
 	struct diff_queue_struct outq;
 	int i, j;
 
-	outq.nr = outq.alloc = 0;
-	outq.queue = NULL;
+	DIFF_QUEUE_CLEAR(&outq);
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index d0ef839..929de15 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -55,8 +55,7 @@ void diffcore_pickaxe(const char *needle, int opts)
 	int i, has_changes;
 	regex_t regex, *regexp = NULL;
 	struct diff_queue_struct outq;
-	outq.queue = NULL;
-	outq.nr = outq.alloc = 0;
+	DIFF_QUEUE_CLEAR(&outq);
 
 	if (opts & DIFF_PICKAXE_REGEX) {
 		int err;
diff --git a/diffcore-rename.c b/diffcore-rename.c
index d6fd3ca..df41be5 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -569,8 +569,7 @@ void diffcore_rename(struct diff_options *options)
 	/* At this point, we have found some renames and copies and they
 	 * are recorded in rename_dst.  The original list is still in *q.
 	 */
-	outq.queue = NULL;
-	outq.nr = outq.alloc = 0;
+	DIFF_QUEUE_CLEAR(&outq);
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
 		struct diff_filepair *pair_to_free = NULL;
diff --git a/diffcore.h b/diffcore.h
index fcd00bf..e77db31 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -91,7 +91,13 @@ 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 = (q)->run = 0; \
+	} while(0);
 
 extern struct diff_queue_struct diff_queued_diff;
 extern struct diff_filepair *diff_queue(struct diff_queue_struct *,
-- 
1.7.0.2.273.gc2413.dirty

[PATCH 2/2 v3] Make git log --follow find copies among unmodified files.

[Bo Yang]

'git log --follow <path>' don't track copies from unmodified
files, and this patch fix it.

Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
---
 Documentation/git-log.txt           |    2 +-
 t/t4205-log-follow-harder-copies.sh |   56 +++++++++++++++++++++++++++++++++++
 tree-diff.c                         |    2 +-
 3 files changed, 58 insertions(+), 2 deletions(-)
 create mode 100755 t/t4205-log-follow-harder-copies.sh

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index fb184ba..0727818 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -56,7 +56,7 @@ include::diff-options.txt[]
 	commits, and doesn't limit diff for those commits.
 
 --follow::
-	Continue listing the history of a file beyond renames.
+	Continue listing the history of a file beyond renames/copies.
 
 --log-size::
 	Before the log message print out its size in bytes. Intended
diff --git a/t/t4205-log-follow-harder-copies.sh b/t/t4205-log-follow-harder-copies.sh
new file mode 100755
index 0000000..ad29e65
--- /dev/null
+++ b/t/t4205-log-follow-harder-copies.sh
@@ -0,0 +1,56 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Bo Yang
+#
+
+test_description='Test --follow should always find copies hard in git log.
+
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/diff-lib.sh
+
+echo >path0 'Line 1
+Line 2
+Line 3
+'
+
+test_expect_success \
+    'add a file path0 and commit.' \
+    'git add path0 &&
+     git commit -m "Add path0"'
+
+echo >path0 'New line 1
+New line 2
+New line 3
+'
+test_expect_success \
+    'Change path0.' \
+    'git add path0 &&
+     git commit -m "Change path0"'
+
+cat <path0 >path1
+test_expect_success \
+    'copy path0 to path1.' \
+    'git add path1 &&
+     git commit -m "Copy path1 from path0"'
+
+test_expect_success \
+    'find the copy path0 -> path1 harder' \
+    'git log --follow --name-status --pretty="format:%s"  path1 > current'
+
+cat >expected <<\EOF
+Copy path1 from path0
+C100	path0	path1
+
+Change path0
+M	path0
+
+Add path0
+A	path0
+EOF
+
+test_expect_success \
+    'validate the output.' \
+    'compare_diff_patch current expected'
+
+test_done
diff --git a/tree-diff.c b/tree-diff.c
index fe9f52c..1fb3e94 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -346,7 +346,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 
 	diff_setup(&diff_opts);
 	DIFF_OPT_SET(&diff_opts, RECURSIVE);
-	diff_opts.detect_rename = DIFF_DETECT_RENAME;
+	DIFF_OPT_SET(&diff_opts, FIND_COPIES_HARDER);
 	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	diff_opts.single_follow = opt->paths[0];
 	diff_opts.break_opt = opt->break_opt;
-- 
1.7.0.2.273.gc2413.dirty

Re: [PATCH 1/2 v3] Make diffcore_std only can run once before a diff_flush.

[Junio C Hamano]

Bo Yang <struggleyb.nku@gmail.com> writes:

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

It actually is stronger than that; we should never run it more than once,
and it would be a bug if we did so.  Which codepath tries to call *_std()
twice?

The standard calling sequence is:

 - start from an empty queue.

 - use diff_change() and diff_addremove() to populate the queue.

 - call diffcore_std(). if you need to use a non-standard chain of
   diffcore transformations, you _could_ call the diffcore_* routines that
   diffcore_std() calls, if you choose to, but as you found out, some of
   them are not idempotent operations, and shouldn't be called twice.

 - and finally call diffcore_flush().

> @@ -3745,6 +3742,12 @@ 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;

Shouldn't this be a BUG() instead?

The trivial rewrite to use this macro is a good idea, but it probably
should be a separate patch.

> +#define DIFF_QUEUE_CLEAR(q) \
> +	do { \
> +		(q)->queue = NULL; \
> +		(q)->nr = (q)->alloc = (q)->run = 0; \
> +	} while(0);

Re: [PATCH 1/2 v3] Make diffcore_std only can run once before a diff_flush.

[Bo Yang]

On Fri, Apr 23, 2010 at 4:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> It actually is stronger than that; we should never run it more than once,
> and it would be a bug if we did so.  Which codepath tries to call *_std()
> twice?

In command 'git log --follow ...'
log_tree_diff call diff_tree_sha1 and then diff_tree_diff_flush, when
'--follow' is given, the former function will call
try_to_follow_renames, which will call diffcore_std to detect rename.
And then, diff_tree_diff_flush call 'diffcore_std' again
unconditional. (and I have try to find a condition to make the call,
but I fail, so I figure out this patch.)

Breakpoint 1, diffcore_std (options=0xbf9cc044) at diff.c:3748
3748		if (diff_queued_diff.run)
(gdb) bt
#0  diffcore_std (options=0xbf9cc044) at diff.c:3748
#1  0x08124206 in try_to_follow_renames (t1=0xbf9cc130, t2=0xbf9cc11c,
base=0x81571c9 "", opt=0xbf9cc468) at tree-diff.c:358
#2  0x08124480 in diff_tree_sha1 (old=0x9c51d8c
"$\033\222T���\a\035\200T����\210;8\235i", new=0x9c51d2c
"\201�\017<�\v��n]\226{�+�\001\003\232\232\230",
    base=0x81571c9 "", opt=0xbf9cc468) at tree-diff.c:418
#3  0x080e660e in log_tree_diff (opt=0xbf9cc220, commit=0x9c51d28,
log=0xbf9cc1ac) at log-tree.c:536
#4  0x080e668f in log_tree_commit (opt=0xbf9cc220, commit=0x9c51d28)
at log-tree.c:560
#5  0x0807faa1 in cmd_log_walk (rev=0xbf9cc220) at builtin/log.c:237
#6  0x080806e2 in cmd_log (argc=5, argv=0xbf9cc788, prefix=0x0) at
builtin/log.c:481
#7  0x0804b8eb in run_builtin (p=0x8161524, argc=5, argv=0xbf9cc788)
at git.c:260
#8  0x0804ba51 in handle_internal_command (argc=5, argv=0xbf9cc788) at git.c:416
#9  0x0804bb2c in run_argv (argcp=0xbf9cc700, argv=0xbf9cc704) at git.c:458
#10 0x0804bcbe in main (argc=5, argv=0xbf9cc788) at git.c:529
(gdb) c
Continuing.

Breakpoint 1, diffcore_std (options=0xbf9cc468) at diff.c:3748
3748		if (diff_queued_diff.run)
(gdb) bt
#0  diffcore_std (options=0xbf9cc468) at diff.c:3748
#1  0x080e6356 in log_tree_diff_flush (opt=0xbf9cc220) at log-tree.c:449
#2  0x080e6619 in log_tree_diff (opt=0xbf9cc220, commit=0x9c51d28,
log=0xbf9cc1ac) at log-tree.c:537
#3  0x080e668f in log_tree_commit (opt=0xbf9cc220, commit=0x9c51d28)
at log-tree.c:560
#4  0x0807faa1 in cmd_log_walk (rev=0xbf9cc220) at builtin/log.c:237
#5  0x080806e2 in cmd_log (argc=5, argv=0xbf9cc788, prefix=0x0) at
builtin/log.c:481
#6  0x0804b8eb in run_builtin (p=0x8161524, argc=5, argv=0xbf9cc788)
at git.c:260
#7  0x0804ba51 in handle_internal_command (argc=5, argv=0xbf9cc788) at git.c:416
#8  0x0804bb2c in run_argv (argcp=0xbf9cc700, argv=0xbf9cc704) at git.c:458
#9  0x0804bcbe in main (argc=5, argv=0xbf9cc788) at git.c:529
(gdb)

> The standard calling sequence is:
>
>  - start from an empty queue.
>
>  - use diff_change() and diff_addremove() to populate the queue.
>
>  - call diffcore_std(). if you need to use a non-standard chain of
>   diffcore transformations, you _could_ call the diffcore_* routines that
>   diffcore_std() calls, if you choose to, but as you found out, some of
>   them are not idempotent operations, and shouldn't be called twice.
>
>  - and finally call diffcore_flush().
>
>> @@ -3745,6 +3742,12 @@ 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;
>
> Shouldn't this be a BUG() instead?

Anyone may call diff_tree_sha1 and then diffcore_std, and
diff_tree_sha1 may call another diffcore_std if '--follow' given. If
this is a BUG, the calling pattern, diff_tree_sha1 -> diffcore_std
should all disappear from our code. And this involved much code
refactor. And I suggest my way that we avoid the duplicate call
actively in diffcore_std.

> The trivial rewrite to use this macro is a good idea, but it probably
> should be a separate patch.
>
>> +#define DIFF_QUEUE_CLEAR(q) \
>> +     do { \
>> +             (q)->queue = NULL; \
>> +             (q)->nr = (q)->alloc = (q)->run = 0; \
>> +     } while(0);
>

You mean split this commit into two?

Regards!
Bo
-- 
My blog: http://blog.morebits.org

Re: [PATCH 1/2 v3] Make diffcore_std only can run once before a diff_flush.

[Bo Yang]

Hi Junio,

   I have not receive any comments on this thread from you, but I
think it worth some words. I want to make these series patches landed
and could you please give some more advice on this?

Regards!
Bo

On Fri, Apr 23, 2010 at 11:55 AM, Bo Yang <struggleyb.nku@gmail.com> wrote:
> On Fri, Apr 23, 2010 at 4:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> It actually is stronger than that; we should never run it more than once,
>> and it would be a bug if we did so.  Which codepath tries to call *_std()
>> twice?
>
> In command 'git log --follow ...'
> log_tree_diff call diff_tree_sha1 and then diff_tree_diff_flush, when
> '--follow' is given, the former function will call
> try_to_follow_renames, which will call diffcore_std to detect rename.
> And then, diff_tree_diff_flush call 'diffcore_std' again
> unconditional. (and I have try to find a condition to make the call,
> but I fail, so I figure out this patch.)
>
> Breakpoint 1, diffcore_std (options=0xbf9cc044) at diff.c:3748
> 3748            if (diff_queued_diff.run)
> (gdb) bt
> #0  diffcore_std (options=0xbf9cc044) at diff.c:3748
> #1  0x08124206 in try_to_follow_renames (t1=0xbf9cc130, t2=0xbf9cc11c,
> base=0x81571c9 "", opt=0xbf9cc468) at tree-diff.c:358
> #2  0x08124480 in diff_tree_sha1 (old=0x9c51d8c
> "$\033\222T���\a\035\200T����\210;8\235i", new=0x9c51d2c
> "\201�\017<�\v��n]\226{�+�\001\003\232\232\230",
>    base=0x81571c9 "", opt=0xbf9cc468) at tree-diff.c:418
> #3  0x080e660e in log_tree_diff (opt=0xbf9cc220, commit=0x9c51d28,
> log=0xbf9cc1ac) at log-tree.c:536
> #4  0x080e668f in log_tree_commit (opt=0xbf9cc220, commit=0x9c51d28)
> at log-tree.c:560
> #5  0x0807faa1 in cmd_log_walk (rev=0xbf9cc220) at builtin/log.c:237
> #6  0x080806e2 in cmd_log (argc=5, argv=0xbf9cc788, prefix=0x0) at
> builtin/log.c:481
> #7  0x0804b8eb in run_builtin (p=0x8161524, argc=5, argv=0xbf9cc788)
> at git.c:260
> #8  0x0804ba51 in handle_internal_command (argc=5, argv=0xbf9cc788) at git.c:416
> #9  0x0804bb2c in run_argv (argcp=0xbf9cc700, argv=0xbf9cc704) at git.c:458
> #10 0x0804bcbe in main (argc=5, argv=0xbf9cc788) at git.c:529
> (gdb) c
> Continuing.
>
> Breakpoint 1, diffcore_std (options=0xbf9cc468) at diff.c:3748
> 3748            if (diff_queued_diff.run)
> (gdb) bt
> #0  diffcore_std (options=0xbf9cc468) at diff.c:3748
> #1  0x080e6356 in log_tree_diff_flush (opt=0xbf9cc220) at log-tree.c:449
> #2  0x080e6619 in log_tree_diff (opt=0xbf9cc220, commit=0x9c51d28,
> log=0xbf9cc1ac) at log-tree.c:537
> #3  0x080e668f in log_tree_commit (opt=0xbf9cc220, commit=0x9c51d28)
> at log-tree.c:560
> #4  0x0807faa1 in cmd_log_walk (rev=0xbf9cc220) at builtin/log.c:237
> #5  0x080806e2 in cmd_log (argc=5, argv=0xbf9cc788, prefix=0x0) at
> builtin/log.c:481
> #6  0x0804b8eb in run_builtin (p=0x8161524, argc=5, argv=0xbf9cc788)
> at git.c:260
> #7  0x0804ba51 in handle_internal_command (argc=5, argv=0xbf9cc788) at git.c:416
> #8  0x0804bb2c in run_argv (argcp=0xbf9cc700, argv=0xbf9cc704) at git.c:458
> #9  0x0804bcbe in main (argc=5, argv=0xbf9cc788) at git.c:529
> (gdb)
>
>> The standard calling sequence is:
>>
>>  - start from an empty queue.
>>
>>  - use diff_change() and diff_addremove() to populate the queue.
>>
>>  - call diffcore_std(). if you need to use a non-standard chain of
>>   diffcore transformations, you _could_ call the diffcore_* routines that
>>   diffcore_std() calls, if you choose to, but as you found out, some of
>>   them are not idempotent operations, and shouldn't be called twice.
>>
>>  - and finally call diffcore_flush().
>>
>>> @@ -3745,6 +3742,12 @@ 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;
>>
>> Shouldn't this be a BUG() instead?
>
> Anyone may call diff_tree_sha1 and then diffcore_std, and
> diff_tree_sha1 may call another diffcore_std if '--follow' given. If
> this is a BUG, the calling pattern, diff_tree_sha1 -> diffcore_std
> should all disappear from our code. And this involved much code
> refactor. And I suggest my way that we avoid the duplicate call
> actively in diffcore_std.
>
>> The trivial rewrite to use this macro is a good idea, but it probably
>> should be a separate patch.
>>
>>> +#define DIFF_QUEUE_CLEAR(q) \
>>> +     do { \
>>> +             (q)->queue = NULL; \
>>> +             (q)->nr = (q)->alloc = (q)->run = 0; \
>>> +     } while(0);
>>
>
> You mean split this commit into two?
>
> Regards!
> Bo
> --
> My blog: http://blog.morebits.org
>