Re: [PATCH v5 2/4] receive-pack: Clean dead code from update_worktree()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

Hi,

On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:

>
> On Tue, Nov 09 2021, Anders Kaseorg wrote:
>
> > +	if (!worktree || !worktree->path)
> > +		BUG("worktree->path must be non-NULL");
>
> Perhaps a metter of taste, but I think BUG() should really be used for
> things that need a custom message over and beyond what assert() gives
> us.
>
> In this case using BUG() gives you a worse message, if you do:
>
>     assert(worktree && worktree->path)
>
> You'll get a sensible message from any modern compiler quotign the
> variable etc, all of which says the same thing as that BUG() message,
> just with less verbosity.

Maybe code reviews should stay away from  contentious matters of taste.

This claim that `assert()` would somehow be preferable to `BUG()` is not
backed up by our very own coding guidelines. See for yourself:
https://github.com/git/git/blob/v2.33.1/Documentation/CodingGuidelines
does not mention it.

The question of `assert()` vs `BUG()` has been brought up on this mailing
list before, without a clear preference for `assert()`, in contrast to
what the comment quoted above would want to make believe.

And the fact that BUG() allows for a well-crafted message without having
to rely on the compiler to guess as to what would make for a helpful
message, that alone speaks volumes.

Ciao,
Johannes