Re: [PATCH 1/3 v4] Add a macro DIFF_QUEUE_CLEAR.

Previous thread: [PATCH] blame-options.txt: Add default value for `-M/-C` options. by Bo Yang on Thursday, May 6, 2010 - 9:51 pm. (1 message)

Next thread: hello by brenda saleeby on Friday, May 7, 2010 - 12:17 am. (1 message)
From: Bo Yang
Date: Thursday, May 6, 2010 - 9:52 pm

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 (3):
  Add a macro DIFF_QUEUE_CLEAR.
  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                          |    7 ++++
 t/t4205-log-follow-harder-copies.sh |   56 +++++++++++++++++++++++++++++++++++
 tree-diff.c                         |    2 +-
 8 files changed, 82 insertions(+), 18 deletions(-)
 create mode 100755 t/t4205-log-follow-harder-copies.sh

--

From: Bo Yang
Date: Thursday, May 6, 2010 - 9:52 pm

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     |    8 ++++++++
 diffcore.h |    2 ++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 4a350e3..f0985bc 100644
--- a/diff.c
+++ b/diff.c
@@ -3737,6 +3737,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)
@@ -3756,6 +3762,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.h b/diffcore.h
index 5d05dea..491bea0 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -91,11 +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 = 0; \
+		(q)->run = 0; \
 	} while(0);
 
 extern struct diff_queue_struct diff_queued_diff;
-- 
1.6.0.4

--

From: Bo Yang
Date: Thursday, May 6, 2010 - 9:52 pm

'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 ...
From: Bo Yang
Date: Thursday, May 6, 2010 - 9:52 pm

Refactor the diff_queue_struct code, this macro help
to reset the structure.

Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
---
 diff.c             |   13 +++++--------
 diffcore-break.c   |    6 ++----
 diffcore-pickaxe.c |    3 +--
 diffcore-rename.c  |    3 +--
 diffcore.h         |    5 +++++
 5 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/diff.c b/diff.c
index e40c127..4a350e3 100644
--- a/diff.c
+++ b/diff.c
@@ -2540,6 +2540,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;
 
@@ -3457,8 +3458,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;
 }
@@ -3586,8 +3586,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);
 
@@ -3609,8 +3608,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;
@@ -3678,8 +3676,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];
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 ...
From: Sven Verdoolaege
Date: Monday, August 2, 2010 - 5:47 am

What's this line for?  It doesn't seem to be explained by the commit
message and it breaks "git diff-files -p --submodule".
Without this line, I get the following output in one of my projects:

    Submodule barvinok contains untracked content
    Submodule barvinok contains modified content
    Submodule barvinok e129555..833e4a6:
      > iscc: use simplified CLooG interface
    Submodule cloog contains untracked content
    Submodule cloog f083938..4684a24:
      > partial doc
      > cloog_names_read_strings: do not generate names if they cannot be read
      > cloog_program_read: separate reading from input from construction of CloogProgram
    Submodule cloog-polylib contains untracked content
    Submodule cloog-polylib contains modified content
    Submodule isl contains untracked content
    Submodule isl 892fb27..5292e00:
      > isl_transitive_closure.c: anonymize input map during incremental computation
      > isl_transitive_closure.c: keep track of domains for Floyd-Warshall
      > isl_dim_drop: always remove tuple name, even if number of dims to drop is zero
      > isl_dim_set_tuple_name: allow explicit removal of tuple name
    Submodule isl-polylib contains untracked content
    Submodule isl-polylib 531cb00..e9e2edf:
      > stop using isl_basic_map internals
    Submodule polylib contains untracked content

With the line, I only get

    Submodule barvinok contains untracked content
    Submodule barvinok contains modified content
    Submodule barvinok e129555..833e4a6:
      > iscc: use simplified CLooG interface

skimo
--

From: Junio C Hamano
Date: Monday, August 2, 2010 - 8:26 am

I don't think this change is warranted.  The macro was supposed to reduce
the repetition of assignment to q->queue, q->nr and q->alloc, and nothing
else.

Also the commit messages in this series are unreadable---I should have
been a bit more careful.

Sorry and thanks for noticing.

--

From: Bo Yang
Date: Monday, August 2, 2010 - 8:40 am

Hi Sven,

Sorry about the broken code and the bad commit message...

This line is used to clear the global queue structure and make it
usable in next round of diff. I am wondering how the submodule part
use the diff API to cause such an issue. :)

-- 
Regards!
Bo
----------------------------
My blog: http://blog.morebits.org
Why Git: http://www.whygitisbetterthanx.com/
--

Previous thread: [PATCH] blame-options.txt: Add default value for `-M/-C` options. by Bo Yang on Thursday, May 6, 2010 - 9:51 pm. (1 message)

Next thread: hello by brenda saleeby on Friday, May 7, 2010 - 12:17 am. (1 message)