Re: [PATCH v6 2/3] revision: add peek functions for lookahead
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 20, 2026, 14:56 UTC
- Message-ID
- <xmqqzf0pfefp.fsf@gitster.g>
- In-Reply-To
- <20260620-ps-pre-commit-indent-v6-2-cdc6d8fd5fbc@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
Show 34 quoted lines
> 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.
Show 25 quoted lines
> +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?
Show 19 quoted lines
> 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