On Mon, Apr 17, 2023 at 12:38 AM Jeff King <peff@xxxxxxxx> 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@xxxxxxxxx> 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.
