Re: [PATCH v2] promisor-remote: fix xcalloc() argument order

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 25, 2022, 13:51 UTC
Message-ID: <220825.86a67s4e6r.gmgdl@evledraar.gmail.com>
In-Reply-To: <YwXiKXONiTt+fIdi@coredump.intra.peff.net>

On Wed, Aug 24 2022, Jeff King wrote:

Show 24 quoted lines

> On Tue, Aug 23, 2022 at 11:57:33AM +0200, SZEDER Gábor wrote:
>
>> Pass the number of elements first and their size second, as expected
>> by xcalloc().
>> 
>> Patch generated with:
>> 
>>   make SPATCH_FLAGS=--recursive-includes contrib/coccinelle/xcalloc.cocci.patch
>
> Thanks for digging here. I think it probably explains a lot of "wait,
> why did this result change" head-scratching I did back when we started
> batching a few years ago.
>
> Is there any reason we wouldn't want --recursive-includes to be added to
> the default SPATCH_FLAGS?
>
> I suspect we'd still want to leave --all-includes there to get as much
> type information as possible. If I understand correctly, the two are
> orthogonal (one is "follow includes of includes" and the other is
> "follow system includes in angle brackets").
>
> Doing so doesn't seem to find any other missed entries in the current
> codebase, but I'm pretty sure there are some it would have caught in a
> less mysterious fashion over the years.

This feels to me like hacks around other issues we should fix the root cause of.

So, first of all, I think this is a perfectly good fix, and something we
should do more of in general. It'll apply the wanted change *and* speed
up the run:
	
	diff --git a/contrib/coccinelle/xcalloc.cocci b/contrib/coccinelle/xcalloc.cocci
	index c291011607e..bd51e33af83 100644
	--- a/contrib/coccinelle/xcalloc.cocci
	+++ b/contrib/coccinelle/xcalloc.cocci
	@@ -1,10 +1,8 @@
	 @@
	-type T;
	-T *ptr;
	 expression n;
	 @@
	   xcalloc(
	 + n,
	-  \( sizeof(T) \| sizeof(*ptr) \)
	+  sizeof(...)
	 - , n
	   )

But it's *not* functionally the same thing, pedantically speaking, but I think it's fine. I pointed this out at the the time in [1].

Also, more generally we could avoid includes in headers, and use forward decls. This would make e.g. the "iwyu" tool report the missing include.

Unfortunately it seems we added it in this case due to "the_repository". I wonder if this hack would be better:

	diff --git a/Makefile b/Makefile
	index 9410a587fc0..92d5726b392 100644
	--- a/Makefile
	+++ b/Makefile
	@@ -3119,6 +3119,7 @@ HCC = $(HCO:hco=hcc)
	 
	 %.hcc: %.h
	 	@echo '#include "git-compat-util.h"' >$@
	+	@echo "extern struct repository *the_repository;" >>$@
	 	@echo '#include "$<"' >>$@
	 
	 $(HCO): %.hco: %.hcc FORCE
	diff --git a/promisor-remote.h b/promisor-remote.h
	index edc45ab0f5f..e864c093a44 100644
	--- a/promisor-remote.h
	+++ b/promisor-remote.h
	@@ -1,7 +1,7 @@
	 #ifndef PROMISOR_REMOTE_H
	 #define PROMISOR_REMOTE_H
	 
	-#include "repository.h"
	+struct repository;
	 
	 struct object_id;
	 
	@@ -23,6 +23,7 @@ static inline void promisor_remote_reinit(void)
	 	repo_promisor_remote_reinit(the_repository);
	 }
	 
	+struct promisor_remote_config;
	 void promisor_remote_clear(struct promisor_remote_config *config);
	 
	 struct promisor_remote *repo_promisor_remote_find(struct repository *r, const char *remote_name);

So yeah, one way to deal with this is --recursive-includes, but I think we're better off heading in the direction of having the includes for a given *.c file be what it actually needs, and not rely on includes by proxy, or in this case to pollute the namespace of everyone using promisor-remote.h with remote.h (probably doesn't matter in that case, but in the general case...).

1. https://lore.kernel.org/git/87ft18tcog.fsf@evledraar.gmail.com/