Re: [GSoC PATCH] object-name: turn INTERPRET_BRANCH_* constants into enum values
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 18, 2026, 16:27 UTC
- Message-ID
- <xmqqjyv9gl2a.fsf@gitster.g>
- In-Reply-To
- <20260318003917.84792-1-jerrywang183@yahoo.com>
Jialong Wang <jerrywang183@yahoo.com> writes:
> Replace the INTERPRET_BRANCH_* preprocessor constants with enum values and use the enum type where these flags are stored or passed around. > > This keeps the existing bitmask semantics, but gives the branch-name interpretation flags a dedicated type instead of plain unsigned values. > ---
Overlong lines without sign-off.
Show 18 quoted lines
> @@ -1660,7 +1660,8 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str
>
> static int reinterpret(struct repository *r,
> const char *name, int namelen, int len,
> - struct strbuf *buf, unsigned allowed)
> + struct strbuf *buf,
> + enum interpret_branch_name_allowed allowed)
> {
> /* we have extra data, which might need further processing */
> struct strbuf tmp = STRBUF_INIT;
> @@ -1692,7 +1693,9 @@ static void set_shortened_ref(struct repository *r, struct strbuf *buf, const ch
> free(s);
> }
>
> -static int branch_interpret_allowed(const char *refname, unsigned allowed)
> +static int branch_interpret_allowed(
> + const char *refname,
> + enum interpret_branch_name_allowed allowed)A funny way to wrap lines. Use what you have in the previous hunk as a template, perhaps?
Show 5 quoted lines
> +enum interpret_branch_name_allowed {
> + INTERPRET_BRANCH_LOCAL = (1 << 0),
> + INTERPRET_BRANCH_REMOTE = (1 << 1),
> + INTERPRET_BRANCH_HEAD = (1 << 2),
> +};I do not think "allowed" matches what this set represents.
The way "reinterpret" uses a parameter of this type (or the bitmask) is to specify which _kind_ of branches are _allowed_ to be considered for its output. The bitset is used to specify the KIND that are ALLOWED. The type should identify itself as representing the kinds of branches, while the parameter name should reflect what these kinds are telling the function to do (i.e., allowed).
I'd name it "enum interpret_branch_kind" or somehing, if I were doing this patch. If the type can stay private to a single C file, we may want to even lose "interpret_" prefix, but I do not think that is the case.
By the way, in the longer term, the set may even want to be possibly unified with what "git branch --list [--remote | --all]" internally uses. At that point it might even become shorter set that looks like
enum branch_kind {
BRANCH_LOCAL, BRANCH_REMOTE, BRANCH_HEAD,
};or "enum ref_kind" that also covers different ref hierarchies like "tags" and "notes".
Needless to say, I do *NOT* want you to be doing this as part of this patch; I do *NOT* want you to drop INTERPRET_ prefix from the names of enum elements in this patch, either.