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

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

On Thu, Feb 03 2022, Han-Wen Nienhuys wrote:

> On Thu, Feb 3, 2022 at 7:05 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> > I proposed both options because a distinct typename lets me jump to
>> > the definition of the flags easily through ctags.
>>
>> I'm not sure I understand you here. I use ctags (via Emacs) and it's
>
> "I proposed both options" (ie. enum or typedef) , so we are in
> resounding agreement.

Ah, so by "because a distinct typename lets me jump to the definition of
the flags easily through ctags." you mean one of "enum x" or "typedef
enum { ... } x", not just the latter?

>> > Another idea is to mark the type of the flags by its name, eg.
>> > transaction_flags, resolve_flags, reftype_flags etc. This wouldn't
>> > help with ctags, but it does help with readability.
>>
>> Yes, enums or not, what I was also pointing out in
>> https://lore.kernel.org/git/220201.86ilty9vq2.gmgdl@evledraar.gmail.com/
>> is that changing just one logical set of flags at a time would make this
>> much easier to review.
>>
>> It doesn't matter for the end result as long as we end up with "unsigned
>> int" everywhere, but would with enums.
>
> Not sure if you need to review it in that detail. If you change a
> definition in the .h file,  the compiler will complain about all
> mismatches. So it doesn't need human verification once you know it
> compiles.

That's true in C++ I think, but not C. Or do you have a compiler that'll
warn about e.g. this change:
	
	diff --git a/refs.c b/refs.c
	index addb26293b4..e6c3931ec00 100644
	--- a/refs.c
	+++ b/refs.c
	@@ -1078,7 +1078,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
	 			   const char *refname,
	 			   const struct object_id *new_oid,
	 			   const struct object_id *old_oid,
	-			   unsigned int flags, const char *msg,
	+			   enum foobar flags, const char *msg,
	 			   struct strbuf *err)
	 {
	 	assert(err);
	diff --git a/refs.h b/refs.h
	index 8f91a7f9ff2..63dde1da4de 100644
	--- a/refs.h
	+++ b/refs.h
	@@ -670,11 +670,12 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
	  * See the above comment "Reference transaction updates" for more
	  * information.
	  */
	+enum foobar { FOOBAR = 0 };
	 int ref_transaction_update(struct ref_transaction *transaction,
	 			   const char *refname,
	 			   const struct object_id *new_oid,
	 			   const struct object_id *old_oid,
	-			   unsigned int flags, const char *msg,
	+			   enum foobar flags, const char *msg,
	 			   struct strbuf *err);
	 
	 /*

What I'm referring to, keeping in mind that that doesn't warn, is that
since we can't get the compiler to whine about e.g. that "flags" being
compared against values not in the enum, or when I pass that "enum" to a
not-that-enum right after in ref_transaction_add_update() is that it's
extra useful for reviewing these sorts of changes if what's logically
one flag is changed at a time, as opposed to a big search/replacement
(and tracking things down for s/int/enum/ would force one to do that).

For doing this sort of change in C I find it to be a useful technique to do this:
	
	diff --git a/refs.c b/refs.c
	index addb26293b4..ab58dd8948d 100644
	--- a/refs.c
	+++ b/refs.c
	@@ -1078,7 +1078,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
	 			   const char *refname,
	 			   const struct object_id *new_oid,
	 			   const struct object_id *old_oid,
	-			   unsigned int flags, const char *msg,
	+			   unsigned int *flags, const char *msg,
	 			   struct strbuf *err)
	 {
	 	assert(err);
	diff --git a/refs.h b/refs.h
	index 8f91a7f9ff2..80ef4616838 100644
	--- a/refs.h
	+++ b/refs.h
	@@ -674,7 +674,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
	 			   const char *refname,
	 			   const struct object_id *new_oid,
	 			   const struct object_id *old_oid,
	-			   unsigned int flags, const char *msg,
	+			   unsigned int *flags, const char *msg,
	 			   struct strbuf *err);
	 
	 /*


Which will get you a hard compilation error, e.g.:
	
	$ make builtin/update-ref.o
	    CC builtin/update-ref.o
	builtin/update-ref.c:205:8: error: incompatible integer to pointer conversion passing 'unsigned int' to parameter of type 'unsigned int *' [-Werror,-Wint-conversion]
	                                   update_flags | create_reflog_flag,
	                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	./refs.h:677:21: note: passing argument to parameter 'flags' here
	                           unsigned int *flags, const char *msg,
	                                         ^
	1 error generated.
	make: *** [Makefile:2542: builtin/update-ref.o] Error 1

Which you'd then "fix" like this:
		
	diff --git a/builtin/update-ref.c b/builtin/update-ref.c
	index a84e7b47a20..33bcde36871 100644
	--- a/builtin/update-ref.c
	+++ b/builtin/update-ref.c
	@@ -185,6 +185,7 @@ static void parse_cmd_update(struct ref_transaction *transaction,
	 	char *refname;
	 	struct object_id new_oid, old_oid;
	 	int have_old;
	+	unsigned int f = update_flags | create_reflog_flag;
	 
	 	refname = parse_refname(&next);
	 	if (!refname)
	@@ -202,7 +203,7 @@ static void parse_cmd_update(struct ref_transaction *transaction,
	 
	 	if (ref_transaction_update(transaction, refname,
	 				   &new_oid, have_old ? &old_oid : NULL,
	-				   update_flags | create_reflog_flag,
	+				   &f,
	 				   msg, &err))
	 		die("%s", err.buf);
	 
Now, obviously those changes suck, but the point is that if you do it
like that you can be assured that you got all callsites, so if you first
change it to an "int *", then get it to compile, and then search/replace
the resulting hunks you just changed & repeat, you can be assured that
you got all the callers, and that we don't have cases left where an
"int" becomes "unsigned int", or that our shiny new "enum" is
immediately passed as an "int" etc.