Re: [GSoC PATCH WIP RFC v3 1/3] graph: add --graph-lane-limit option

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

Pablo Sabater <pabloosabaterr@gmail.com> writes:

> Repositories that have many active branches produce very wide outputs
> with 'git log --graph --all' making it difficult to read.

If you have active branches, whether they are merged to some very
small number of integration branches or they are left updating
without getting merged for a long time, you'd end up getting very
wide output from "log --all --graph"?  Or is this a problem only
when these active branches are merged and having to show merge
commits create the need for wider output?  I cannot quite see which
from the above description.

>
> Add MINIMUM_GRAPH_COLUMNS = 1

This unfinished sentence looks a bit out of place.  It is unclear
how that variable about "columns" related to "lane" that is
introduced in the next paratraph.

> Add '--graph-lane-limit=<n>' to the revision options, this option
> needs --graph explicitly and rejects values under MINIMUM_GRAPH_COLUMNS.

"and rejects values under ..." -> "has to be at least 1".  And the
previous paragraph that consists of a single unfinished sentence can
be discarded.

The implementation detail that you happened to choose a C
proprocessor macro instead of hardcoded constatnt to write the lower
limit is not something readers of the log message needs to know.
They can see that in the "log -p" output easily.  

What is more helpful for readers to know is what you mean by
"graph-lane", what you are counting, and why you want its lower
limit to 1 (instead of 0 or 2).  These reasoning behind the design
is much more important to record to help future developers who want
to fix bugs in this code or who want to extend the feature this code
adds, without violating the underlying assumption and design goals
of the original author (i.e., you).

> Add graph_max_lanes to rev_info and store what the user set
>
> Add graph_needs_truncation() and teach it to know when a column is
> over the limit following graph_max_lanes, if the limit is 0, treat it like
> no limit.
>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  graph.c    |  6 ++++++
>  graph.h    |  2 ++
>  revision.c | 11 +++++++++++
>  revision.h |  1 +
>  4 files changed, 20 insertions(+)
>
> diff --git a/graph.c b/graph.c
> index 26f6fbf000..a95c0a9a73 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -317,6 +317,12 @@ struct git_graph {
>  	struct strbuf prefix_buf;
>  };
>  
> +static int graph_needs_truncation(struct git_graph *graph, int col)
> +{
> +	int max = graph->revs->graph_max_lanes;
> +	return max > 0 && col >= max;
> +}
> +
>  static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)
>  {
>  	struct git_graph *graph = data;
> diff --git a/graph.h b/graph.h
> index 3fd1dcb2e9..9a4551dd29 100644
> --- a/graph.h
> +++ b/graph.h
> @@ -262,4 +262,6 @@ void graph_show_commit_msg(struct git_graph *graph,
>  			   FILE *file,
>  			   struct strbuf const *sb);
>  
> +#define MINIMUM_GRAPH_COLUMNS 1
> +
>  #endif /* GRAPH_H */
> diff --git a/revision.c b/revision.c
> index 31808e3df0..aeddf2d166 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2605,6 +2605,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	} else if (!strcmp(arg, "--no-graph")) {
>  		graph_clear(revs->graph);
>  		revs->graph = NULL;
> +	} else if ((argcount = parse_long_opt("graph-lane-limit", argv, &optarg))) {
> +		int max_lanes = parse_count(optarg);
> +		if (max_lanes < MINIMUM_GRAPH_COLUMNS)
> +			die(_("minimum lanes is %d, cannot be set to %d"),
> +			    MINIMUM_GRAPH_COLUMNS, max_lanes);
> +		revs->graph_max_lanes = max_lanes;
> +		return argcount;
>  	} else if (!strcmp(arg, "--encode-email-headers")) {
>  		revs->encode_email_headers = 1;
>  	} else if (!strcmp(arg, "--no-encode-email-headers")) {
> @@ -3172,6 +3179,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  
>  	if (revs->no_walk && revs->graph)
>  		die(_("options '%s' and '%s' cannot be used together"), "--no-walk", "--graph");
> +
> +	if (revs->graph_max_lanes > 0 && !revs->graph)
> +		die(_("option '%s' requires '%s'"), "--graph-lane-limit", "--graph");
> +
>  	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
>  		die(_("the option '%s' requires '%s'"), "--grep-reflog", "--walk-reflogs");
>  
> diff --git a/revision.h b/revision.h
> index 69242ecb18..597116f885 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -304,6 +304,7 @@ struct rev_info {
>  
>  	/* Display history graph */
>  	struct git_graph *graph;
> +	unsigned int graph_max_lanes;

This is "unsigned int"; don't we want the other places (like the
on-stack local variable handle_revision_opt() uses to parse the
value from the command line) and the parameter used in
graph_needs_truncation() helper function all consistently use the
same type?

>  	/* special limits */
>  	int skip_count;