archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <>
To: Jeff King <>
Cc: "SZEDER Gábor" <>,, "Junio C Hamano" <>
Subject: Re: [PATCH v2] promisor-remote: fix xcalloc() argument order
Date: Thu, 25 Aug 2022 15:51:21 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Wed, Aug 24 2022, Jeff King wrote:

> 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;
	 + 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 @@
	-#include "repository.h"
	+struct repository;
	 struct object_id;
	@@ -23,6 +23,7 @@ static inline void promisor_remote_reinit(void)
	+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...).


  parent reply	other threads:[~2022-08-25 13:59 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-22 21:34 [PATCH] promisor-remote: fix xcalloc() argument order SZEDER Gábor
2022-08-22 22:14 ` Junio C Hamano
2022-08-23  1:09   ` Junio C Hamano
2022-08-23  7:04     ` SZEDER Gábor
2022-08-23  9:21       ` SZEDER Gábor
2022-08-23  9:56         ` SZEDER Gábor
2022-08-23  9:57 ` [PATCH v2] " SZEDER Gábor
2022-08-24  8:32   ` Jeff King
2022-08-24 11:39     ` SZEDER Gábor
2022-08-25 10:40       ` Jeff King
2022-08-25 13:51     ` Ævar Arnfjörð Bjarmason [this message]
2022-08-24 15:58   ` Junio C Hamano
2022-08-24 17:33     ` SZEDER Gábor
2022-08-24 21:13       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).