Re: Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"

[Duy Nguyen <pclouds@gmail.com>]

On Thu, Sep 27, 2018 at 9:20 AM Elijah Newren <newren@gmail.com> wrote:
> 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.

Phew.. so technically it's not my fault, I just helped expose the bug
with my BUG() line, probably.

> 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.

You clear s->change just before the second call, but perhaps you
should do it right after the first. It seems other code authors were
already aware of this sharing "s" and worked around changing
s->use_color at the first call site, it seems neater to keep all this
temporary fixes in just one place:

    saved_color_setting = s->use_color;
    s->use_color = 0;
    commitable = run_status(s->fp, index_file, prefix, 1, s);
    s->use_color = saved_color_setting;

> 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.

I agree. Keep the bug fix to the point. Cleanup and stuff could be
done later (and perhaps try to run all the heavy "diff" just once
instead of twice like this).
-- 
Duy