Re: [PATCH v6 2/3] revision: add peek functions for lookahead

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

Pablo Sabater <pabloosabaterr@gmail.com> writes:

> The graph code in a subsequent commit needs to be able to look ahead in
> order to set indentation-related flags.
>
> Using revs->commits is brittle and the data structure that holds the
> pending commits might change in the future.
>
> Add two functions that abstract this for the graph.
>
> Helped-by: Kristofer Karlsson <stoansen@gmail.com>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  revision.c | 38 ++++++++++++++++++++++++++++++++++++++
>  revision.h | 10 ++++++++++
>  2 files changed, 48 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index e91d7e1f11..a472a28853 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3708,6 +3708,44 @@ static unsigned int count_explore_walked;
>  static unsigned int count_indegree_walked;
>  static unsigned int count_topo_walked;
>  
> +struct commit *revision_peek_next_commit (struct rev_info *revs)
> +{
> +	struct topo_walk_info *info = revs->topo_walk_info;
> +
> +	if (info)
> +		return prio_queue_peek(&info->topo_queue);
> +	if (revs->commits)
> +		return revs->commits->item;
> +
> +	return NULL;
> +}

OK.  "If we are doing topo_walk, topo_queue is the priority queue to
peek into, otherwise revs->commits list is being used" is a bit too
intimate implementation detail I am not comfortable to depend on,
but as long as it is contained inside revision.c it should be OK.

Lose the space between the function name and its (parameter list)
from this and the next function.

> +int revision_has_commits_after (struct rev_info *revs, int n)
> +{
> +	struct topo_walk_info *info = revs->topo_walk_info;
> +
> +	if (info) {
> +		int visible = 0;
> +		for (size_t i = 0; i < info->topo_queue.nr && visible < n; i++) {
> +			struct commit *c = info->topo_queue.array[i].data;
> +			if (get_commit_action(revs, c) == commit_show)
> +				visible++;
> +		}
> +		return visible > n-1;
> +	}
> +	if (revs->commits) {
> +		struct commit_list *cl;
> +		int visible = 0;
> +		for (cl = revs->commits; cl && visible < n; cl = cl->next) {
> +			if (get_commit_action(revs, cl->item) == commit_show)
> +				visible++;
> +		}
> +		return visible > n-1;
> +	}
> +
> +	return 0;
> +}

Regarding the use of get_commit_action() here, I wondered if this is
safe, because usually get_commit_action() is called only once per
commit during history traversal from simplify_commit(), but this
patch adds calls to it for all of the remaining commits being
processed without consuming them (so get_commit_action() will be
called on these commits again later as the history traversal
progresses).

If get_commit_action() a pure function without any side effects,
this may be safe, but line-log has something with side effect in the
function.

I _think_ this is OK, as "--graph" sets .rewrite_parents bit (as
well as .topo_order bit) on, which makes want_ancestry() to return
true.  Which in turn means even if -L is in effect, we will not call
line_log_process_ranges_arbitrary_commit() that is the only source
of side effect in this function.  Somebody needs to sanity check
this, but we may want to leave an in-code comment to warn future
developers not to call get_commit_action() on random commits outside
of the normal history traversal under what condition (namely, -L
without rewrite_parents).

Even better, perhaps add

	if (revs->line_level_traverse && !want_ancestry(revs))
		BUG("do not call this");

at the beginning of revision_has_commits_after() function, and
describe why in the header file comment for this function below?

> diff --git a/revision.h b/revision.h
> index 00c392be37..a10c6b0940 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -572,4 +572,14 @@ int rewrite_parents(struct rev_info *revs,
>   */
>  struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit);
>  
> +/*
> + * Peek into revision's next commit without consuming it.
> + */
> +struct commit *revision_peek_next_commit(struct rev_info *revs);
> +
> +/*
> + * Check if there are n more commits to be shown yet.
> + */
> +int revision_has_commits_after(struct rev_info *revs, int n);
> +
>  #endif