Web lists-archives.com

Re: [PATCH 5/6] commit.c: use generation to halt paint walk




On Tue,  3 Apr 2018 12:51:42 -0400
Derrick Stolee <dstolee@xxxxxxxxxxxxx> wrote:

> -static int queue_has_nonstale(struct prio_queue *queue)
> +static int queue_has_nonstale(struct prio_queue *queue, uint32_t min_gen)
>  {
> -	int i;
> -	for (i = 0; i < queue->nr; i++) {
> -		struct commit *commit = queue->array[i].data;
> -		if (!(commit->object.flags & STALE))
> -			return 1;
> +	if (min_gen != GENERATION_NUMBER_UNDEF) {
> +		if (queue->nr > 0) {
> +			struct commit *commit = queue->array[0].data;
> +			return commit->generation >= min_gen;
> +		}

This only works if the prio_queue has
compare_commits_by_gen_then_commit_date. Also, I don't think that the
min_gen != GENERATION_NUMBER_UNDEF check is necessary. So I would write
this as:

  if (queue->compare == compare_commits_by_gen_then_commit_date &&
      queue->nr) {
    struct commit *commit = queue->array[0].data;
    return commit->generation >= min_gen;
  }
  for (i = 0 ...

If you'd rather not perform the comparison to
compare_commits_by_gen_then_commit_date every time you invoke
queue_has_nonstale(), that's fine with me too, but document somewhere
that queue_has_nonstale() only works if this comparison function is
used.

> +		if (commit->generation > last_gen)
> +			BUG("bad generation skip");
> +
> +		last_gen = commit->generation;

last_gen seems to only be used to ensure that the priority queue returns
elements in the correct order - I think we can generally trust the
queue, and if we need to test it, we can do it elsewhere.