Re: Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"
- From
Elijah Newren <newren@gmail.com>
- Date
- Sep 27, 2018, 07:20 UTC
- Message-ID
- <20180927072043.19130-1-newren@gmail.com>
On Wed, Sep 26, 2018 at 9:20 PM Elijah Newren <newren@gmail.com> wrote:
Show 42 quoted lines
> On Wed, Sep 26, 2018, 2:27 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > On Wed, Sep 26 2018, Andrea Stacchiotti wrote:
> >
> > > I'm very sorry, I indeed forgot the `diff.renames=copies`.
> > >
> > > The following script can reproduce the bug even with a blank config:
>
> Thanks for the bug report and the simple testcase.
>
> > > ---------------------
> > >
> > > # Make a test repo
> > > git init testrepo
> > > cd testrepo
> > > git config user.name A
> > > git config user.email B
> > > git config diff.renames copies
> > >
> > > # Add a file called orig
> > > echo 'a' > orig
> > > git add orig
> > > git commit -m'orig'
> > >
> > > # Copy orig in new and modify orig
> > > cp orig new
> > > echo 'b' > orig
> > >
> > > # add -N and then commit trigger the bug
> > > git add -N new
> > > git commit
> > >
> > > # Cleanup
> > > cd ..
> > > rm -rf testrepo
> >
> > Thanks. Bisecting shows that the bug is in dc6b1d92ca ("wt-status: use
> > settings from git_diff_ui_config", 2018-05-04) first released with
> > 2.18.0.
>
> The bisect is slightly misleading; the bug was introduced in 2.17.0
> for renames, and when copy detection became a thing in 2.18.0 it also
> incidentally would trigger with copies. I'll post a patch soon.-- 8< -- Subject: [PATCH] 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> --- builtin/commit.c | 1 + t/t7500-commit.sh | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+)
diff --git a/builtin/commit.c b/builtin/commit.c index b57d8e4b82..ee98d703f2 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -921,6 +921,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, if (!commitable && whence != FROM_MERGE && !allow_empty && !(amend && is_a_merge(current_head))) { s->display_comment_prefix = old_display_comment_prefix; + string_list_clear(&s->change, 1); run_status(stdout, index_file, prefix, 0, s); if (amend) fputs(_(empty_amend_advice), stderr); diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh index 170b4810e0..387637b9b0 100755 --- a/t/t7500-commit.sh +++ b/t/t7500-commit.sh @@ -359,4 +359,27 @@ test_expect_success 'new line found before status message in commit template' ' test_i18ncmp expected-template editor-input ' +test_expect_success 'setup empty commit with unstaged rename and copy' ' + test_create_repo unstaged_rename_and_copy && + ( + cd unstaged_rename_and_copy && + + echo content >orig && + git add orig && + test_commit orig && + + cp -a orig new_copy && + mv orig new_rename && + git add -N new_copy new_rename + ) +' + +test_expect_success 'check commit with unstaged rename and copy' ' + ( + cd unstaged_rename_and_copy && + + test_must_fail git -c diff.renames=copy commit + ) +' + test_done
-- 2.19.0.222.g923d35e14f