Re: [PATCH v5] show-branch: convert per-branch flags to commit-slab

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

Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:

> +static struct commit_rev_flags rev_flags_slab;
> +static int flags_stride; /* number of uint64_t words per commit */
> +
>  static struct commit_name *commit_to_name(struct commit *commit)
>  {
>  	return *commit_name_slab_at(&name_slab, commit);
>  }
>  
> +static uint64_t *get_rev_flags_ptr(struct commit *commit)
> +{
> +	return commit_rev_flags_at(&rev_flags_slab, commit);
> +}
> +
> +static uint64_t *peek_rev_flags_ptr(struct commit *commit)
> +{
> +	return commit_rev_flags_peek(&rev_flags_slab, commit);
> +}
> +
> +static int has_any_rev_flags(struct commit *commit)
> +{
> +	uint64_t *f = peek_rev_flags_ptr(commit);
> +	int i;
> +	if (!f)
> +		return 0;
> +	for (i = 0; i < flags_stride; i++)
> +		if (f[i])
> +			return 1;
> +	return 0;
> +}

We are no longer limited to 26 or 64, which is excellent.  Early
in "git show-branch --help", we prominently say "It cannot show
more than 26 branches and commits", which needs updating.

I wonder if we have enough test coverage for this command.  If we
were paranoid, we might have had a test that feeds 30 revs to make
sure the command fails, which would now fail with this change.
We should check if any existing tests need updating, and write a
few new ones to ensure proper coverage of the expanded limits.

> @@ -226,34 +285,34 @@ static void join_revs(struct prio_queue *queue,
>  		      struct commit_list **seen_p,
>  		      int num_rev, int extra)
>  {
> -	int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
> -	int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
> -
>  	while (queue->nr) {
>  		struct commit_list *parents;
>  		int still_interesting = !!interesting(queue);
>  		struct commit *commit = prio_queue_peek(queue);
>  		bool get_pending = true;
> -		int flags = commit->object.flags & all_mask;
>  
>  		if (!still_interesting && extra <= 0)
>  			break;
>  
>  		mark_seen(commit, seen_p);
> -		if ((flags & all_revs) == all_revs)
> -			flags |= UNINTERESTING;
> +		if (has_all_rev_flags(commit, num_rev))
> +			commit->object.flags |= UNINTERESTING;

I am afraid these two do quite different things.

In the original, a local 'flags' variable is made UNINTERESTING,
which is then used in the 'while' loop below to inspect and
propagate the UNINTERESTING (and other) bits to the parents,
without smudging the current commit itself.

In the updated code, you smudge the commit in question itself with
the UNINTERESTING bit.  Won't that prevent this commit, which is a
merge-base, from being shown?

>  		parents = commit->parents;
>  
>  		while (parents) {
>  			struct commit *p = parents->item;
> -			int this_flag = p->object.flags;
>  			parents = parents->next;
> -			if ((this_flag & flags) == flags)
> +			if (has_all_rev_flags(p, num_rev))
>  				continue;
>  			repo_parse_commit(the_repository, p);
>  			if (mark_seen(p, seen_p) && !still_interesting)
>  				extra--;
> -			p->object.flags |= flags;
> +			{
> +				int _b;
> +				for (_b = 0; _b < num_rev; _b++)
> +					if (test_rev_flag_bit(commit, _b))
> +						or_rev_flag_bit(p, _b);
> +			}

This part also behaves quite differently.  The original checks if
the parent already has all the bits in 'flags' (including the
UNINTERESTING bit) and avoids traversing further if so.  If the
parent is missing any of those bits, however, they are
propagated down to it.

In the updated code, you do not paint these parents
UNINTERESTING at all.

> @@ -263,7 +322,6 @@ static void join_revs(struct prio_queue *queue,
>  		if (get_pending)
>  			prio_queue_get(queue);
>  	}
> -
>  	/*
>  	 * Postprocess to complete well-poisoning.
>  	 *

What is this change about?

> -		warning(Q_("ignoring %s; cannot handle more than %d ref",
> -			   "ignoring %s; cannot handle more than %d refs",
> +		warning(Q_("ignoring %s; cannot handle more than %zu ref",
> +			   "ignoring %s; cannot handle more than %zu refs",
>  			   MAX_REVS), refname, MAX_REVS);

Indeed.  Since you are no longer limited to 27 or 64 bits, it is
certainly nice to see that the code is prepared to bust the %d
limit.  ;-)

However, our CodingGuidelines document says we cannot portably use
"%zu" yet.  Can't we use an unsigned long or something more
established here?  We surely do not expect to ever fill the full
range expressible by size_t.

Thanks.