On 09/11, Thomas Gummerer wrote:
> I think you're on the right track here. I can not test this on Mac
> OS, but on Linux, the following fails when running the test under
> valgrind:
>
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 2237c7f4af..a8b0ef8c1d 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -142,4 +142,9 @@ test_expect_success 'changed message' '
> test_cmp expected actual
> '
>
> +test_expect_success 'amend and check' '
> + git commit --amend -m "new message" &&
> + git range-diff master HEAD@{1} HEAD
> +'
> +
> test_done
>
> valgrind gives me the following:
>
> ==18232== Invalid read of size 4
> ==18232== at 0x34D7B5: compute_assignment (linear-assignment.c:54)
> ==18232== by 0x2A4253: get_correspondences (range-diff.c:245)
> ==18232== by 0x2A4BFB: show_range_diff (range-diff.c:427)
> ==18232== by 0x19D453: cmd_range_diff (range-diff.c:108)
> ==18232== by 0x122698: run_builtin (git.c:418)
> ==18232== by 0x1229D8: handle_builtin (git.c:637)
> ==18232== by 0x122BCC: run_argv (git.c:689)
> ==18232== by 0x122D90: cmd_main (git.c:766)
> ==18232== by 0x1D55A3: main (common-main.c:45)
> ==18232== Address 0x4f4d844 is 0 bytes after a block of size 4 alloc'd
> ==18232== at 0x483777F: malloc (vg_replace_malloc.c:299)
> ==18232== by 0x3381B0: do_xmalloc (wrapper.c:60)
> ==18232== by 0x338283: xmalloc (wrapper.c:87)
> ==18232== by 0x2A3F8C: get_correspondences (range-diff.c:207)
> ==18232== by 0x2A4BFB: show_range_diff (range-diff.c:427)
> ==18232== by 0x19D453: cmd_range_diff (range-diff.c:108)
> ==18232== by 0x122698: run_builtin (git.c:418)
> ==18232== by 0x1229D8: handle_builtin (git.c:637)
> ==18232== by 0x122BCC: run_argv (git.c:689)
> ==18232== by 0x122D90: cmd_main (git.c:766)
> ==18232== by 0x1D55A3: main (common-main.c:45)
> ==18232==
>
> I'm looking into why that fails. Also adding Dscho to Cc here as the
> author of this code.
The diff below seems to fix it. Not submitting this as a proper
patch, as I don't quite understand what the original code tried to do
here. However this does pass all tests we currently have and fixes
the out of bounds memory read that's caught by valgrind (and that I
imagine could cause the segfault on Mac OS).
This matches how the initial minimum for the reduction transfer is
calculated in [1].
I'll try to convince myself of the right solution, but should someone
more familiar with the linear-assignment algorithm have an idea, feel
free to take this over :)
[1]: https://github.com/src-d/lapjv/blob/master/lap.h#L276
--- >8 ---
diff --git a/linear-assignment.c b/linear-assignment.c
index 9b3e56e283..ab0aa5fd41 100644
--- a/linear-assignment.c
+++ b/linear-assignment.c
@@ -51,7 +51,7 @@ void compute_assignment(int column_count, int row_count, int *cost,
else if (j1 < -1)
row2column[i] = -2 - j1;
else {
- int min = COST(!j1, i) - v[!j1];
+ int min = INT_MAX;
for (j = 1; j < column_count; j++)
if (j != j1 && min > COST(j, i) - v[j])
min = COST(j, i) - v[j];
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 2237c7f4af..a8b0ef8c1d 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -142,4 +142,9 @@ test_expect_success 'changed message' '
test_cmp expected actual
'
+test_expect_success 'amend and check' '
+ git commit --amend -m "new message" &&
+ git range-diff master HEAD@{1} HEAD
+'
+
test_done
--- >8 ---
