Re: [GSoC PATCH] object-name: turn INTERPRET_BRANCH_* constants into enum values

[Junio C Hamano <gitster@pobox.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.

> @@ -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?

> +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.