| Message ID | 5dd305d2f0de43a70b46336c8f1a62437e0511e1.1578408947.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix two bugs in graph.c | expand |
On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > The assert() macro is sometimes compiled out. Instead, switch these into > BUG() statements using our own custom macro. > > Reported-by: Eric Sunshine <sunshine@sunshineco.com> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> I can buy the argument that compiling with and without NDEBUG can lead to confusion. But if that is the case, wouldn't it be so for all of the assert() calls, not just ones in the graph code? Previous discussions[1] seemed to conclude that having a kernel-style BUG_ON() is probably the right way forward. I.e., replace this: > +#define graph_assert(exp) if (!(exp)) { BUG("assert failed: "#exp""); } with something similar in git-compat-util.h. Even if we don't convert everybody to it immediately, it would be available for use. At any rate, I think this patch (and the third one) can be post-v2.25. But we'd want the first one before the release. -Peff [1] https://lore.kernel.org/git/20171122223827.26773-1-sbeller@google.com/
On Tue, Jan 7, 2020 at 10:36 AM Jeff King <peff@peff.net> wrote: > On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote: > > The assert() macro is sometimes compiled out. Instead, switch these into > > BUG() statements using our own custom macro. > > I can buy the argument that compiling with and without NDEBUG can lead > to confusion. But if that is the case, wouldn't it be so for all of the > assert() calls, not just ones in the graph code? This wasn't just a matter of potential confusion. It's one thing to have assert()s in the code in general, but another thing when a scripted test specifically depends upon the asserted condition, as was the case with the test as originally proposed. Since the final patch series removes that particular assert() altogether, it's perhaps not that important anymore.
On 1/7/2020 10:51 AM, Eric Sunshine wrote: > On Tue, Jan 7, 2020 at 10:36 AM Jeff King <peff@peff.net> wrote: >> On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote: >>> The assert() macro is sometimes compiled out. Instead, switch these into >>> BUG() statements using our own custom macro. >> >> I can buy the argument that compiling with and without NDEBUG can lead >> to confusion. But if that is the case, wouldn't it be so for all of the >> assert() calls, not just ones in the graph code? > > This wasn't just a matter of potential confusion. It's one thing to > have assert()s in the code in general, but another thing when a > scripted test specifically depends upon the asserted condition, as was > the case with the test as originally proposed. Since the final patch > series removes that particular assert() altogether, it's perhaps not > that important anymore. I'm happy to drop this commit, too. I misunderstood your point. -Stolee
diff --git a/graph.c b/graph.c index aaf97069bd..ac78bdf96c 100644 --- a/graph.c +++ b/graph.c @@ -6,6 +6,8 @@ #include "revision.h" #include "argv-array.h" +#define graph_assert(exp) if (!(exp)) { BUG("assert failed: "#exp""); } + /* Internal API */ /* @@ -316,7 +318,7 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void struct git_graph *graph = data; static struct strbuf msgbuf = STRBUF_INIT; - assert(opt); + graph_assert(opt); strbuf_reset(&msgbuf); if (opt->line_prefix) @@ -865,13 +867,13 @@ static void graph_output_pre_commit_line(struct git_graph *graph, * * We need 2 extra rows for every parent over 2. */ - assert(graph->num_parents >= 3); + graph_assert(graph->num_parents >= 3); /* * graph->expansion_row tracks the current expansion row we are on. * It should be in the range [0, num_expansion_rows - 1] */ - assert(0 <= graph->expansion_row && + graph_assert(0 <= graph->expansion_row && graph->expansion_row < graph_num_expansion_rows(graph)); /* @@ -923,13 +925,13 @@ static void graph_output_commit_char(struct git_graph *graph, struct graph_line * (We should only see boundary commits when revs->boundary is set.) */ if (graph->commit->object.flags & BOUNDARY) { - assert(graph->revs->boundary); + graph_assert(graph->revs->boundary); graph_line_addch(line, 'o'); return; } /* - * get_revision_mark() handles all other cases without assert() + * get_revision_mark() handles all other cases without graph_assert() */ graph_line_addstr(line, get_revision_mark(graph->revs, graph->commit)); } @@ -1094,7 +1096,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l for (j = 0; j < graph->num_parents; j++) { par_column = graph_find_new_column_by_commit(graph, parents->item); - assert(par_column >= 0); + graph_assert(par_column >= 0); c = merge_chars[idx]; graph_line_write_column(line, &graph->new_columns[par_column], c); @@ -1172,14 +1174,14 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l * the graph much more legible, since whenever branches * cross, only one is moving directions. */ - assert(target * 2 <= i); + graph_assert(target * 2 <= i); if (target * 2 == i) { /* * This column is already in the * correct place */ - assert(graph->mapping[i] == -1); + graph_assert(graph->mapping[i] == -1); graph->mapping[i] = target; } else if (graph->mapping[i - 1] < 0) { /* @@ -1225,8 +1227,8 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l * The space just to the left of this * branch should always be empty. */ - assert(graph->mapping[i - 1] > target); - assert(graph->mapping[i - 2] < 0); + graph_assert(graph->mapping[i - 1] > target); + graph_assert(graph->mapping[i - 2] < 0); graph->mapping[i - 2] = target; /* * Mark this branch as the horizontal edge to