From: Gatla Vishweshwar Reddy Date: Tue, 14 Jul 2026 20:01:22 GMT Subject: [PATCH v2] show-branch: convert object.flags to commit-slab with uint64_t Message-ID: <20260714200237.70509-1-gatlavishweshwarreddy26@gmail.com> In-Reply-To: <20260714183028.67857-1-gatlavishweshwarreddy26@gmail.com> 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). 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. Add helper functions get_rev_flags() and or_rev_flags() to encapsulate slab access cleanly. Update all bit operations to use UINT64_C(1) instead of 1u to ensure correct 64-bit shifts. Initialize and clear the slab in cmd_show_branch() to avoid memory leaks. Signed-off-by: Gatla Vishweshwar Reddy --- Changes in v2: - Use uint64_t instead of unsigned int for the slab element type. This lifts MAX_REVS from 27 to 62 branches since uint64_t provides 64 bits instead of the 32 bits available in unsigned int. - Update all bit shift operations from 1u to UINT64_C(1) to ensure correct 64-bit shifts without undefined behavior. - Update printf format specifiers from %d to %zu for MAX_REVS since sizeof() expressions produce size_t, not int. I noticed the prior RFC by Meet Soni (Feb 2025, Message-ID: <20250217055024.3978-1-meetsoni3017@gmail.com>) which Junio C Hamano and Jeff King reviewed. That patch did the basic conversion but did not lift the MAX_REVS limitation. This v2 addresses Junio's feedback where he suggested "using a slab whose element is still a bag of bits that is wider than object.flags word is the most straight-forward way to lift MAX_REVS limitation." We use uint64_t as that wider element. builtin/show-branch.c | 106 ++++++++++++++++++++++++------------------ 1 file changed, 61 insertions(+), 45 deletions(-) diff --git a/builtin/show-branch.c b/builtin/show-branch.c index f02831b085..625e456411 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -34,15 +34,13 @@ static enum git_colorbool showbranch_use_color = GIT_COLOR_UNKNOWN; static struct strvec default_args = STRVEC_INIT; -/* - * TODO: convert this use of commit->object.flags to commit-slab - * instead to store a pointer to ref name directly. Then use the same - * UNINTERESTING definition from revision.h here. - */ #define UNINTERESTING 01 +static uint64_t get_rev_flags(struct commit *commit); +static void or_rev_flags(struct commit *commit, uint64_t flags); + #define REV_SHIFT 2 -#define MAX_REVS (FLAG_BITS - REV_SHIFT) /* should not exceed bits_per_int - REV_SHIFT */ +#define MAX_REVS (sizeof(uint64_t) * 8 - REV_SHIFT) #define DEFAULT_REFLOG 4 @@ -64,7 +62,7 @@ static struct commit *interesting(struct prio_queue *queue) { for (size_t i = 0; i < queue->nr; i++) { struct commit *commit = queue->array[i].data; - if (commit->object.flags & UNINTERESTING) + if (get_rev_flags(commit) & UNINTERESTING) continue; return commit; } @@ -79,11 +77,25 @@ struct commit_name { define_commit_slab(commit_name_slab, struct commit_name *); static struct commit_name_slab name_slab; +define_commit_slab(commit_rev_flags, uint64_t); +static struct commit_rev_flags rev_flags_slab; + static struct commit_name *commit_to_name(struct commit *commit) { return *commit_name_slab_at(&name_slab, commit); } +static uint64_t get_rev_flags(struct commit *commit) +{ + uint64_t *f = commit_rev_flags_peek(&rev_flags_slab, commit); + return f ? *f : 0; +} + +static void or_rev_flags(struct commit *commit, uint64_t flags) +{ + *commit_rev_flags_at(&rev_flags_slab, commit) |= flags; +} + /* Name the commit as nth generation ancestor of head_name; * we count only the first-parent relationship for naming purposes. @@ -215,7 +227,7 @@ static void name_commits(struct commit_list *list, static int mark_seen(struct commit *commit, struct commit_list **seen_p) { - if (!commit->object.flags) { + if (!get_rev_flags(commit)) { commit_list_insert(commit, seen_p); return 1; } @@ -226,15 +238,15 @@ 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); + uint64_t all_mask = ((UINT64_C(1) << (REV_SHIFT + num_rev)) - 1); + uint64_t all_revs = all_mask & ~((UINT64_C(1) << 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; + uint64_t flags = get_rev_flags(commit) & all_mask; if (!still_interesting && extra <= 0) break; @@ -246,14 +258,14 @@ static void join_revs(struct prio_queue *queue, while (parents) { struct commit *p = parents->item; - int this_flag = p->object.flags; + uint64_t this_flag = get_rev_flags(p); parents = parents->next; if ((this_flag & flags) == flags) continue; repo_parse_commit(the_repository, p); if (mark_seen(p, seen_p) && !still_interesting) extra--; - p->object.flags |= flags; + or_rev_flags(p, flags); if (get_pending) prio_queue_replace(queue, p); else @@ -278,8 +290,8 @@ static void join_revs(struct prio_queue *queue, struct commit *c = s->item; struct commit_list *parents; - if (((c->object.flags & all_revs) != all_revs) && - !(c->object.flags & UNINTERESTING)) + if (((get_rev_flags(c) & all_revs) != all_revs) && + !(get_rev_flags(c) & UNINTERESTING)) continue; /* The current commit is either a merge base or @@ -292,8 +304,8 @@ static void join_revs(struct prio_queue *queue, while (parents) { struct commit *p = parents->item; parents = parents->next; - if (!(p->object.flags & UNINTERESTING)) { - p->object.flags |= UNINTERESTING; + if (!(get_rev_flags(p) & UNINTERESTING)) { + or_rev_flags(p, UNINTERESTING); changed = 1; } } @@ -410,8 +422,8 @@ static int append_ref(const char *refname, const struct object_id *oid, return 0; } if (MAX_REVS <= ref_name_cnt) { - 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); return 0; } @@ -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); + } } return exit_status; @@ -530,17 +544,17 @@ static int show_merge_base(const struct commit_list *seen, int num_rev) static int show_independent(struct commit **rev, int num_rev, - unsigned int *rev_mask) + uint64_t *rev_mask) { int i; for (i = 0; i < num_rev; i++) { struct commit *commit = rev[i]; - unsigned int flag = rev_mask[i]; + uint64_t flag = rev_mask[i]; - if (commit->object.flags == flag) + if (get_rev_flags(commit) == flag) puts(oid_to_hex(&commit->object.oid)); - commit->object.flags |= UNINTERESTING; + or_rev_flags(commit, UNINTERESTING); } return 0; } @@ -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); 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) @@ -648,10 +662,10 @@ int cmd_show_branch(int ac, char *reflog_msg[MAX_REVS] = {0}; struct commit_list *seen = NULL; struct prio_queue queue = { compare_commits_by_commit_date }; - unsigned int rev_mask[MAX_REVS]; + uint64_t rev_mask[MAX_REVS]; int num_rev, i, extra = 0; int all_heads = 0, all_remotes = 0; - int all_mask, all_revs; + uint64_t all_mask, all_revs; enum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER; char *head; struct object_id head_oid; @@ -714,6 +728,7 @@ int cmd_show_branch(int ac, int ret; init_commit_name_slab(&name_slab); + init_commit_rev_flags(&rev_flags_slab); repo_config(the_repository, git_show_branch_config, NULL); @@ -759,7 +774,7 @@ int cmd_show_branch(int ac, struct object_id oid; char *ref; int base = 0; - unsigned int flags = 0; + uint64_t flags = 0; if (ac == 0) { static const char *fake_av[2]; @@ -779,8 +794,8 @@ int cmd_show_branch(int ac, die(_("--reflog option needs one branch name")); if (MAX_REVS < reflog) - die(Q_("only %d entry can be shown at one time.", - "only %d entries can be shown at one time.", + die(Q_("only %zu entry can be shown at one time.", + "only %zu entries can be shown at one time.", MAX_REVS), MAX_REVS); if (!repo_dwim_ref(the_repository, *av, strlen(*av), &oid, &ref, 0)) @@ -870,11 +885,11 @@ int cmd_show_branch(int ac, for (num_rev = 0; ref_name[num_rev]; num_rev++) { struct object_id revkey; - unsigned int flag = 1u << (num_rev + REV_SHIFT); + uint64_t flag = UINT64_C(1) << (num_rev + REV_SHIFT); if (MAX_REVS <= num_rev) - die(Q_("cannot handle more than %d rev.", - "cannot handle more than %d revs.", + die(Q_("cannot handle more than %zu rev.", + "cannot handle more than %zu revs.", MAX_REVS), MAX_REVS); if (repo_get_oid(the_repository, ref_name[num_rev], &revkey)) die(_("'%s' is not a valid ref."), ref_name[num_rev]); @@ -889,13 +904,13 @@ int cmd_show_branch(int ac, * and so on. REV_SHIFT bits from bit 0 are used for * internal bookkeeping. */ - commit->object.flags |= flag; - if (commit->object.flags == flag) + or_rev_flags(commit, flag); + if (get_rev_flags(commit) == flag) prio_queue_put(&queue, commit); rev[num_rev] = commit; } for (i = 0; i < num_rev; i++) - rev_mask[i] = rev[i]->object.flags; + rev_mask[i] = get_rev_flags(rev[i]); if (0 <= extra) join_revs(&queue, &seen, num_rev, extra); @@ -958,12 +973,12 @@ int cmd_show_branch(int ac, if (!sha1_name && !no_name) name_commits(seen, rev, ref_name, num_rev); - all_mask = ((1u << (REV_SHIFT + num_rev)) - 1); - all_revs = all_mask & ~((1u << REV_SHIFT) - 1); + all_mask = ((UINT64_C(1) << (REV_SHIFT + num_rev)) - 1); + all_revs = all_mask & ~((UINT64_C(1) << REV_SHIFT) - 1); for (struct commit_list *l = seen; l; l = l->next) { struct commit *commit = l->item; - int this_flag = commit->object.flags; + uint64_t this_flag = get_rev_flags(commit); int is_merge_point = ((this_flag & all_revs) == all_revs); shown_merge_point |= is_merge_point; @@ -973,14 +988,14 @@ int cmd_show_branch(int ac, commit->parents->next); if (topics && !is_merge_point && - (this_flag & (1u << REV_SHIFT))) + (this_flag & (UINT64_C(1) << REV_SHIFT))) continue; if (!sparse && is_merge && omit_in_dense(commit, rev, num_rev)) continue; for (i = 0; i < num_rev; i++) { int mark; - if (!(this_flag & (1u << (i + REV_SHIFT)))) + if (!(this_flag & (UINT64_C(1) << (i + REV_SHIFT)))) mark = ' '; else if (is_merge) mark = '-'; @@ -1010,6 +1025,7 @@ int cmd_show_branch(int ac, free(reflog_msg[i]); commit_list_free(seen); clear_prio_queue(&queue); + clear_commit_rev_flags(&rev_flags_slab); free(args_copy); free(head); return ret; -- 2.54.0