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

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

[Jialong Wang]

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.
---
 builtin/branch.c |  2 +-
 object-name.c    |  7 +++++--
 object-name.h    | 11 +++++++----
 refs.c           |  3 ++-
 refs.h           |  3 ++-
 5 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index a1a43380d0..6ef6a7ca0f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -228,7 +228,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	int ret = 0;
 	int remote_branch = 0;
 	struct strbuf bname = STRBUF_INIT;
-	unsigned allowed_interpret;
+	enum interpret_branch_name_allowed allowed_interpret;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *item;
 	int branch_name_pos;
diff --git a/object-name.c b/object-name.c
index 7b14c3bf9b..fbf7c5dbf0 100644
--- a/object-name.c
+++ b/object-name.c
@@ -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)
 {
 	if (!allowed)
 		return 1;
diff --git a/object-name.h b/object-name.h
index cda4934cd5..a544b65235 100644
--- a/object-name.h
+++ b/object-name.h
@@ -101,9 +101,12 @@ int set_disambiguate_hint_config(const char *var, const char *value);
  * If the input was ok but there are not N branch switches in the
  * reflog, it returns 0.
  */
-#define INTERPRET_BRANCH_LOCAL (1<<0)
-#define INTERPRET_BRANCH_REMOTE (1<<1)
-#define INTERPRET_BRANCH_HEAD (1<<2)
+enum interpret_branch_name_allowed {
+	INTERPRET_BRANCH_LOCAL = (1 << 0),
+	INTERPRET_BRANCH_REMOTE = (1 << 1),
+	INTERPRET_BRANCH_HEAD = (1 << 2),
+};
+
 struct interpret_branch_name_options {
 	/*
 	 * If "allowed" is non-zero, it is a treated as a bitfield of allowable
@@ -111,7 +114,7 @@ struct interpret_branch_name_options {
 	 * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
 	 * allowed, even ones to refs outside of those namespaces.
 	 */
-	unsigned allowed;
+	enum interpret_branch_name_allowed allowed;
 
 	/*
 	 * If ^{upstream} or ^{push} (or equivalent) is requested, and the
diff --git a/refs.c b/refs.c
index 6fb8f9d10c..b3b39ce37f 100644
--- a/refs.c
+++ b/refs.c
@@ -740,7 +740,8 @@ static char *substitute_branch_name(struct repository *r,
 	return NULL;
 }
 
-void copy_branchname(struct strbuf *sb, const char *name, unsigned allowed)
+void copy_branchname(struct strbuf *sb, const char *name,
+		     enum interpret_branch_name_allowed allowed)
 {
 	int len = strlen(name);
 	struct interpret_branch_name_options options = {
diff --git a/refs.h b/refs.h
index d98c1fc591..8a5b034f8a 100644
--- a/refs.h
+++ b/refs.h
@@ -1,6 +1,7 @@
 #ifndef REFS_H
 #define REFS_H
 
+#include "object-name.h"
 #include "commit.h"
 #include "repository.h"
 #include "repo-settings.h"
@@ -225,7 +226,7 @@ char *repo_default_branch_name(struct repository *r, int quiet);
  * repo_interpret_branch_name() for details.
  */
 void copy_branchname(struct strbuf *sb, const char *name,
-		       unsigned allowed);
+		     enum interpret_branch_name_allowed allowed);
 
 /*
  * Like copy_branchname() above, but confirm that the result is
-- 
2.51.0

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

[Junio C Hamano]

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.

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

[Jialong Wang]

Replace the INTERPRET_BRANCH_* preprocessor constants with enum
values and use that type where these flags are stored or passed
around.

These flags describe which kinds of branches may be considered during
branch-name interpretation, so represent them as an enum describing
branch kinds while keeping the existing bitmask semantics and
INTERPRET_BRANCH_* element names.

Signed-off-by: Jialong Wang <jerrywang183@yahoo.com>
---
v2:
 - rename the enum type to reflect that it describes branch kinds,
   not the allowed set itself
 - keep the INTERPRET_BRANCH_* enum element names unchanged
 - wrap updated declarations and commit message more conventionally

 builtin/branch.c |  2 +-
 object-name.c    |  6 ++++--
 object-name.h    | 11 +++++++----
 refs.c           |  3 ++-
 refs.h           |  3 ++-
 5 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index a1a43380d0..1572a4f9ef 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -228,7 +228,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	int ret = 0;
 	int remote_branch = 0;
 	struct strbuf bname = STRBUF_INIT;
-	unsigned allowed_interpret;
+	enum interpret_branch_kind allowed_interpret;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *item;
 	int branch_name_pos;
diff --git a/object-name.c b/object-name.c
index 7b14c3bf9b..2b2a0435fd 100644
--- a/object-name.c
+++ b/object-name.c
@@ -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_kind allowed)
 {
 	/* we have extra data, which might need further processing */
 	struct strbuf tmp = STRBUF_INIT;
@@ -1692,7 +1693,8 @@ 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_kind allowed)
 {
 	if (!allowed)
 		return 1;
diff --git a/object-name.h b/object-name.h
index cda4934cd5..167a9154ea 100644
--- a/object-name.h
+++ b/object-name.h
@@ -101,9 +101,12 @@ int set_disambiguate_hint_config(const char *var, const char *value);
  * If the input was ok but there are not N branch switches in the
  * reflog, it returns 0.
  */
-#define INTERPRET_BRANCH_LOCAL (1<<0)
-#define INTERPRET_BRANCH_REMOTE (1<<1)
-#define INTERPRET_BRANCH_HEAD (1<<2)
+enum interpret_branch_kind {
+	INTERPRET_BRANCH_LOCAL = (1 << 0),
+	INTERPRET_BRANCH_REMOTE = (1 << 1),
+	INTERPRET_BRANCH_HEAD = (1 << 2),
+};
+
 struct interpret_branch_name_options {
 	/*
 	 * If "allowed" is non-zero, it is a treated as a bitfield of allowable
@@ -111,7 +114,7 @@ struct interpret_branch_name_options {
 	 * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
 	 * allowed, even ones to refs outside of those namespaces.
 	 */
-	unsigned allowed;
+	enum interpret_branch_kind allowed;
 
 	/*
 	 * If ^{upstream} or ^{push} (or equivalent) is requested, and the
diff --git a/refs.c b/refs.c
index 6fb8f9d10c..18b28db6d4 100644
--- a/refs.c
+++ b/refs.c
@@ -740,7 +740,8 @@ static char *substitute_branch_name(struct repository *r,
 	return NULL;
 }
 
-void copy_branchname(struct strbuf *sb, const char *name, unsigned allowed)
+void copy_branchname(struct strbuf *sb, const char *name,
+		     enum interpret_branch_kind allowed)
 {
 	int len = strlen(name);
 	struct interpret_branch_name_options options = {
diff --git a/refs.h b/refs.h
index d98c1fc591..d65de6ab5f 100644
--- a/refs.h
+++ b/refs.h
@@ -1,6 +1,7 @@
 #ifndef REFS_H
 #define REFS_H
 
+#include "object-name.h"
 #include "commit.h"
 #include "repository.h"
 #include "repo-settings.h"
@@ -225,7 +226,7 @@ char *repo_default_branch_name(struct repository *r, int quiet);
  * repo_interpret_branch_name() for details.
  */
 void copy_branchname(struct strbuf *sb, const char *name,
-		       unsigned allowed);
+		     enum interpret_branch_kind allowed);
 
 /*
  * Like copy_branchname() above, but confirm that the result is
-- 
2.51.0

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

Jialong Wang <jerrywang183@yahoo.com> writes:

> Replace the INTERPRET_BRANCH_* preprocessor constants with enum
> values and use that type where these flags are stored or passed
> around.
>
> These flags describe which kinds of branches may be considered during
> branch-name interpretation, so represent them as an enum describing
> branch kinds while keeping the existing bitmask semantics and
> INTERPRET_BRANCH_* element names.
>
> Signed-off-by: Jialong Wang <jerrywang183@yahoo.com>

[snip]

> diff --git a/object-name.h b/object-name.h
> index cda4934cd5..167a9154ea 100644
> --- a/object-name.h
> +++ b/object-name.h
> @@ -101,9 +101,12 @@ int set_disambiguate_hint_config(const char *var, const char *value);
>   * If the input was ok but there are not N branch switches in the
>   * reflog, it returns 0.
>   */
> -#define INTERPRET_BRANCH_LOCAL (1<<0)
> -#define INTERPRET_BRANCH_REMOTE (1<<1)
> -#define INTERPRET_BRANCH_HEAD (1<<2)
> +enum interpret_branch_kind {
> +	INTERPRET_BRANCH_LOCAL = (1 << 0),
> +	INTERPRET_BRANCH_REMOTE = (1 << 1),
> +	INTERPRET_BRANCH_HEAD = (1 << 2),
> +};

Generally when we use preprocessor constants with bit setting like
`1 << 0`, we want to use them as flags which aren't mutually exclusive,
allowing us to do 'INTERPRET_BRANCH_LOCAL | INTERPRET_BRANCH_HEAD' and
so on.

Is this the case here? If not, maybe we want to mention that explicitly
and simply use '1, 2....N'?

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>> diff --git a/object-name.h b/object-name.h
>> index cda4934cd5..167a9154ea 100644
>> --- a/object-name.h
>> +++ b/object-name.h
>> @@ -101,9 +101,12 @@ int set_disambiguate_hint_config(const char *var, const char *value);
>>   * If the input was ok but there are not N branch switches in the
>>   * reflog, it returns 0.
>>   */
>> -#define INTERPRET_BRANCH_LOCAL (1<<0)
>> -#define INTERPRET_BRANCH_REMOTE (1<<1)
>> -#define INTERPRET_BRANCH_HEAD (1<<2)
>> +enum interpret_branch_kind {
>> +	INTERPRET_BRANCH_LOCAL = (1 << 0),
>> +	INTERPRET_BRANCH_REMOTE = (1 << 1),
>> +	INTERPRET_BRANCH_HEAD = (1 << 2),
>> +};
>
> Generally when we use preprocessor constants with bit setting like
> `1 << 0`, we want to use them as flags which aren't mutually exclusive,
> allowing us to do 'INTERPRET_BRANCH_LOCAL | INTERPRET_BRANCH_HEAD' and
> so on.
>
> Is this the case here? If not, maybe we want to mention that explicitly
> and simply use '1, 2....N'?

Taking a brief look at the way these constants are used, e.g.,

        static int branch_interpret_allowed(const char *refname, unsigned allowed)
        {
                if (!allowed)
                        return 1;

                if ((allowed & INTERPRET_BRANCH_LOCAL) &&
                    starts_with(refname, "refs/heads/"))
                        return 1;
                if ((allowed & INTERPRET_BRANCH_REMOTE) &&
                    starts_with(refname, "refs/remotes/"))
                        return 1;

                return 0;
        }

it should be obvious that these are not mutually exclusive choices,
rather they are independent flags that you can flip ON to express
dwimming a short name to what types of branches are allowed.

Besides, the original assignes one-bit-per-value to these constants;
it is not a place for this "CPP macro turned into enum to help those
who inspect a running program with gdb" patch to change assignments
of values.  That would be a separate topic.