Re: [PATCH v2] refs.h: make all flags arguments unsigned

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Feb 01 2022, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> As discussed in
> https://lore.kernel.org/git/xmqqbkzrkevo.fsf@gitster.g/ , we don't
> want to treat the sign bit specially, so make all flags in refs.h
> unsigned.
>
> For uniformity, rename all variables to `flags` or `unused_flags`,
> from `flag`. In a couple of shadowing cases, use `ref_flags` for
> clarity.

For what it's worth I thought the suggestion of enums in your
https://lore.kernel.org/git/xmqqbkzrkevo.fsf@gitster.g/ made more sense,
e.g. I tried this on top:
	
	diff --git a/refs.c b/refs.c
	index 5f29775def1..dc33573f064 100644
	--- a/refs.c
	+++ b/refs.c
	@@ -265,7 +265,8 @@ int ref_resolves_to_object(const char *refname,
	 }
	 
	 char *refs_resolve_refdup(struct ref_store *refs, const char *refname,
	-			  int resolve_flags, struct object_id *oid,
	+			  enum resolve_ref_flags resolve_flags,
	+			  struct object_id *oid,
	 			  unsigned int *flags)
	 {
	 	const char *result;
	@@ -276,7 +277,8 @@ char *refs_resolve_refdup(struct ref_store *refs, const char *refname,
	 	return xstrdup_or_null(result);
	 }
	 
	-char *resolve_refdup(const char *refname, unsigned int resolve_flags,
	+char *resolve_refdup(const char *refname,
	+		     enum resolve_ref_flags resolve_flags,
	 		     struct object_id *oid, unsigned int *flags)
	 {
	 	return refs_resolve_refdup(get_main_ref_store(the_repository),
	@@ -292,7 +294,8 @@ struct ref_filter {
	 	void *cb_data;
	 };
	 
	-int read_ref_full(const char *refname, unsigned int resolve_flags,
	+int read_ref_full(const char *refname,
	+		  enum resolve_ref_flags resolve_flags,
	 		  struct object_id *oid, unsigned int *flags)
	 {
	 	int ignore_errno;
	@@ -1679,7 +1682,8 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
	 }
	 
	 const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
	-				    int resolve_flags, struct object_id *oid,
	+				    enum resolve_ref_flags resolve_flags,
	+				    struct object_id *oid,
	 				    unsigned int *flags, int *failure_errno)
	 {
	 	static struct strbuf sb_refname = STRBUF_INIT;
	@@ -1779,7 +1783,8 @@ int refs_init_db(struct strbuf *err)
	 	return refs->be->init_db(refs, err);
	 }
	 
	-const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
	+const char *resolve_ref_unsafe(const char *refname,
	+			       enum resolve_ref_flags resolve_flags,
	 			       struct object_id *oid, unsigned int *flags)
	 {
	 	int ignore_errno;
	diff --git a/refs.h b/refs.h
	index c5462b75807..8c7404eaf78 100644
	--- a/refs.h
	+++ b/refs.h
	@@ -64,24 +64,30 @@ struct worktree;
	  * type of failure encountered, but not necessarily one that came from
	  * a syscall. We might have faked it up.
	  */
	-#define RESOLVE_REF_READING 0x01
	-#define RESOLVE_REF_NO_RECURSE 0x02
	-#define RESOLVE_REF_ALLOW_BAD_NAME 0x04
	+enum resolve_ref_flags {
	+	RESOLVE_REF_READING = 1 << 0,
	+	RESOLVE_REF_NO_RECURSE = 1 << 1,
	+	RESOLVE_REF_ALLOW_BAD_NAME = 1 << 2,
	+};
	 
	 const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
	-				    int resolve_flags, struct object_id *oid,
	+				    enum resolve_ref_flags resolve_flags,
	+				    struct object_id *oid,
	 				    unsigned int *flags, int *failure_errno);
	 
	-const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
	+const char *resolve_ref_unsafe(const char *refname,
	+			       enum resolve_ref_flags resolve_flags,
	 			       struct object_id *oid, unsigned int *flags);
	 
	 char *refs_resolve_refdup(struct ref_store *refs, const char *refname,
	-			  int resolve_flags, struct object_id *oid,
	+			  enum resolve_ref_flags resolve_flags,
	+			  struct object_id *oid,
	 			  unsigned int *flags);
	-char *resolve_refdup(const char *refname, unsigned int resolve_flags,
	+char *resolve_refdup(const char *refname,
	+		     enum resolve_ref_flags resolve_flags,
	 		     struct object_id *oid, unsigned int *flags);
	 
	-int read_ref_full(const char *refname, unsigned int resolve_flags,
	+int read_ref_full(const char *refname, enum resolve_ref_flags resolve_flags,
	 		  struct object_id *oid, unsigned int *flags);
	 int read_ref(const char *refname, struct object_id *oid);

I don't mind if this goes in, but I think doing that would make this
much easier to deal with, as it would be more obvious when working on
the code just what flag you're dealing with. Even as Junio points out
downthread of that it mostly doesn't matter in C.

See though 3f9ab7ccdea (parse-options.[ch]: consistently use "enum
parse_opt_flags", 2021-10-08) where doing it this way makes debugging
much nicer to work with.

> -		int flag;
> -		char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
> -		if (head_ref &&
> -		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
> +		unsigned int flags;
> +		char *head_ref = resolve_refdup("HEAD", 0, NULL, &flags);
> +		if (head_ref && (!(flags & REF_ISSYMREF) ||
> +				 strcmp(head_ref, new_branch_info->path)))

I don't think this needs a re-roll, but FWIW I think it's much easier to
review changes like these if the changes are kept apart. E.g. this
variable renaming from the type change.

> -static int add_one_refname(const char *refname,
> -			   const struct object_id *oid,
> -			   int flag, void *cbdata)
> +static int add_one_refname(const char *refname, const struct object_id *oid,
> +			   unsigned int unused_flags, void *cbdata)

And this change not mentioned in the commit message of seemingly doing
some re-flowing of argument lists while we're at it.

The post-image is better, but better done is another step IMO.

I didn't look through the rest of the diff as you sent it, but locally
with appropriate word-diff flags to hide any formatting changes.

The post-image LGTM, but I'm also a bit "meh" on the churn just for
signed->unsigned, especially given the conflict with my in-flight
ab/no-errno-from-resolve-ref-unsafe. But it's not too bad, and if Junio
hasn't complained about it...