@matheusbsbs / matheus-bagliano / commits / 3e73cc62c0

commit: fix erroneous BUG, 'multiple renames on the same target? how?'

builtin/commit.c:prepare_to_commit() can call run_status() twice if using the editor, including status, and the user attempts to record a non-merge empty commit without explicit --allow-empty. If there is also a rename involved as well (due to using 'git add -N'), then a BUG in wt-status.c is triggered: BUG: wt-status.c:476: multiple renames on the same target? how? The reason we hit this bug is that both run_status() calls use the same struct wt_status * (named s), and s->change is not freed between runs. Changes are inserted into s with string_list_insert, which usually means that the second run just recomputes all the same results and overwrites what was computed the first time. However, ever since commit 176ea7479309 ("wt-status.c: handle worktree renames", 2017-12-27), wt-status started checking for renames and copies but also added a preventative check that d->rename_status wasn't already set and output a BUG message if it was. The problem isn't that there are multiple rename targets to a single path as the error implies, the problem is that 's' is not freed/cleared between the two run_status() calls. Ever since commit dc6b1d92ca9c ("wt-status: use settings from git_diff_ui_config", 2018-05-04), which stopped hardcoding DIFF_DETECT_RENAME and allowed users to ask for copy detection, this bug has also been triggerable with a copy instead of a rename. Fix the bug by clearing s->change. A better change might be to clean up all of s between the two run_status() calls. A good first step towards such a goal might be writing a function to free the necessary fields in the wt_status * struct; a cursory glance at the code suggests all of its allocated data is probably leaked. However, doing all that cleanup is a bigger task for someone else interested to tackle; just fix the bug for now. Reported-by: Andrea Stacchiotti <andreastacchiotti@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Sep 27, 2018 at 10:36 UTC 3e73cc62c0d80fedd1fc612e256b5eb3f3068de4

2 files changed +24

builtin/commit.c

@@ -872,6 +872,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,

872 s->use_color = 0;

873 commitable = run_status(s->fp, index_file, prefix, 1, s);

874 s->use_color = saved_color_setting;

875 + string_list_clear(&s->change, 1);

876 } else {

877 struct object_id oid;

878 const char *parent = "HEAD";

t/t7500-commit.sh

+23

@@ -359,4 +359,27 @@ test_expect_success 'new line found before status message in commit template' '

359 test_i18ncmp expected-template editor-input

360 '

361

362 +test_expect_success 'setup empty commit with unstaged rename and copy' '

363 + test_create_repo unstaged_rename_and_copy &&

364 + (

365 + cd unstaged_rename_and_copy &&

366 +

367 + echo content >orig &&

368 + git add orig &&

369 + test_commit orig &&

370 +

371 + cp orig new_copy &&

372 + mv orig new_rename &&

373 + git add -N new_copy new_rename

374 + )

375 +'

376 +

377 +test_expect_success 'check commit with unstaged rename and copy' '

378 + (

379 + cd unstaged_rename_and_copy &&

380 +

381 + test_must_fail git -c diff.renames=copy commit

382 + )

383 +'

384 +

385 test_done