Header cleanups (splitting up cache.h)

[newren]

(gitgitgadget ran into an issue sending out v3. I'm trying to resend, but I do not know if it'll come out as v3 or v4...)

Changes since v2:

• Dropped patch 21 (Calvin is making more thorough changes to strbuf.[ch] in his series which will obsolete this patch and dropping it makes it easier to deconflict.)
• Added Calvin's Acked-by to all the other patches.

Changes since v1:

• Moved a few more things from cache.h to object.h, object-file.h, and object-name.h
• Split the object.h changes into two patches to be more like all the other patches in 8-19
• Squashed patches 22 & 23, since both were just manual replacements of cache.h inclusion with some other header
• Removed a couple cases of duplicate inclusions of git-compat-util.h

Maintainer notes: (1) This series is based on a merge of en/header-split-cleanup and ab/remove-implicit-use-of-the-repository (both in next). (2) Although I've tweaked the series to minimize conflicts (and this series merges syntactically cleanly with next and seen), there is a semantic conflict with each of ja/worktree-orphan and ab/tag-object-type-errors in seen; to correct, simply add an include of advice.h in builtin/worktree.c and an include of gettext.h in tag.c, respectively.

This series builds on en/header-cleanup (https://lore.kernel.org/git/pull.1485.v2.git.1677197376.gitgitgadget@gmail.com/) and en/header-split-cleanup (https://lore.kernel.org/git/pull.1493.v2.git.1679379968.gitgitgadget@gmail.com/), and continues to focus on splitting declarations from cache.h to separate headers. This series continues dropping the number of cache.h includes; in this series we go from 254 such includes to 190.

The series may appear to be long at first glance, but is repetitive and simple. It should be relatively easy to review, and falls into roughly 3 categories. An overview:

• Patches 1-6, 7: Being more explicit about dependencies. This was motivated by the fact that trying to find unnecessary dependencies on cache.h were being made harder by implicit dependencies on trace.h, trace2.h, and advice.h that were included via cache.h. (Similar to gettext.h handling in the previous series.) So I simply try to make dependencies more explicit, for both these headers and a few others. To make review easy, I split it into half a dozen patches, one header per patch (well, except that I handle trace.h and trace2.h together). Patch 7 then removes several includes of cache.h that are no longer needed due to patches 1-6.
• Patches 8-19: For several choices of FOO, move declarations of functions for FOO.c from cache.h to FOO.h. To simplify reviewing, each case is split into two patches, with the second patch cleaning up unnecessary includes of cache.h in other source files.
• Patches 20-24: Other small manual cleanups noticed while doing above work

Since patches 1-15 & 17-19 are just more of the same types of patches already reviewed in the last two series, it probably makes more sense for reviewers to focus on the other patches: 16 + 20-24 (which also happen to be smaller). I would particularly most like review of patches 16, 22, & 24 since they are the least like any previously reviewed patches.

There are more changes I plan to make after this series graduates (still focused around splitting up cache.h), but this series was long enough.

And thanks once again to Dscho for gitgitgadget. Getting multiple platform testing + all the special tests (sparse, cocci, hdr-check, etc.) really helps clean out all the issues that would otherwise hit a series like this.

cc: Calvin Wan calvinwan@google.com
cc: Elijah Newren newren@gmail.com
cc: "Elijah Newren via GitGitGadget" gitgitgadget@gmail.com
cc: Michael J Gruber git@grubix.eu
cc: Jeff King peff@peff.net
cc: Eric Sunshine sunshine@sunshineco.com

[newren]

/submit

[gitgitgadget]

Error: git fetch https://github.com/gitgitgadget/git -- +refs/notes/gitgitgadget:refs/notes/gitgitgadget failed: 128,
fatal: unable to access 'https://github.com/gitgitgadget/git/': Failed to connect to github.com port 443: Connection refused

[newren]

/submit

[gitgitgadget]

Submitted as pull.1509.git.1680361837.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1509/newren/header-cleanup-3-v1 

To fetch this version to local tag pr-1509/newren/header-cleanup-3-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1509/newren/header-cleanup-3-v1 

[gitgitgadget]

The pull request has 33 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Sat, Apr 1, 2023 at 8:10 AM Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote: > [...] > This series builds on en/header-cleanup > (https://lore.kernel.org/git/pull.1485.v2.git.1677197376.gitgitgadget@gmail.com/) > and en/header-split-cleanup > (https://lore.kernel.org/git/pull.1493.v2.git.1679379968.gitgitgadget@gmail.com/), > and continues to focus on splitting declarations from cache.h to separate > headers. [...] > > There are more changes I plan to make after this series graduates (still > focused around splitting up cache.h), but this series was long enough. While preparing some of those additional splits of cache.h, I found a few small tweaks that make more sense being squashed into this series. I'll send out a re-roll tonight.

[gitgitgadget]

User Elijah Newren <newren@gmail.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <newren@gmail.com> writes: >> 5579f44d2f ("treewide: remove unnecessary cache.h inclusion", 2023-04-11) >> broke connections via git protocol because it removed the inclusion of >> the default port macro. While some may consider this transport to be >> deprecated, it still serves some purpose. > > In particular the problem is that > >	const char *port = STR(DEFAULT_GIT_PORT); > > translates now to > >	const char *port = "DEFAULT_GIT_PORT"; > > instead of > >	const char *port = "9418"; Wow, that is a bad one. If people do want "DEFAULT_GIT_PORT", they would never write STR(DEFAULT_GIT_PORT). I wonder if we can have a bit more clever STR() macro that catches this kind of mistake at the compile time. If this is worth fixing, the fix would probably be worth protecting with a test or two, but the networking test with fixed port is not something we can easily do without a sealed environment, so... Thanks Michael for catching this. > I've got a patch that does precisely this that I just submitted as > part of my follow-on to the en/header-split-cache-h series. I've included > that patch below in case Junio wants to advance it faster than the rest of > that series. Yeah, burying it in a 24-patch series is a bit unfortunate. > -- >8 -- > Subject: [PATCH] protocol.h: move definition of DEFAULT_GIT_PORT from cache.h > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > cache.h | 21 --------------------- > daemon.c | 1 + > protocol.h | 21 +++++++++++++++++++++ > 3 files changed, 22 insertions(+), 21 deletions(-) > > diff --git a/cache.h b/cache.h > index 2f21704da9e..71e2fe74c4f 100644 > --- a/cache.h > +++ b/cache.h > @@ -39,27 +39,6 @@ > #define S_DIFFTREE_IFXMIN_NEQ	0x80000000 >  >  > -/* > - * Intensive research over the course of many years has shown that > - * port 9418 is totally unused by anything else. Or > - * > - *	Your search - "port 9418" - did not match any documents. > - * > - * as www.google.com puts it. > - * > - * This port has been properly assigned for git use by IANA: > - * git (Assigned-9418) [I06-050728-0001]. > - * > - *	git 9418/tcp git pack transfer service > - *	git 9418/udp git pack transfer service > - * > - * with Linus Torvalds <torvalds@osdl.org> as the point of > - * contact. September 2005. > - * > - * See http://www.iana.org/assignments/port-numbers > - */ > -#define DEFAULT_GIT_PORT 9418 > - > /* > * Basic data structures for the directory cache > */ > diff --git a/daemon.c b/daemon.c > index db8a31a6ea2..75c3c064574 100644 > --- a/daemon.c > +++ b/daemon.c > @@ -4,6 +4,7 @@ > #include "config.h" > #include "environment.h" > #include "pkt-line.h" > +#include "protocol.h" > #include "run-command.h" > #include "setup.h" > #include "strbuf.h" > diff --git a/protocol.h b/protocol.h > index cef1a4a01c7..de66bf80f84 100644 > --- a/protocol.h > +++ b/protocol.h > @@ -1,6 +1,27 @@ > #ifndef PROTOCOL_H > #define PROTOCOL_H >  > +/* > + * Intensive research over the course of many years has shown that > + * port 9418 is totally unused by anything else. Or > + * > + *	Your search - "port 9418" - did not match any documents. > + * > + * as www.google.com puts it. > + * > + * This port has been properly assigned for git use by IANA: > + * git (Assigned-9418) [I06-050728-0001]. > + * > + *	git 9418/tcp git pack transfer service > + *	git 9418/udp git pack transfer service > + * > + * with Linus Torvalds <torvalds@osdl.org> as the point of > + * contact. September 2005. > + * > + * See http://www.iana.org/assignments/port-numbers > + */ > +#define DEFAULT_GIT_PORT 9418 > + > enum protocol_version { >	protocol_unknown_version = -1, >	protocol_v0 = 0,

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff King <peff@peff.net> writes: > The preprocessor doesn't know that we'll be confused if "s" isn't > resolved, and by the time the compiler sees it, it's a string already. > > Obviously we could add a test that catches this at run-time, but we > should be able to do better (catch it earlier, and with less code). Ah, great minds think alike. I am glad you have already thought it through. > My first thought was: why can't we just treat the port as an "int" in > the first place? The answer is mostly that getaddrinfo() expects it as a > string. It could even be a non-numeric service like "http" in theory > (and looked up in /etc/services; Debian's even has "git" in it!), but > our get_host_and_port() refuses to allow that. But even if we didn't > want to ever support non-numeric service names, it makes the code more > awkward (we have to format the port into an extra buffer). > > This would work: > > diff --git a/connect.c b/connect.c > index fd3179e545..1eba71e34c 100644 > --- a/connect.c > +++ b/connect.c > @@ -753,7 +753,7 @@ static char *host_end(char **hoststart, int removebrackets) > } >  > #define STR_(s)	# s > -#define STR(s)	STR_(s) > +#define STR(s) (STR_(s) + BUILD_ASSERT_OR_ZERO(s)) OOoooh. Clever. A pointer plus N indexes into an array, but if the offset is N then the pointer is left intact so the caller does not see the difference. > ... But the > BUILD_ASSERT doesn't seem too bad to me. Indeed.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes: >> #define STR_(s)	# s >> -#define STR(s)	STR_(s) >> +#define STR(s) (STR_(s) + BUILD_ASSERT_OR_ZERO(s)) > > OOoooh. Clever. A pointer plus N indexes into an array, but if the > offset is N then the pointer is left intact so the caller does not Sorry, but s/N/0/ obviously. > see the difference. > >> ... But the >> BUILD_ASSERT doesn't seem too bad to me. > > Indeed.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Mon, Apr 17, 2023 at 9:29 AM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > >> 5579f44d2f ("treewide: remove unnecessary cache.h inclusion", 2023-04-11) > >> broke connections via git protocol because it removed the inclusion of > >> the default port macro. While some may consider this transport to be > >> deprecated, it still serves some purpose. > > > > In particular the problem is that > > > > const char *port = STR(DEFAULT_GIT_PORT); > > > > translates now to > > > > const char *port = "DEFAULT_GIT_PORT"; > > > > instead of > > > > const char *port = "9418"; > > Wow, that is a bad one. If people do want "DEFAULT_GIT_PORT", they > would never write STR(DEFAULT_GIT_PORT). I wonder if we can have a > bit more clever STR() macro that catches this kind of mistake at the > compile time. > > If this is worth fixing, the fix would probably be worth protecting > with a test or two, but the networking test with fixed port is not > something we can easily do without a sealed environment, so... > > Thanks Michael for catching this. Yup. > > I've got a patch that does precisely this that I just submitted as > > part of my follow-on to the en/header-split-cache-h series. I've included > > that patch below in case Junio wants to advance it faster than the rest of > > that series. > > Yeah, burying it in a 24-patch series is a bit unfortunate. I didn't know it was a fix for anything when I wrote it; it was in the 24-patch series just as a further refactoring. Then I found out after this report and doing a little digging I found it might be considered a good fix for the issue so I included it here too. If you apply this patch directly on this series, I'm happy to drop it from the other series or do whatever makes things easiest.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Mon, Apr 17, 2023 at 12:38 AM Jeff King <peff@peff.net> wrote: > > On Sat, Apr 15, 2023 at 10:47:35PM -0700, Elijah Newren wrote: > > > On Sat, Apr 15, 2023 at 4:06 AM Michael J Gruber <git@grubix.eu> wrote: > > > > > > 5579f44d2f ("treewide: remove unnecessary cache.h inclusion", 2023-04-11) > > > broke connections via git protocol because it removed the inclusion of > > > the default port macro. While some may consider this transport to be > > > deprecated, it still serves some purpose. > > > > In particular the problem is that > > > > const char *port = STR(DEFAULT_GIT_PORT); > > > > translates now to > > > > const char *port = "DEFAULT_GIT_PORT"; > > > > instead of > > > > const char *port = "9418"; > > > > Since both compile and nothing in the testsuite tests this, I just > > missed this problem when making the other changes. > > Your fix looks obviously correct, but the much more interesting thing to > me is how surprising it is that neither the compiler nor tests caught > it. The tests don't catch it because we never use the default port for > our daemon tests, since we don't want two scripts running in parallel to > conflict. And your example above shows what the compiler sees, but root > issue is this funky string-ification macro: > > #define STR_(s) # s > #define STR(s) STR_(s) > > The preprocessor doesn't know that we'll be confused if "s" isn't > resolved, and by the time the compiler sees it, it's a string already. > > Obviously we could add a test that catches this at run-time, but we > should be able to do better (catch it earlier, and with less code). > > My first thought was: why can't we just treat the port as an "int" in > the first place? The answer is mostly that getaddrinfo() expects it as a > string. It could even be a non-numeric service like "http" in theory > (and looked up in /etc/services; Debian's even has "git" in it!), but > our get_host_and_port() refuses to allow that. But even if we didn't > want to ever support non-numeric service names, it makes the code more > awkward (we have to format the port into an extra buffer). > > This would work: > > diff --git a/connect.c b/connect.c > index fd3179e545..1eba71e34c 100644 > --- a/connect.c > +++ b/connect.c > @@ -753,7 +753,7 @@ static char *host_end(char **hoststart, int removebrackets) > } > > #define STR_(s) # s > -#define STR(s) STR_(s) > +#define STR(s) (STR_(s) + BUILD_ASSERT_OR_ZERO(s)) > > static void get_host_and_port(char **host, const char **port) > { > > The error message is a bit verbose, but it starts with: > > connect.c: In function ‘git_tcp_connect_sock’: > connect.c:801:32: error: ‘DEFAULT_GIT_PORT’ undeclared (first use in this function) > 801 | const char *port = STR(DEFAULT_GIT_PORT); > | ^~~~~~~~~~~~~~~~ > > which seems OK in practice. Seems pretty good to me. > Another alternative is to just declare this STR() thing too clever, and > put: > > #define DEFAULT_GIT_PORT_STR "9418" > > next to the int declaration. It's not like its going to change. But the > BUILD_ASSERT doesn't seem too bad to me. Yeah, I like the BUILD_ASSERT.

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Mon, Apr 17, 2023 at 09:33:57AM -0700, Junio C Hamano wrote: > > diff --git a/connect.c b/connect.c > > index fd3179e545..1eba71e34c 100644 > > --- a/connect.c > > +++ b/connect.c > > @@ -753,7 +753,7 @@ static char *host_end(char **hoststart, int removebrackets) > > } > >  > > #define STR_(s)	# s > > -#define STR(s)	STR_(s) > > +#define STR(s) (STR_(s) + BUILD_ASSERT_OR_ZERO(s)) >  > OOoooh. Clever. A pointer plus N indexes into an array, but if the > offset is N then the pointer is left intact so the caller does not > see the difference. >  > > ... But the > > BUILD_ASSERT doesn't seem too bad to me. >  > Indeed. So I started to write this up as a patch, but there's another subtle thing going on. The BUILD_ASSERT is actually checking two things: that the result compiles (which is what we care about here), and that the expression it evaluates is nonzero (which we don't). So this would fail for example with: #define ZERO 0 const char *x = STR(ZERO); That is OK for our purposes here (a zero port does not make any sense). But it feels a bit weird for a macro as generically named as STR(). At least it's local to the one file. But maybe it should be PORT_TO_STR() or something. All of that makes me wonder if we wouldn't be just as happy with it as a string in the first place. In three out of four locations that use it, they want the string anyway (to feed to getaddrinfo). And in the final one (git-daemon), we need to convert from the user's "--port" anyway, so there's always some string-to-int parsing. And depending on the #ifdefs, in most cases we turn it back into a string anyway to feed to...you guessed it, getaddrinfo! The exception is when NO_IPV6 is defined, in which we do want the numeric value. But we could delay parsing until that point (and otherwise let getaddrinfo handle, which seems more correct anyway). Something like this (though I'd probably split it into a few patches to reason about the motivation and implications of each): diff --git a/cache.h b/cache.h index 2f21704da9..2ece09a2b8 100644 --- a/cache.h +++ b/cache.h @@ -58,7 +58,7 @@ * * See http://www.iana.org/assignments/port-numbers */ -#define DEFAULT_GIT_PORT 9418 +#define DEFAULT_GIT_PORT "9418" /* * Basic data structures for the directory cache diff --git a/connect.c b/connect.c index fd3179e545..189367604c 100644 --- a/connect.c +++ b/connect.c @@ -1,4 +1,5 @@ #include "git-compat-util.h" +#include "cache.h" /* or protocol.h after Elijah's patch */ #include "config.h" #include "environment.h" #include "gettext.h" @@ -752,9 +753,6 @@ static char *host_end(char **hoststart, int removebrackets)	return end; } -#define STR_(s)	# s -#define STR(s)	STR_(s) - static void get_host_and_port(char **host, const char **port) {	char *colon, *end; @@ -798,7 +796,7 @@ static int git_tcp_connect_sock(char *host, int flags) {	struct strbuf error_message = STRBUF_INIT;	int sockfd = -1; -	const char *port = STR(DEFAULT_GIT_PORT); +	const char *port = DEFAULT_GIT_PORT;	struct addrinfo hints, *ai0, *ai;	int gai;	int cnt = 0; @@ -868,7 +866,7 @@ static int git_tcp_connect_sock(char *host, int flags) {	struct strbuf error_message = STRBUF_INIT;	int sockfd = -1; -	const char *port = STR(DEFAULT_GIT_PORT); +	const char *port = DEFAULT_GIT_PORT;	char *ep;	struct hostent *he;	struct sockaddr_in sa; @@ -1020,7 +1018,7 @@ static int git_use_proxy(const char *host) static struct child_process *git_proxy_connect(int fd[2], char *host) { -	const char *port = STR(DEFAULT_GIT_PORT); +	const char *port = DEFAULT_GIT_PORT;	struct child_process *proxy;	get_host_and_port(&host, &port); diff --git a/daemon.c b/daemon.c index db8a31a6ea..ce692bad35 100644 --- a/daemon.c +++ b/daemon.c @@ -984,22 +984,20 @@ static const char *ip2str(int family, struct sockaddr *sin, socklen_t len) #ifndef NO_IPV6 -static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist) +static int setup_named_sock(char *listen_addr, const char *listen_port, struct socketlist *socklist) {	int socknum = 0; -	char pbuf[NI_MAXSERV];	struct addrinfo hints, *ai0, *ai;	int gai;	long flags; -	xsnprintf(pbuf, sizeof(pbuf), "%d", listen_port);	memset(&hints, 0, sizeof(hints));	hints.ai_family = AF_UNSPEC;	hints.ai_socktype = SOCK_STREAM;	hints.ai_protocol = IPPROTO_TCP;	hints.ai_flags = AI_PASSIVE; -	gai = getaddrinfo(listen_addr, pbuf, &hints, &ai0); +	gai = getaddrinfo(listen_addr, listen_port, &hints, &ai0);	if (gai) {	logerror("getaddrinfo() for %s failed: %s", listen_addr, gai_strerror(gai));	return 0; @@ -1065,15 +1063,27 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis #else /* NO_IPV6 */ -static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist) +static int parse_port(const char *s) +{ +	unsigned long ret; +	char *end; + +	ret = strtoul(s, &end, 0); +	if (!ret || ret > 65535 || *end) +	die(_("invalid listen port: %s"), s); + +	return ret; +} + +static int setup_named_sock(char *listen_addr, const char *listen_port, struct socketlist *socklist) {	struct sockaddr_in sin;	int sockfd;	long flags;	memset(&sin, 0, sizeof sin);	sin.sin_family = AF_INET; -	sin.sin_port = htons(listen_port); +	sin.sin_port = parse_port(listen_port);	if (listen_addr) {	/* Well, host better be an IP address here. */ @@ -1122,7 +1132,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis #endif -static void socksetup(struct string_list *listen_addr, int listen_port, struct socketlist *socklist) +static void socksetup(struct string_list *listen_addr, const char *listen_port, struct socketlist *socklist) {	if (!listen_addr->nr)	setup_named_sock(NULL, listen_port, socklist); @@ -1133,7 +1143,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s listen_port, socklist);	if (socknum == 0) -	logerror("unable to allocate any listen sockets for host %s on port %u", +	logerror("unable to allocate any listen sockets for host %s on port %s", listen_addr->items[i].string, listen_port);	}	} @@ -1246,14 +1256,14 @@ static struct credentials *prepare_credentials(const char *user_name, } #endif -static int serve(struct string_list *listen_addr, int listen_port, +static int serve(struct string_list *listen_addr, const char *listen_port, struct credentials *cred) {	struct socketlist socklist = { NULL, 0, 0 };	socksetup(listen_addr, listen_port, &socklist);	if (socklist.nr == 0) -	die("unable to allocate any listen sockets on port %u", +	die("unable to allocate any listen sockets on port %s", listen_port);	drop_privileges(cred); @@ -1265,7 +1275,7 @@ static int serve(struct string_list *listen_addr, int listen_port, int cmd_main(int argc, const char **argv) { -	int listen_port = 0; +	const char *listen_port = NULL;	struct string_list listen_addr = STRING_LIST_INIT_NODUP;	int serve_mode = 0, inetd_mode = 0;	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL; @@ -1282,13 +1292,8 @@ int cmd_main(int argc, const char **argv)	continue;	}	if (skip_prefix(arg, "--port=", &v)) { -	char *end; -	unsigned long n; -	n = strtoul(v, &end, 0); -	if (*v && !*end) { -	listen_port = n; -	continue; -	} +	listen_port = v; +	continue;	}	if (!strcmp(arg, "--serve")) {	serve_mode = 1; @@ -1439,7 +1444,7 @@ int cmd_main(int argc, const char **argv)	if (inetd_mode && (listen_port || (listen_addr.nr > 0)))	die("--listen= and --port= are incompatible with --inetd"); -	else if (listen_port == 0) +	else if (!listen_port)	listen_port = DEFAULT_GIT_PORT;	if (group_name && !user_name)

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Mon, Apr 17, 2023 at 06:56:42PM -0700, Elijah Newren wrote: > > The error message is a bit verbose, but it starts with: > > > > connect.c: In function ‘git_tcp_connect_sock’: > > connect.c:801:32: error: ‘DEFAULT_GIT_PORT’ undeclared (first use in this function) > > 801 | const char *port = STR(DEFAULT_GIT_PORT); > > | ^~~~~~~~~~~~~~~~ > > > > which seems OK in practice. >  > Seems pretty good to me. The rest of it is less nice: connect.c: In function ‘git_tcp_connect_sock’: connect.c:801:32: error: ‘DEFAULT_GIT_PORT’ undeclared (first use in this function) 801 | const char *port = STR(DEFAULT_GIT_PORT); | ^~~~~~~~~~~~~~~~ git-compat-util.h:93:31: note: in definition of macro ‘BUILD_ASSERT_OR_ZERO’ 93 | (sizeof(char [1 - 2*!(cond)]) - 1) | ^~~~ connect.c:801:28: note: in expansion of macro ‘STR’ 801 | const char *port = STR(DEFAULT_GIT_PORT); | ^~~ connect.c:801:32: note: each undeclared identifier is reported only once for each function it appears in 801 | const char *port = STR(DEFAULT_GIT_PORT); | ^~~~~~~~~~~~~~~~ git-compat-util.h:93:31: note: in definition of macro ‘BUILD_ASSERT_OR_ZERO’ 93 | (sizeof(char [1 - 2*!(cond)]) - 1) | ^~~~ connect.c:801:28: note: in expansion of macro ‘STR’ 801 | const char *port = STR(DEFAULT_GIT_PORT); | ^~~ but that's kind of the nature of BUILD_ASSERT (it's even worse if the code compiles but the assertion is false, since the first thing is that funky sizeof() line). I guess it's the best we can do. The point is that these aren't supposed to happen very often. -Peff

[gitgitgadget]

This patch series was integrated into seen via git@ef0957b.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/header-split-cache-h on the Git mailing list:

Header clean-up. Will merge to 'master'. source: <pull.1509.v3.git.1681182060.gitgitgadget@gmail.com> 

[gitgitgadget]

This patch series was integrated into seen via git@656fc68.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff King <peff@peff.net> writes: > All of that makes me wonder if we wouldn't be just as happy with it as a > string in the first place. In three out of four locations that use it, > they want the string anyway (to feed to getaddrinfo). And in the final > one (git-daemon), we need to convert from the user's "--port" anyway, so > there's always some string-to-int parsing. And depending on the #ifdefs, > in most cases we turn it back into a string anyway to feed to...you > guessed it, getaddrinfo! > > The exception is when NO_IPV6 is defined, in which we do want the > numeric value. But we could delay parsing until that point (and > otherwise let getaddrinfo handle, which seems more correct anyway). > > Something like this (though I'd probably split it into a few patches to > reason about the motivation and implications of each): The updated code keeps the "port" as a string and turns it into a short integer only when it is needed, which is nice. In the future, parse_port() may even want to be extended to call something like getservbyname(), to allow something silly like	#define DEFAULT_GIT_PORT "git" 

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <newren@gmail.com> writes: > I didn't know it was a fix for anything when I wrote it; it was in the > 24-patch series just as a further refactoring. Then I found out after > this report and doing a little digging I found it might be considered > a good fix for the issue so I included it here too. Yup, let's queue it at the tip of (and as a part of) the base series with a bit of explanation. How does this look? ----- >8 --------- >8 --------- >8 --------- >8 ----- From: Elijah Newren <newren@gmail.com> Date: Sun, 16 Apr 2023 03:03:05 +0000 Subject: [PATCH] protocol.h: move definition of DEFAULT_GIT_PORT from cache.h Michael J Gruber noticed that connection via the git:// protocol no longer worked after a recent header clean-up. This was caused by funny interaction of few gotchas. First, a necessary definition	#define DEFAULT_GIT_PORT 9418 was made invisible to a place where	const char *port = STR(DEFAULT_GIT_PORT); was expecting to turn the integer into "9418" with a clever STR() macro, and ended up stringifying it to	const char *port = "DEFAULT_GIT_PORT"; without giving any chance to compilers to notice such a mistake. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- cache.h | 21 --------------------- daemon.c | 1 + protocol.h | 21 +++++++++++++++++++++ 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/cache.h b/cache.h index 2f21704da9..71e2fe74c4 100644 --- a/cache.h +++ b/cache.h @@ -39,27 +39,6 @@ #define S_DIFFTREE_IFXMIN_NEQ	0x80000000 -/* - * Intensive research over the course of many years has shown that - * port 9418 is totally unused by anything else. Or - * - *	Your search - "port 9418" - did not match any documents. - * - * as www.google.com puts it. - * - * This port has been properly assigned for git use by IANA: - * git (Assigned-9418) [I06-050728-0001]. - * - *	git 9418/tcp git pack transfer service - *	git 9418/udp git pack transfer service - * - * with Linus Torvalds <torvalds@osdl.org> as the point of - * contact. September 2005. - * - * See http://www.iana.org/assignments/port-numbers - */ -#define DEFAULT_GIT_PORT 9418 - /* * Basic data structures for the directory cache */ diff --git a/daemon.c b/daemon.c index db8a31a6ea..75c3c06457 100644 --- a/daemon.c +++ b/daemon.c @@ -4,6 +4,7 @@ #include "config.h" #include "environment.h" #include "pkt-line.h" +#include "protocol.h" #include "run-command.h" #include "setup.h" #include "strbuf.h" diff --git a/protocol.h b/protocol.h index cef1a4a01c..de66bf80f8 100644 --- a/protocol.h +++ b/protocol.h @@ -1,6 +1,27 @@ #ifndef PROTOCOL_H #define PROTOCOL_H +/* + * Intensive research over the course of many years has shown that + * port 9418 is totally unused by anything else. Or + * + *	Your search - "port 9418" - did not match any documents. + * + * as www.google.com puts it. + * + * This port has been properly assigned for git use by IANA: + * git (Assigned-9418) [I06-050728-0001]. + * + *	git 9418/tcp git pack transfer service + *	git 9418/udp git pack transfer service + * + * with Linus Torvalds <torvalds@osdl.org> as the point of + * contact. September 2005. + * + * See http://www.iana.org/assignments/port-numbers + */ +#define DEFAULT_GIT_PORT 9418 + enum protocol_version {	protocol_unknown_version = -1,	protocol_v0 = 0, --  2.40.0-352-g667fcf4e15 

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Tue, Apr 18, 2023 at 5:06 PM Junio C Hamano <gitster@pobox.com> wrote: > From: Elijah Newren <newren@gmail.com> > Subject: [PATCH] protocol.h: move definition of DEFAULT_GIT_PORT from cache.h > > Michael J Gruber noticed that connection via the git:// protocol no > longer worked after a recent header clean-up. A link to Michael's email might be useful for future readers of this commit message. Michale J Gruber noticed[1] that connection... [1]: https://lore.kernel.org/git/5d4e0ce10f537b4bb795a70dd51db12ecaf0206d.1681556597.git.git@grubix.eu/ > This was caused by > funny interaction of few gotchas. First, a necessary definition > > #define DEFAULT_GIT_PORT 9418 > > was made invisible to a place where > > const char *port = STR(DEFAULT_GIT_PORT); > > was expecting to turn the integer into "9418" with a clever STR() > macro, and ended up stringifying it to > > const char *port = "DEFAULT_GIT_PORT"; > > without giving any chance to compilers to notice such a mistake. > > Signed-off-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> Perhaps an additional tailer would be appropriate? Reported-by: Michael J Gruber <git@grubix.eu>

[gitgitgadget]

User Eric Sunshine <sunshine@sunshineco.com> has been added to the cc: list.

[gitgitgadget]

This patch series was integrated into seen via git@06c61bf.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Apr 18, 2023 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > I didn't know it was a fix for anything when I wrote it; it was in the > > 24-patch series just as a further refactoring. Then I found out after > > this report and doing a little digging I found it might be considered > > a good fix for the issue so I included it here too. > > Yup, let's queue it at the tip of (and as a part of) the base series > with a bit of explanation. How does this look? > > ----- >8 --------- >8 --------- >8 --------- >8 ----- > From: Elijah Newren <newren@gmail.com> > Date: Sun, 16 Apr 2023 03:03:05 +0000 > Subject: [PATCH] protocol.h: move definition of DEFAULT_GIT_PORT from cache.h > > Michael J Gruber noticed that connection via the git:// protocol no > longer worked after a recent header clean-up. This was caused by > funny interaction of few gotchas. First, a necessary definition > > #define DEFAULT_GIT_PORT 9418 > > was made invisible to a place where > > const char *port = STR(DEFAULT_GIT_PORT); > > was expecting to turn the integer into "9418" with a clever STR() > macro, and ended up stringifying it to > > const char *port = "DEFAULT_GIT_PORT"; > > without giving any chance to compilers to notice such a mistake. > > Signed-off-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> Looks great! > --- > cache.h | 21 --------------------- > daemon.c | 1 + > protocol.h | 21 +++++++++++++++++++++ > 3 files changed, 22 insertions(+), 21 deletions(-) > > diff --git a/cache.h b/cache.h > index 2f21704da9..71e2fe74c4 100644 > --- a/cache.h > +++ b/cache.h > @@ -39,27 +39,6 @@ > #define S_DIFFTREE_IFXMIN_NEQ 0x80000000 > > > -/* > - * Intensive research over the course of many years has shown that > - * port 9418 is totally unused by anything else. Or > - * > - * Your search - "port 9418" - did not match any documents. > - * > - * as www.google.com puts it. > - * > - * This port has been properly assigned for git use by IANA: > - * git (Assigned-9418) [I06-050728-0001]. > - * > - * git 9418/tcp git pack transfer service > - * git 9418/udp git pack transfer service > - * > - * with Linus Torvalds <torvalds@osdl.org> as the point of > - * contact. September 2005. > - * > - * See http://www.iana.org/assignments/port-numbers > - */ > -#define DEFAULT_GIT_PORT 9418 > - > /* > * Basic data structures for the directory cache > */ > diff --git a/daemon.c b/daemon.c > index db8a31a6ea..75c3c06457 100644 > --- a/daemon.c > +++ b/daemon.c > @@ -4,6 +4,7 @@ > #include "config.h" > #include "environment.h" > #include "pkt-line.h" > +#include "protocol.h" > #include "run-command.h" > #include "setup.h" > #include "strbuf.h" > diff --git a/protocol.h b/protocol.h > index cef1a4a01c..de66bf80f8 100644 > --- a/protocol.h > +++ b/protocol.h > @@ -1,6 +1,27 @@ > #ifndef PROTOCOL_H > #define PROTOCOL_H > > +/* > + * Intensive research over the course of many years has shown that > + * port 9418 is totally unused by anything else. Or > + * > + * Your search - "port 9418" - did not match any documents. > + * > + * as www.google.com puts it. > + * > + * This port has been properly assigned for git use by IANA: > + * git (Assigned-9418) [I06-050728-0001]. > + * > + * git 9418/tcp git pack transfer service > + * git 9418/udp git pack transfer service > + * > + * with Linus Torvalds <torvalds@osdl.org> as the point of > + * contact. September 2005. > + * > + * See http://www.iana.org/assignments/port-numbers > + */ > +#define DEFAULT_GIT_PORT 9418 > + > enum protocol_version { > protocol_unknown_version = -1, > protocol_v0 = 0, > -- > 2.40.0-352-g667fcf4e15 > > >

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Apr 18, 2023 at 2:25 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Tue, Apr 18, 2023 at 5:06 PM Junio C Hamano <gitster@pobox.com> wrote: > > From: Elijah Newren <newren@gmail.com> > > Subject: [PATCH] protocol.h: move definition of DEFAULT_GIT_PORT from cache.h > > > > Michael J Gruber noticed that connection via the git:// protocol no > > longer worked after a recent header clean-up. > > A link to Michael's email might be useful for future readers of this > commit message. > > Michale J Gruber noticed[1] that connection... > > [1]: https://lore.kernel.org/git/5d4e0ce10f537b4bb795a70dd51db12ecaf0206d.1681556597.git.git@grubix.eu/ > > > This was caused by > > funny interaction of few gotchas. First, a necessary definition > > > > #define DEFAULT_GIT_PORT 9418 > > > > was made invisible to a place where > > > > const char *port = STR(DEFAULT_GIT_PORT); > > > > was expecting to turn the integer into "9418" with a clever STR() > > macro, and ended up stringifying it to > > > > const char *port = "DEFAULT_GIT_PORT"; > > > > without giving any chance to compilers to notice such a mistake. > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > > Perhaps an additional tailer would be appropriate? > > Reported-by: Michael J Gruber <git@grubix.eu> These both look like good additions too.

[gitgitgadget]

This patch series was integrated into seen via git@061e8ac.

[gitgitgadget]

This patch series was integrated into seen via git@3b80e30.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/header-split-cache-h on the Git mailing list:

Header clean-up. Will merge to 'master'. source: <pull.1509.v3.git.1681182060.gitgitgadget@gmail.com> 

[gitgitgadget]

This patch series was integrated into seen via git@3e168d6.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> writes: > A link to Michael's email might be useful for future readers of this > commit message. > > Michale J Gruber noticed[1] that connection... I don't think so. At least, I tried to write the proposed log message in such a way that it is not needed. I encourage people not to add URLs just so that they can be lazy and omit summarizing the discussion for readers so that they do not have to refer to external material in order to understand "git log" output. And I was trying to show by example. Was there anything you couldn't read out of what I wrote that is necessary to understand the issue the commit tried to address without reading the original discussion?

[gitgitgadget]

This patch series was integrated into seen via git@f5a1149.

[gitgitgadget]

This patch series was integrated into seen via git@2f83968.

[gitgitgadget]

This patch series was integrated into seen via git@0807e57.

[gitgitgadget]

This patch series was integrated into master via git@0807e57.

[gitgitgadget]

Closed via 0807e57.