threads / patch / 25026

patchfast-export: ensure that a renamed file is printed after all references

Subject: [PATCH] fast-export: ensure that a renamed file is printed after all references

## tl;dr

4 messages between Sep 7, 2010 and Sep 8, 2010. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Johannes Sixt· Sep 7, 2010, 19:33 UTC · lore

t9350 sets up a commit where a file is both copied and renamed. The output of fast-export for this commit should look like this:

  author ...
  committer ...
  from :19
  C "file2" "file4"
  R "file2" "file5"

The order of the two modification lines is derived from the result that the diff machinery produces.

060df62 (fast-export: Fix output order of D/F changes) inserted a qsort call that modifies the order of the diff result. Unfortunately, qsort need not be stable. Therefore, it is possible that the 'R' line appears before the 'C' line and the resulting fast-import stream is incorrect.

Fix it by forcing that the rename entry is printed after all other modification lines with the same file name.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 builtin/fast-export.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

Show changes to builtin/fast-export.c +9 −1

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 965e90e..007bba6 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -166,7 +166,15 @@ static int depth_first(const void *a_, const void *b_)
 	cmp = memcmp(name_a, name_b, len);
 	if (cmp)
 		return cmp;
-	return (len_b - len_a);
+	cmp = len_b - len_a;
+	if (cmp)
+		return cmp;
+	/*
+	 * Move 'R'ename entries last so that all references of the file
+	 * appear in the output before it is renamed (e.g., when a file
+	 * was copied and renamed in the same commit).
+	 */
+	return (a->status == 'R') - (b->status == 'R');
 }
 
 static void show_filemodify(struct diff_queue_struct *q,

-- 
1.7.1.402.gf1eeb

Elijah Newren· Sep 8, 2010, 05:46 UTC · re: Johannes Sixt · lore

Re: [PATCH] fast-export: ensure that a renamed file is printed after all references

Hi,

On Tue, Sep 7, 2010 at 1:33 PM, Johannes Sixt <j6t@kdbg.org> wrote:

Show 19 quoted lines

> t9350 sets up a commit where a file is both copied and renamed. The output
> of fast-export for this commit should look like this:
>
>  author ...
>  committer ...
>  from :19
>  C "file2" "file4"
>  R "file2" "file5"
>
> The order of the two modification lines is derived from the result that
> the diff machinery produces.
>
> 060df62 (fast-export: Fix output order of D/F changes) inserted a qsort
> call that modifies the order of the diff result. Unfortunately, qsort need
> not be stable. Therefore, it is possible that the 'R' line appears before
> the 'C' line and the resulting fast-import stream is incorrect.
>
> Fix it by forcing that the rename entry is printed after all other
> modification lines with the same file name.

Patch and description looks good to me...however, I was a little surprised to see no testsuite addition or additional explanation. Are you seeing some t9350 tests fail on some platform despite passing on linux?

Elijah

Johannes Sixt· Sep 8, 2010, 18:47 UTC · re: Elijah Newren · lore

Re: [PATCH] fast-export: ensure that a renamed file is printed after all references

On Mittwoch, 8. September 2010, Elijah Newren wrote:

> Patch and description looks good to me...however, I was a little
> surprised to see no testsuite addition or additional explanation.  Are
> you seeing some t9350 tests fail on some platform despite passing on
> linux?

Yes, I see the problem in t9350 on Windows. It is impossible to write a test for it because the test would depend on the unspecified behavior of qsort.

BTW, I don't know whether the diff machinery guarantees to produce 'C' and 'R' records in this order. If there is no such guarantee, then fast-export would have been broken even before 060df62.

-- Hannes

Elijah Newren· Sep 8, 2010, 19:26 UTC · re: Johannes Sixt · lore

Re: [PATCH] fast-export: ensure that a renamed file is printed after all references

On Wed, Sep 8, 2010 at 12:47 PM, Johannes Sixt <j6t@kdbg.org> wrote:

Show 8 quoted lines

> On Mittwoch, 8. September 2010, Elijah Newren wrote:
>> Patch and description looks good to me...however, I was a little
>> surprised to see no testsuite addition or additional explanation.  Are
>> you seeing some t9350 tests fail on some platform despite passing on
>> linux?
>
> Yes, I see the problem in t9350 on Windows. It is impossible to write a test
> for it because the test would depend on the unspecified behavior of qsort.

Makes sense. Would it be worth briefly mentioning Windows in the commit message?

Either way,
Acked-by: Elijah Newren <newren@gmail.com>

> BTW, I don't know whether the diff machinery guarantees to produce 'C' and 'R'
> records in this order. If there is no such guarantee, then fast-export would
> have been broken even before 060df62.