Re: [PATCH v6] log: fix coloring of certain octupus merge shapes

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

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> For octopus merges where the first parent edge immediately merges into
> the next column to the left:
>
> | *-.
> | |\ \
> |/ / /
>
> then the number of columns should be one less than the usual case:
>
> | *-.
> | |\ \
> | | | *

I had a bit hard time parsing the above, especially with "then",
which probably would make it easier to read if it is not there.

> Also refactor the code to iterate over columns rather than dashes,
> building from an initial patch suggestion by Jeff King.

s/suggestion/suggested/ perhaps?

>
> Signed-off-by: Noam Postavsky <npostavs@users.sourceforge.net>
> Reviewed-by: Jeff King <peff@peff.net>
> ---

Thanks, both.

>  /*
> + * Draw the horizontal dashes of an octopus merge and return the number of
> + * characters written.
>   */
>  static int graph_draw_octopus_merge(struct git_graph *graph,
>  				    struct strbuf *sb)
>  {
>  	/*
> +	 * Here dashless_parents represents the number of parents which don't
> +	 * need to have dashes (the edges labeled "0" and "1").  And
> +	 * dashful_parents are the remaining ones.

Here "dash" refers to that horizontal line on the same line as the
resulting merge.  A very clearly explained definition.  OK.

> +	 * | *---.
> +	 * | |\ \ \
> +	 * | | | | |
> +	 * x 0 1 2 3
> +	 *
> +	 */
> +	const int dashless_parents = 2;

That counts parent #0 (the first parent) and parent #1.

> +	int dashful_parents = graph->num_parents - dashless_parents;

When a mistaken caller calls this function on a commit that is not
an octopus, this can underflow.  dashful_parents would be -1 for a
non-merge, dashful_parents would be 0 for a normal merge, and then
dashful_parents would be 1 for a merge of three histories.  OK.

> +	/*
> +	 * Usually, each parent gets its own column, like the diagram above, but
> +	 * sometimes the first parent goes into an existing column, like this:
> +	 *
> +	 * | *---.
> +	 * | |\ \ \
> +	 * |/ / / /
> +	 * x 0 1 2
> +	 *
> +	 * In which case there will be more parents than the delta of columns.
> +	 */

It is unclear to me what "delta of columns" means here.  Is this
because I am unfamiliar with the internal of graph.[ch] API (and
'delta of columns' is used elsewhere in the API internals already)?

> +	int delta_cols = (graph->num_new_columns - graph->num_columns);

So in the second picture above, new-columns (which is the columns
used after showing the current line) is narrower (because 'x' reuses
an already allocated column without getting a new one) than columns
(which is the columns for the octopus merge we are showing)?

I am not sure I follow what is going on around here, sorry.

> +	int parent_in_old_cols = graph->num_parents - delta_cols;
> +	/*
> +	 * In both cases, commit_index corresponds to the edge labeled "0".
> +	 */
> +	int first_col = graph->commit_index + dashless_parents
> +	    - parent_in_old_cols;
> +
> +	int i;
> +	for (i = 0; i < dashful_parents; i++) {
> +		strbuf_write_column(sb, &graph->new_columns[i+first_col], '-');
> +		strbuf_write_column(sb, &graph->new_columns[i+first_col],
> +				    i == dashful_parents-1 ? '.' : '-');

Draw a dash-dash for each, except we show dash-dot only for the last
one.  OK.  It is interesting that dashful_parents does not have to
change between the two examples you gave above, and it is
understandable because it only depends on the shape of the graph
near the octopus merge itself (in other words, the placement of the
parent commits does not contribute to it at all).  Makes sense.

>  	}
> -	col_num = (i / 2) + dashless_commits + graph->commit_index;
> -	strbuf_write_column(sb, &graph->new_columns[col_num], '.');
> -	return num_dashes + 1;
> +	return 2 * dashful_parents;

This is natural, as we showed either dash-dash or dash-dot only for
dashful_parents after the merge itself. OK.

Thanks, will queue.