[PATCH] xdiff: use BUG(...), not xdl_bug(...)

Ævar Arnfjörð Bjarmason· Jun 7, 2021, 16:43 UTC · lore

The xdl_bug() function was introduced in e8adf23d1e (xdl_change_compact(): introduce the concept of a change group, 2016-08-22), let's use our usual BUG() function instead.

We'll now have meaningful line numbers if we encounter bugs in xdiff, and less code duplication.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 xdiff/xdiffi.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

Show changes to xdiff/xdiffi.c +8 −14

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 380eb728ed..a4542c05b6 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -796,12 +796,6 @@ static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g, long flags)
 	}
 }
 
-static void xdl_bug(const char *msg)
-{
-	fprintf(stderr, "BUG: %s\n", msg);
-	exit(1);
-}
-
 /*
  * Move back and forward change groups for a consistent and pretty diff output.
  * This also helps in finding joinable change groups and reducing the diff
@@ -841,7 +835,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 			/* Shift the group backward as much as possible: */
 			while (!group_slide_up(xdf, &g, flags))
 				if (group_previous(xdfo, &go))
-					xdl_bug("group sync broken sliding up");
+					BUG("group sync broken sliding up");
 
 			/*
 			 * This is this highest that this group can be shifted.
@@ -857,7 +851,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 				if (group_slide_down(xdf, &g, flags))
 					break;
 				if (group_next(xdfo, &go))
-					xdl_bug("group sync broken sliding down");
+					BUG("group sync broken sliding down");
 
 				if (go.end > go.start)
 					end_matching_other = g.end;
@@ -882,9 +876,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 			 */
 			while (go.end == go.start) {
 				if (group_slide_up(xdf, &g, flags))
-					xdl_bug("match disappeared");
+					BUG("match disappeared");
 				if (group_previous(xdfo, &go))
-					xdl_bug("group sync broken sliding to match");
+					BUG("group sync broken sliding to match");
 			}
 		} else if (flags & XDF_INDENT_HEURISTIC) {
 			/*
@@ -925,9 +919,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 
 			while (g.end > best_shift) {
 				if (group_slide_up(xdf, &g, flags))
-					xdl_bug("best shift unreached");
+					BUG("best shift unreached");
 				if (group_previous(xdfo, &go))
-					xdl_bug("group sync broken sliding to blank line");
+					BUG("group sync broken sliding to blank line");
 			}
 		}
 
@@ -936,11 +930,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 		if (group_next(xdf, &g))
 			break;
 		if (group_next(xdfo, &go))
-			xdl_bug("group sync broken moving to next group");
+			BUG("group sync broken moving to next group");
 	}
 
 	if (!group_next(xdfo, &go))
-		xdl_bug("group sync broken at end of file");
+		BUG("group sync broken at end of file");
 
 	return 0;
 }

-- 
2.32.0.rc3.434.gd8aed1f08a7

Jeff King· Jun 7, 2021, 19:44 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] xdiff: use BUG(...), not xdl_bug(...)

On Mon, Jun 07, 2021 at 06:43:49PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 6 quoted lines

> The xdl_bug() function was introduced in
> e8adf23d1e (xdl_change_compact(): introduce the concept of a change
> group, 2016-08-22), let's use our usual BUG() function instead.
> 
> We'll now have meaningful line numbers if we encounter bugs in xdiff,
> and less code duplication.

Thanks, I think this is a good direction.

Back when that commit was originally done, we didn't include git-compat-util.h in xdiff at all, so we couldn't rely on Git-specific code (though I think BUG() did not even exist back then anyway!).

These days I think we're comfortable with the notion that there is no active upstream for xdiff, so we'd not be likely to send our changes anywhere (and thus dependencies on our helpers are not a problem). But there is one complication: libgit2 uses xdiff, too, and has pulled in our changes including xdl_bug().

I wondered if we could make it easier on them by keeping the change in one spot. But doing:

  static void xdl_bug(const char *msg)
  {
	BUG("%s", msg);
  }

is not really helpful, as it loses the line numbers. I guess we could do:

  #ifdef BUG
  #define xdl_bug(msg) BUG(msg)
  #else
  ...the current implementation
  #endif

But really, libgit2 could just as easily define their own BUG() as they see fit. Whether it is called xdl_bug() or BUG() in the call-sites is not really that important.

So I think this patch is fine in its current form.

-Peff

patchxdiff: use BUG(...), not xdl_bug(...)