Re: [PATCH v5] show-branch: convert per-branch flags to commit-slab
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 15, 2026, 17:17 UTC
- Message-ID
- <xmqqy0fcnpee.fsf@gitster.g>
- In-Reply-To
- <20260715120156.53025-1-gatlavishweshwarreddy26@gmail.com>
Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:
Show 29 quoted lines
> +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.
Show 22 quoted lines
> @@ -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?
Show 19 quoted lines
> 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.
Show 8 quoted lines
> @@ -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?
Show 5 quoted lines
> - 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.