Re: [PATCH v2] show-branch: convert object.flags to commit-slab with uint64_t

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

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

> show-branch uses commit->object.flags to store per-commit data:
> the UNINTERESTING bit and per-branch reachability bits. Using the
> shared object.flags field for this purpose is fragile as it
> conflicts with other users of the same field, and limits the
> number of branches that can be shown to MAX_REVS (27).

The command was written with the understanding that it would not
allow other parts of the system to touch these per-object flag
bits.  Therefore, fragility is not a relevant issue.  The primary
problem with this design is that the flags word has only a fixed
number of available bits, meaning it cannot process hundreds of
branches simultaneously.

This limitation is precisely where the concept of using a commit
slab shines.  However, to truly take advantage of a commit slab, the
slab stride must be variable.  If the tool is handling more than 80
branches, for example, each commit requires a `uint64_t[2]` array
allocation (since a single `uint64_t` provides only 64 bits, while
`uint64_t[2]` can store up to 128 bits).

> Convert this usage to a dedicated commit-slab using uint64_t as
> the element type. This is the canonical way to associate per-commit
> data in Git without polluting the shared object flags. Using
> uint64_t instead of unsigned int lifts the MAX_REVS limitation
> from 27 to 62 branches, as suggested in prior review discussions.

I do not understand the reference to 62.  As I previously noted,
storing a fixed uint64_t[1] instead of variable-length uint64_t[n]
in each slab entry fails to realize the full potential of using
commit slabs.  Furthermore, we should be able to utilize all 64 bits
of a uint64_t word.  There is no need to pollute this dedicated,
one-bit-per-branch slab with the UNINTERESTING bit, which is used
for the command's revision walking.  Revision walking can continue
using the UNINTERESTING bit in the standard object.flags instead.

> @@ -511,18 +523,20 @@ static int rev_is_head(const char *head, const char *name)
>
>  static int show_merge_base(const struct commit_list *seen, int num_rev)
>  {
> -	int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
> -	int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
> +	uint64_t all_mask = ((UINT64_C(1) << (REV_SHIFT + num_rev)) - 1);
> +	uint64_t all_revs = all_mask & ~((UINT64_C(1) << REV_SHIFT) - 1);
>  	int exit_status = 1;
>
>  	for (const struct commit_list *s = seen; s; s = s->next) {
>  		struct commit *commit = s->item;
> -		int flags = commit->object.flags & all_mask;
> +		uint64_t flags = get_rev_flags(commit) & all_mask;
>  		if (!(flags & UNINTERESTING) &&
>  		    ((flags & all_revs) == all_revs)) {
>  			puts(oid_to_hex(&commit->object.oid));
>  			exit_status = 0;
> -			commit->object.flags |= UNINTERESTING;
> +
> +or_rev_flags(commit, UNINTERESTING);
> +
>  		}
>  	}

What's this funny indentation?

> @@ -607,9 +621,9 @@ static int omit_in_dense(struct commit *commit, struct commit **rev, int n)
>  	for (i = 0; i < n; i++)
>  		if (rev[i] == commit)
>  			return 0;
> -	flag = commit->object.flags;
> +	flag = get_rev_flags(commit);

Has the definition of local variable "flag" in omit_in_dense() been
updated to u64?  If it is still "int", then this would not work
well on platforms whose "int" is still i32.

>  	for (i = count = 0; i < n; i++) {
> -		if (flag & (1u << (i + REV_SHIFT)))
> +		if (flag & (UINT64_C(1) << (i + REV_SHIFT)))
>  			count++;
>  	}
>  	if (count == 1)