Re: [PATCH v5 2/4] receive-pack: Clean dead code from update_worktree()
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Nov 10, 2021, 12:11 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2111101303380.21127@tvgsbejvaqbjf.bet>
- In-Reply-To
- <211110.86y25wirtj.gmgdl@evledraar.gmail.com>
Hi,
On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
Show 17 quoted lines
>
> 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