Re: [PATCH v4 2/4] Add testcases for the --detect-dir-renames diffcore flag.

Yann Dirson wrote:

On Mon, Oct 04, 2010 at 03:32:41PM -0500, Jonathan Nieder wrote:

quoted

It might make sense to compute the tree, commit, etc one at a time
instead of this long one-liner.

If moved into a function which would make it readable, yes.

Something like

	commit_index () {
		test_tick &&
		tree=$(git write-tree) &&
		commit=$(
			printf "%s\n" "$*" |
			git commit-tree $tree
		) &&
		git update-ref HEAD $commit
	}

? Maybe the following (similar to what you use in later tests)
would be even better, for more verbose output when running with -v.

	commit_index () {
		test_tick &&
		git commit -m "$*"
	}

quoted

	printf "Line %s\n" 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 >a/path0 &&
	...

Well, when seeing for the first time such a construct, i tend to
wonder how portable it is amont printf implementations.

Yes, it's portable.

quoted

Nit: although compare_diff_patch ensures the result is not dependent
on the hash function, these hard-coded hashes are still hard for a
human to read.  Could they be computed instead?

Well, that would just make the test harder to read imho.  Using
regexps would help for readability

If you had said "writability" I would agree with you here.  And that's
an important concern, too.

What I was suggesting looks like this:

	path0_id2=$(git rev-parse :b/path0) &&
	path100_id=$(git rev-parse :b/path100) &&
	cat >expected <<-EOF &&
	:040000 040000 $zeroes $zeroes R100	a/	b/
	:100644 000000 $path3_id $zeroes D	a/path3
	:100644 100644 $path2_id $path2_id R100	a/path2	b/2path
	:100644 100644 $path0_id $path0_id2 R093	a/path0	b/path0
	:100644 100644 $path1_id $path1_id R100	a/path1	b/path1
	:000000 100644 $zeroes $path100_id A	b/path100
	EOF
	...

Maybe it would be better to do

	cat >expected <<-\EOF &&
	:040000 040000 X X R100	a/	b/
	:100644 000000 X X D	a/path3
	:100644 100644 X X R100	a/path2	b/2path
	:100644 100644 X X R093	a/path0	b/path0
	:100644 100644 X X R100	a/path1	b/path1
	:000000 100644 X X A	b/path100
	EOF

since the hashes are not being checked, anyway.