Re: [PATCH v2 2/2] diffcore-rename: filter rename_src list when possible

From: Elijah Newren <newren@gmail.com>
Date: Feb 13, 2021, 04:43 UTC
Message-ID: <CABPp-BHj2k0dDwYB-F=G_aXTemSp5Hu-DK=mrX2pZf3WTH9fQg@mail.gmail.com>
In-Reply-To: <xmqq8s7svkr9.fsf@gitster.c.googlers.com>

On Fri, Feb 12, 2021 at 5:06 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > @@ -578,8 +624,7 @@ void diffcore_rename(struct diff_options *options)
> >                       struct diff_filespec *one = rename_src[j].p->one;
> >                       struct diff_score this_src;
> >
> > -                     if (one->rename_used && !want_copies)
> > -                             continue;
> > +                     assert(!one->rename_used || want_copies || break_idx);
>
> Doesn't assert becomes a no-op in production builds?  Shouldn't this
> be a BUG() instead?

I guess it depends on the reason for the line. If we're just trying to help others understand the code by documenting conditions that are true (and perhaps help them when they are refactoring), then comments like

    /* The following is true at this point: !one->rename_used ||
want_copies || break_idx */

could also be used, but it's kind of verbose.  The form

    assert(!one->rename_used || want_copies || break_idx);

is shorter, clearer, and is likely to be up-to-date because even if most builds and users turn off assertions, some folks will build and run with assertions on. If it's a refactoring-aid, then the latter form is also more likely to help the future developer (who may be future me).

If, however, the purpose is to check for bad input or worries about programming logic possibly have edge cases, and we're afraid that such conditions might cause severe and hard to debug problems later in the code, then we absolutely would rather use a BUG().

Most of my uses of assert() fall in the former category. I use BUG() when it's a line meant for the latter category. There are a few that perhaps fall in between, where I just have to make a judgement call. I'd like to say that I'm more likely to use BUG() for those, but the lack of a BUG_ON() does tend to make it pretty annoying to use and certainly discourages it.

Granted, that's the way I look at it. I'm curious if others have a different view. It certainly seems like the project likes to use both forms nearly equally: $ git grep assert\( origin/master | wc -l 468 $ git grep BUG\( origin/master | wc -l 506

so I'm curious if there are other factors that make folks pick one or the other.