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

Hi,

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

On Tue, Nov 09 2021, Anders Kaseorg wrote:

quoted

+	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