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

[Elijah Newren <newren@gmail.com>]

On Fri, Feb 12, 2021 at 5:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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.