From 2ecbd0a0db0f9d59a5df02a6daeb87e611171fa4 Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Fri, 23 May 2008 19:24:10 -0700 Subject: [PATCH 1/5] graph API: fix graph mis-alignment after uninteresting commits The graphing code had a bug that caused it to output branch lines incorrectly after ignoring an uninteresting commit. When computing how to match up the branch lines from the current commit to the next one, it forgot to take into account that it needed to initially start with 2 empty spaces where the missing commit would have gone. So, instead of drawing this, | * | <- Commit with uninteresting parent | / * | It used to incorrectly draw this: | * | <- Commit with uninteresting parent * | Signed-off-by: Adam Simpkins Signed-off-by: Junio C Hamano --- graph.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/graph.c b/graph.c index 9d6ed30b0..400f0147b 100644 --- a/graph.c +++ b/graph.c @@ -190,7 +190,10 @@ static void graph_insert_into_new_columns(struct git_graph *graph, * Ignore uinteresting and pruned commits */ if (commit->object.flags & (UNINTERESTING | TREESAME)) + { + *mapping_index += 2; return; + } /* * If the commit is already in the new_columns list, we don't need to From 37a75abc985a25a0612c2c176ed35d438722752d Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Fri, 23 May 2008 19:24:11 -0700 Subject: [PATCH 2/5] graph API: don't print branch lines for uninteresting merge parents Previously, the graphing code printed lines coming out of a merge commit for all of its parents, even if some of them were uninteresting. Now it only prints lines for interesting commits. For example, for a merge commit where only the first parent is interesting, the code now prints: * merge commit * interesting child instead of: M merge commit |\ * interesting child Signed-off-by: Adam Simpkins Signed-off-by: Junio C Hamano --- graph.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 13 deletions(-) diff --git a/graph.c b/graph.c index 400f0147b..add7e4477 100644 --- a/graph.c +++ b/graph.c @@ -55,9 +55,11 @@ struct git_graph { */ struct commit *commit; /* - * The number of parents this commit has. - * (Stored so we don't have to walk over them each time we need - * this number) + * The number of interesting parents that this commit has. + * + * Note that this is not the same as the actual number of parents. + * This count excludes parents that won't be printed in the graph + * output, as determined by graph_is_interesting(). */ int num_parents; /* @@ -180,6 +182,18 @@ static void graph_ensure_capacity(struct git_graph *graph, int num_columns) sizeof(int) * 2 * graph->column_capacity); } +/* + * Returns 1 if the commit will be printed in the graph output, + * and 0 otherwise. + */ +static int graph_is_interesting(struct commit *commit) +{ + /* + * Uninteresting and pruned commits won't be printed + */ + return (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1; +} + static void graph_insert_into_new_columns(struct git_graph *graph, struct commit *commit, int *mapping_index) @@ -187,13 +201,10 @@ static void graph_insert_into_new_columns(struct git_graph *graph, int i; /* - * Ignore uinteresting and pruned commits + * Ignore uinteresting commits */ - if (commit->object.flags & (UNINTERESTING | TREESAME)) - { - *mapping_index += 2; + if (!graph_is_interesting(commit)) return; - } /* * If the commit is already in the new_columns list, we don't need to @@ -231,8 +242,8 @@ static void graph_update_width(struct git_graph *graph, int max_cols = graph->num_columns + graph->num_parents; /* - * Even if the current commit has no parents, it still takes up a - * column for itself. + * Even if the current commit has no parents to be printed, it + * still takes up a column for itself. */ if (graph->num_parents < 1) max_cols++; @@ -316,6 +327,7 @@ static void graph_update_columns(struct git_graph *graph) } if (col_commit == graph->commit) { + int old_mapping_idx = mapping_idx; seen_this = 1; for (parent = graph->commit->parents; parent; @@ -324,6 +336,14 @@ static void graph_update_columns(struct git_graph *graph) parent->item, &mapping_idx); } + /* + * We always need to increment mapping_idx by at + * least 2, even if it has no interesting parents. + * The current commit always takes up at least 2 + * spaces. + */ + if (mapping_idx == old_mapping_idx) + mapping_idx += 2; } else { graph_insert_into_new_columns(graph, col_commit, &mapping_idx); @@ -353,11 +373,13 @@ void graph_update(struct git_graph *graph, struct commit *commit) graph->commit = commit; /* - * Count how many parents this commit has + * Count how many interesting parents this commit has */ graph->num_parents = 0; - for (parent = commit->parents; parent; parent = parent->next) - graph->num_parents++; + for (parent = commit->parents; parent; parent = parent->next) { + if (graph_is_interesting(parent->item)) + graph->num_parents++; + } /* * Call graph_update_columns() to update @@ -543,6 +565,15 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb) if (col_commit == graph->commit) { seen_this = 1; + /* + * If the commit has more than 1 interesting + * parent, print 'M' to indicate that it is a + * merge. Otherwise, print '*'. + * + * Note that even if this is actually a merge + * commit, we still print '*' if less than 2 of its + * parents are interesting. + */ if (graph->num_parents > 1) strbuf_addch(sb, 'M'); else From 7528f27dd677bed65d758667a621034b853595b4 Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Sun, 25 May 2008 00:07:21 -0700 Subject: [PATCH 3/5] log --graph --left-right: show left/right information in place of '*' With the --graph option, the graph already outputs 'o' instead of '*' for boundary commits. Make it emit '<' or '>' when --left-right is specified. (This change also disables the '^' prefix for UNINTERESTING commits. The graph code currently doesn't print anything special for these commits, since it assumes no UNINTERESTING, non-BOUNDARY commits are displayed. This is potentially a bug if UNINTERESTING non-BOUNDARY commits can actually be displayed via some code path.) [jc: squashed the left-right change from Dscho and Adam's fixup into one] Signed-off-by: Adam Simpkins Signed-off-by: Junio C Hamano --- Documentation/technical/api-history-graph.txt | 2 +- builtin-rev-list.c | 21 ++++++---- graph.c | 22 +++++----- graph.h | 2 +- log-tree.c | 41 +++++++++++-------- revision.c | 2 +- 6 files changed, 49 insertions(+), 41 deletions(-) diff --git a/Documentation/technical/api-history-graph.txt b/Documentation/technical/api-history-graph.txt index ce1c08ee8..e9559790a 100644 --- a/Documentation/technical/api-history-graph.txt +++ b/Documentation/technical/api-history-graph.txt @@ -115,7 +115,7 @@ Sample usage ------------ struct commit *commit; -struct git_graph *graph = graph_init(); +struct git_graph *graph = graph_init(opts); while ((commit = get_revision(opts)) != NULL) { graph_update(graph, commit); diff --git a/builtin-rev-list.c b/builtin-rev-list.c index 54d55cc3a..b47452740 100644 --- a/builtin-rev-list.c +++ b/builtin-rev-list.c @@ -65,15 +65,18 @@ static void show_commit(struct commit *commit) printf("%lu ", commit->date); if (header_prefix) fputs(header_prefix, stdout); - if (commit->object.flags & BOUNDARY) - putchar('-'); - else if (commit->object.flags & UNINTERESTING) - putchar('^'); - else if (revs.left_right) { - if (commit->object.flags & SYMMETRIC_LEFT) - putchar('<'); - else - putchar('>'); + + if (!revs.graph) { + if (commit->object.flags & BOUNDARY) + putchar('-'); + else if (commit->object.flags & UNINTERESTING) + putchar('^'); + else if (revs.left_right) { + if (commit->object.flags & SYMMETRIC_LEFT) + putchar('<'); + else + putchar('>'); + } } if (revs.abbrev_commit && revs.abbrev) fputs(find_unique_abbrev(commit->object.sha1, revs.abbrev), diff --git a/graph.c b/graph.c index add7e4477..dc2c1ec5d 100644 --- a/graph.c +++ b/graph.c @@ -54,6 +54,8 @@ struct git_graph { * The commit currently being processed */ struct commit *commit; + /* The rev-info used for the current traversal */ + struct rev_info *revs; /* * The number of interesting parents that this commit has. * @@ -127,10 +129,11 @@ struct git_graph { int *new_mapping; }; -struct git_graph *graph_init(void) +struct git_graph *graph_init(struct rev_info *opt) { struct git_graph *graph = xmalloc(sizeof(struct git_graph)); graph->commit = NULL; + graph->revs = opt; graph->num_parents = 0; graph->expansion_row = 0; graph->state = GRAPH_PADDING; @@ -565,16 +568,13 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb) if (col_commit == graph->commit) { seen_this = 1; - /* - * If the commit has more than 1 interesting - * parent, print 'M' to indicate that it is a - * merge. Otherwise, print '*'. - * - * Note that even if this is actually a merge - * commit, we still print '*' if less than 2 of its - * parents are interesting. - */ - if (graph->num_parents > 1) + + if (graph->revs && graph->revs->left_right) { + if (graph->commit->object.flags & SYMMETRIC_LEFT) + strbuf_addch(sb, '<'); + else + strbuf_addch(sb, '>'); + } else if (graph->num_parents > 1) strbuf_addch(sb, 'M'); else strbuf_addch(sb, '*'); diff --git a/graph.h b/graph.h index a7748a5b2..eab4e3dab 100644 --- a/graph.h +++ b/graph.h @@ -8,7 +8,7 @@ struct git_graph; * Create a new struct git_graph. * The graph should be freed with graph_release() when no longer needed. */ -struct git_graph *graph_init(); +struct git_graph *graph_init(struct rev_info *opt); /* * Destroy a struct git_graph and free associated memory. diff --git a/log-tree.c b/log-tree.c index 1474d1f5d..5505606ed 100644 --- a/log-tree.c +++ b/log-tree.c @@ -228,15 +228,17 @@ void show_log(struct rev_info *opt) if (!opt->verbose_header) { graph_show_commit(opt->graph); - if (commit->object.flags & BOUNDARY) - putchar('-'); - else if (commit->object.flags & UNINTERESTING) - putchar('^'); - else if (opt->left_right) { - if (commit->object.flags & SYMMETRIC_LEFT) - putchar('<'); - else - putchar('>'); + if (!opt->graph) { + if (commit->object.flags & BOUNDARY) + putchar('-'); + else if (commit->object.flags & UNINTERESTING) + putchar('^'); + else if (opt->left_right) { + if (commit->object.flags & SYMMETRIC_LEFT) + putchar('<'); + else + putchar('>'); + } } fputs(diff_unique_abbrev(commit->object.sha1, abbrev_commit), stdout); if (opt->print_parents) @@ -293,15 +295,18 @@ void show_log(struct rev_info *opt) fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), stdout); if (opt->commit_format != CMIT_FMT_ONELINE) fputs("commit ", stdout); - if (commit->object.flags & BOUNDARY) - putchar('-'); - else if (commit->object.flags & UNINTERESTING) - putchar('^'); - else if (opt->left_right) { - if (commit->object.flags & SYMMETRIC_LEFT) - putchar('<'); - else - putchar('>'); + + if (!opt->graph) { + if (commit->object.flags & BOUNDARY) + putchar('-'); + else if (commit->object.flags & UNINTERESTING) + putchar('^'); + else if (opt->left_right) { + if (commit->object.flags & SYMMETRIC_LEFT) + putchar('<'); + else + putchar('>'); + } } fputs(diff_unique_abbrev(commit->object.sha1, abbrev_commit), stdout); diff --git a/revision.c b/revision.c index 7142cf96c..1341f3d12 100644 --- a/revision.c +++ b/revision.c @@ -1205,7 +1205,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch if (!prefixcmp(arg, "--graph")) { revs->topo_order = 1; revs->rewrite_parents = 1; - revs->graph = graph_init(); + revs->graph = graph_init(revs); continue; } if (!strcmp(arg, "--root")) { From 3c68d67b572bce7ff41de463e75ee093e9dd71b7 Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Sat, 24 May 2008 16:02:04 -0700 Subject: [PATCH 4/5] Fix output of "git log --graph --boundary" Previously the graphing API wasn't aware of the revs->boundary flag, and it always assumed that commits marked UNINTERESTING would not be displayed. As a result, the boundary commits were printed at the end of the log output, but they didn't have any branch lines connecting them to their children in the graph. There was also another bug in the get_revision() code that caused graph_update() to be called twice on the first boundary commit. This caused the graph API to think that a commit had been skipped, and print a "..." line in the output. Signed-off-by: Adam Simpkins Signed-off-by: Junio C Hamano --- graph.c | 79 +++++++++++++++++++++++++++++++++++++++++++++--------- revision.c | 2 +- 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/graph.c b/graph.c index dc2c1ec5d..9b3495c46 100644 --- a/graph.c +++ b/graph.c @@ -189,8 +189,25 @@ static void graph_ensure_capacity(struct git_graph *graph, int num_columns) * Returns 1 if the commit will be printed in the graph output, * and 0 otherwise. */ -static int graph_is_interesting(struct commit *commit) +static int graph_is_interesting(struct git_graph *graph, struct commit *commit) { + /* + * If revs->boundary is set, commits whose children have + * been shown are always interesting, even if they have the + * UNINTERESTING or TREESAME flags set. + * + * However, ignore the commit if SHOWN is set. If SHOWN is set, + * the commit is interesting, but it has already been printed. + * This can happen because get_revision() doesn't return the + * boundary commits in topological order, even when + * revs->topo_order is set. + */ + if (graph->revs && graph->revs->boundary) { + if ((commit->object.flags & (SHOWN | CHILD_SHOWN)) == + CHILD_SHOWN) + return 1; + } + /* * Uninteresting and pruned commits won't be printed */ @@ -206,7 +223,7 @@ static void graph_insert_into_new_columns(struct git_graph *graph, /* * Ignore uinteresting commits */ - if (!graph_is_interesting(commit)) + if (!graph_is_interesting(graph, commit)) return; /* @@ -380,7 +397,7 @@ void graph_update(struct git_graph *graph, struct commit *commit) */ graph->num_parents = 0; for (parent = commit->parents; parent; parent = parent->next) { - if (graph_is_interesting(parent->item)) + if (graph_is_interesting(graph, parent->item)) graph->num_parents++; } @@ -543,6 +560,51 @@ static void graph_output_pre_commit_line(struct git_graph *graph, graph->state = GRAPH_COMMIT; } +static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) +{ + /* + * For boundary commits, print 'o' + * (We should only see boundary commits when revs->boundary is set.) + */ + if (graph->commit->object.flags & BOUNDARY) { + assert(graph->revs->boundary); + strbuf_addch(sb, 'o'); + return; + } + + /* + * If revs->left_right is set, print '<' for commits that + * come from the left side, and '>' for commits from the right + * side. + */ + if (graph->revs && graph->revs->left_right) { + if (graph->commit->object.flags & SYMMETRIC_LEFT) + strbuf_addch(sb, '<'); + else + strbuf_addch(sb, '>'); + return; + } + + /* + * Print 'M' for merge commits + * + * Note that we don't check graph->num_parents to determine if the + * commit is a merge, since that only tracks the number of + * "interesting" parents. We want to print 'M' for merge commits + * even if they have less than 2 interesting parents. + */ + if (graph->commit->parents != NULL && + graph->commit->parents->next != NULL) { + strbuf_addch(sb, 'M'); + return; + } + + /* + * Print '*' in all other cases + */ + strbuf_addch(sb, '*'); +} + void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb) { int seen_this = 0; @@ -568,16 +630,7 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb) if (col_commit == graph->commit) { seen_this = 1; - - if (graph->revs && graph->revs->left_right) { - if (graph->commit->object.flags & SYMMETRIC_LEFT) - strbuf_addch(sb, '<'); - else - strbuf_addch(sb, '>'); - } else if (graph->num_parents > 1) - strbuf_addch(sb, 'M'); - else - strbuf_addch(sb, '*'); + graph_output_commit_char(graph, sb); if (graph->num_parents < 2) strbuf_addch(sb, ' '); diff --git a/revision.c b/revision.c index 1341f3d12..181fb0b95 100644 --- a/revision.c +++ b/revision.c @@ -1697,7 +1697,7 @@ static struct commit *get_revision_internal(struct rev_info *revs) * switch to boundary commits output mode. */ revs->boundary = 2; - return get_revision(revs); + return get_revision_internal(revs); } /* From 4603ec0f960e582a7da7241563d3f235ad7f0d3e Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Sat, 24 May 2008 16:02:05 -0700 Subject: [PATCH 5/5] get_revision(): honor the topo_order flag for boundary commits Now get_revision() sorts the boundary commits when topo_order is set. Since sort_in_topological_order() takes a struct commit_list, it first places the boundary commits into revs->commits. Signed-off-by: Adam Simpkins Signed-off-by: Junio C Hamano --- graph.c | 9 +------ revision.c | 73 ++++++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 58 insertions(+), 24 deletions(-) diff --git a/graph.c b/graph.c index 9b3495c46..26b8c5209 100644 --- a/graph.c +++ b/graph.c @@ -195,16 +195,9 @@ static int graph_is_interesting(struct git_graph *graph, struct commit *commit) * If revs->boundary is set, commits whose children have * been shown are always interesting, even if they have the * UNINTERESTING or TREESAME flags set. - * - * However, ignore the commit if SHOWN is set. If SHOWN is set, - * the commit is interesting, but it has already been printed. - * This can happen because get_revision() doesn't return the - * boundary commits in topological order, even when - * revs->topo_order is set. */ if (graph->revs && graph->revs->boundary) { - if ((commit->object.flags & (SHOWN | CHILD_SHOWN)) == - CHILD_SHOWN) + if (commit->object.flags & CHILD_SHOWN) return 1; } diff --git a/revision.c b/revision.c index 181fb0b95..fb9924e5a 100644 --- a/revision.c +++ b/revision.c @@ -1612,28 +1612,62 @@ static void gc_boundary(struct object_array *array) } } +static void create_boundary_commit_list(struct rev_info *revs) +{ + unsigned i; + struct commit *c; + struct object_array *array = &revs->boundary_commits; + struct object_array_entry *objects = array->objects; + + /* + * If revs->commits is non-NULL at this point, an error occurred in + * get_revision_1(). Ignore the error and continue printing the + * boundary commits anyway. (This is what the code has always + * done.) + */ + if (revs->commits) { + free_commit_list(revs->commits); + revs->commits = NULL; + } + + /* + * Put all of the actual boundary commits from revs->boundary_commits + * into revs->commits + */ + for (i = 0; i < array->nr; i++) { + c = (struct commit *)(objects[i].item); + if (!c) + continue; + if (!(c->object.flags & CHILD_SHOWN)) + continue; + if (c->object.flags & (SHOWN | BOUNDARY)) + continue; + c->object.flags |= BOUNDARY; + commit_list_insert(c, &revs->commits); + } + + /* + * If revs->topo_order is set, sort the boundary commits + * in topological order + */ + sort_in_topological_order(&revs->commits, revs->lifo); +} + static struct commit *get_revision_internal(struct rev_info *revs) { struct commit *c = NULL; struct commit_list *l; if (revs->boundary == 2) { - unsigned i; - struct object_array *array = &revs->boundary_commits; - struct object_array_entry *objects = array->objects; - for (i = 0; i < array->nr; i++) { - c = (struct commit *)(objects[i].item); - if (!c) - continue; - if (!(c->object.flags & CHILD_SHOWN)) - continue; - if (!(c->object.flags & SHOWN)) - break; - } - if (array->nr <= i) - return NULL; - - c->object.flags |= SHOWN | BOUNDARY; + /* + * All of the normal commits have already been returned, + * and we are now returning boundary commits. + * create_boundary_commit_list() has populated + * revs->commits with the remaining commits to return. + */ + c = pop_commit(&revs->commits); + if (c) + c->object.flags |= SHOWN; return c; } @@ -1697,6 +1731,13 @@ static struct commit *get_revision_internal(struct rev_info *revs) * switch to boundary commits output mode. */ revs->boundary = 2; + + /* + * Update revs->commits to contain the list of + * boundary commits. + */ + create_boundary_commit_list(revs); + return get_revision_internal(revs); }