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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 3, 2018, 19:01 UTC
Message-ID: <20180403120133.a1327fae0475a6ad1467e364@google.com>
In-Reply-To: <20180403165143.80661-6-dstolee@microsoft.com>

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

Show 13 quoted lines

> -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.