Re: [GSoC PATCH v5 1/2] graph: add --graph-lane-limit option

[Junio C Hamano <gitster@pobox.com>]

Pablo Sabater <pabloosabaterr@gmail.com> writes:

> +static int graph_needs_truncation(struct git_graph *graph, int lane)
> +{
> +	int max = graph->revs->graph_max_lanes;
> +	/*
> +	 * Ignore values <= 0, meaning no limit.
> +	 */
> +	return max > 0 && lane >= max;
> +}

Make a mental note that this helper function works on number of
lanes, not display columns (which is roughly twice the number of
lanes).

> @@ -696,6 +705,18 @@ static void graph_update_columns(struct git_graph *graph)
>  		}
>  	}
>  
> +	/*
> +	 * If graph_max_lanes is set, cap the padding from the branches
> +	 */
> +	if (graph->revs->graph_max_lanes > 0) {
> +		/*
> +		 * width of "| " per lanes plus truncation mark "~ ".
> +		 */
> +		int max_columns_width = graph->revs->graph_max_lanes * 2 + 2;
> +		if (graph->width > max_columns_width)
> +			graph->width = max_columns_width;
> +	}
> +
>  	/*
>  	 * Shrink mapping_size to be the minimum necessary
>  	 */
> @@ -846,6 +867,10 @@ static void graph_output_padding_line(struct git_graph *graph,
>  	 * Output a padding row, that leaves all branch lines unchanged
>  	 */
>  	for (i = 0; i < graph->num_new_columns; i++) {
> +		if (graph_needs_truncation(graph, i)) {
> +			graph_line_addstr(line, "~ ");
> +			break;
> +		}

And that mental note helps to convince us this loop makes sense, as
it increments 'i' one by one ;-)

> @@ -903,6 +928,9 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
>  			seen_this = 1;
>  			graph_line_write_column(line, col, '|');
>  			graph_line_addchars(line, ' ', graph->expansion_row);
> +		} else if (seen_this && graph_needs_truncation(graph, i)) {
> +			graph_line_addstr(line, "~ ");
> +			break;
>  		} else if (seen_this && (graph->expansion_row == 0)) {
>  			/*
>  			 * This is the first line of the pre-commit output.
> @@ -994,6 +1022,12 @@ static void graph_draw_octopus_merge(struct git_graph *graph, struct graph_line
>  		col = &graph->new_columns[j];
>  
>  		graph_line_write_column(line, col, '-');

And here, 'j' comes from graph->mapping[] array.  Does that count in
display columns or lanes?

> +		if (graph_needs_truncation(graph, j / 2 + i)) {

This makes it look as if 'j' counts in columns and needs to be
divided by 2 to make it comparable to lanes.

> +			graph_line_addstr(line, "~ ");
> +			break;
> +		}
> +
>  		graph_line_write_column(line, col, (i == dashed_parents - 1) ? '.' : '-');
>  	}
>  

> +	if (graph->num_parents > 1) {
> +		if (!graph_needs_truncation(graph, graph->commit_index)) {
> +			graph_update_state(graph, GRAPH_POST_MERGE);
> +		} else {
> +			struct commit_list *first_parent = first_interesting_parent(graph);
> +			int first_parent_col = graph_find_new_column_by_commit(graph, first_parent->item);

Are we sure that first_interesting_parent() will always give us a
non-NULL pointer?

Can we use a bit shorter identifier names to deal with these overly
long lines?  The lifetime of these two variables is very short so they
do not have to be so descriptive.

			struct commit *p = first_interesting_parent(graph)->item;
			int lane = graph_find_new_column_by_commit(graph, p);

> +			if (!graph_needs_truncation(graph, first_parent_col))
> +				graph_update_state(graph, GRAPH_POST_MERGE);
> +			else if (graph_is_mapping_correct(graph))
> +				graph_update_state(graph, GRAPH_PADDING);
> +			else
> +				graph_update_state(graph, GRAPH_COLLAPSING);
> +		}
> +	} else if (graph_is_mapping_correct(graph))