Re: [PATCH v2] log: add --nonlinear-barrier to help see non-linear history

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  v2 renames the option name to --nonlinear-barrier and fixes using it
>  with --dense. Best used with --no-merges to see patch series.

I think that the earlier name "show linear-break" is more easily
understood than the new name, but maybe that is just me.  It's not
like you are blocking something from going forward with a barrier,
and internally it is called a "break-bar".

>  Documentation/rev-list-options.txt |  7 ++++++
>  log-tree.c                         |  4 +++
>  revision.c                         | 51 +++++++++++++++++++++++++++++++++++---
>  revision.h                         |  6 +++++
>  4 files changed, 65 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 03533af..435aa2d 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -750,6 +750,13 @@ This enables parent rewriting, see 'History Simplification' below.
>  This implies the `--topo-order` option by default, but the
>  `--date-order` option may also be specified.
>  
> +--nonlinear-barrier[=<barrier>]::
> +	When --graph is not used, all history branches are flatten and

s/flatten and/flattened and it/, perhaps?

> +	could be hard to see that the two consecutive commits do not
> +	belong to a linear branch. This option puts a barrier in
> +	between them in that case. If `<barrier>` is specified, it
> +	is the string that will be shown instead of the default one.
> +
>  ifdef::git-rev-list[]
>  --count::
>  	Print a number stating how many commits would have been
> diff --git a/log-tree.c b/log-tree.c
> index 08970bf..17862f6 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -805,12 +805,16 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
>  	if (opt->line_level_traverse)
>  		return line_log_print(opt, commit);
>  
> +	if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
> +		printf("\n%s\n", opt->break_bar);
>  	shown = log_tree_diff(opt, commit, &log);
>  	if (!shown && opt->loginfo && opt->always_show_header) {
>  		log.parent = NULL;
>  		show_log(opt);
>  		shown = 1;
>  	}
> +	if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
> +		printf("\n%s\n", opt->break_bar);

Each time get_revision() returns a commit, it is inspected and
opt->linear is updated, this function is called and we show the
break in the single-strand-of-pearls if this is not a parent of the
commit we showed immediately before.  If we are showing the commit
in reverse, we have to go the other way around, showing the commit
and then the break.

OK.  It makes sense to me.

>  	opt->loginfo = NULL;
>  	maybe_flush_or_die(stdout, "stdout");
>  	return shown;

Does this new feature interact with -z format output in any way?
Should it, and if so how?

> diff --git a/revision.c b/revision.c
> index a68fde6..0a4849e 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1832,6 +1832,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		revs->notes_opt.use_default_notes = 1;
>  	} else if (!strcmp(arg, "--show-signature")) {
>  		revs->show_signature = 1;
> +	} else if (!strcmp(arg, "--nonlinear-barrier")) {
> +		revs->track_linear = 1;
> +		revs->break_bar = "                    ..........";
> +	} else if (starts_with(arg, "--nonlinear-barrier=")) {
> +		revs->track_linear = 1;
> +		revs->break_bar = xstrdup(arg + 20);
>  	} else if (starts_with(arg, "--show-notes=") ||
>  		   starts_with(arg, "--notes=")) {
>  		struct strbuf buf = STRBUF_INIT;
> @@ -2897,6 +2903,32 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
>  	return action;
>  }
>  
> +define_commit_slab(saved_linear, int);
> +
> +static void track_linear(struct rev_info *revs, struct commit *commit)
> +{
> +	struct commit_list *p = revs->previous_parents;
> +
> +	if (p) {
> +		int got_parent = 0;
> +		for (; p && !got_parent; p = p->next)
> +			got_parent = !hashcmp(p->item->object.sha1,
> +					      commit->object.sha1);
> +		revs->linear = got_parent;
> +		free_commit_list(revs->previous_parents);
> +	} else
> +		revs->linear = 1;
> +	if (revs->reverse) {
> +		if (!revs->saved_linear_slab) {
> +			revs->saved_linear_slab = xmalloc(sizeof(struct saved_linear));
> +			init_saved_linear(revs->saved_linear_slab);
> +		}
> +
> +		*saved_linear_at(revs->saved_linear_slab, commit) = revs->linear;
> +	}
> +	revs->previous_parents = copy_commit_list(commit->parents);

We are showing commit, and the parents (after history
simplification) of the commit we showed before this commit is kept
in previous-parents.  If we are one of them, we are showing
linearly, which makes sense.  While we are accumulating the output
in the forward direction in preparation for later emitting them in
reverse, we need to save away the linear-ness bit somewhere, and a
slab is a logical place to save that, which also makes sense.  But
why do you need a full int?  Doesn't an unsigned char wide enough?

I also wondered if the saved-parents slab we already have can be
easily reused for this, but it probably would not help.

I do not quite understand the "if we do not have previous parents"
bit, though.  Is it meant to trigger only at the very beginning?

Thanks.