On 2025-06-06 14:05:37 [-0700], Junio C Hamano wrote:
> > This builds on top of v2.50.0-rc1 on s390x and -rc0 and x86-64. The
> > testsuite passes.
>
> What's missing from the above proposed log message is what problem
> this patch is trying to address. "The testsuite passes" may refer
> to the state with this patch, but the message does not talk about
> the state without the patch. We'd prefer to see the log message
> begin more like so:
>
> Since 6547d1c9 (bswap.h: add support for built-in bswap
> functions, 2025-04-23) tweaked the way the bswap32/64 macros are
> defined, on platforms with __builtin_bswap32/64 supported, the
> bswap32/64 macros are defined even on big endian platforms.
>
> However this file assumes that bswap32/64 are defined ONLY on
> little endian machines and uses that assumption to redefine
> ntohl/ntohll macros. The said commit broke t1234-be.sh test
> gets broken, among many others on s390x.
>
> Especially pay close attention to how we name the commit in prose,
> not as "the cited commit" (because we do not 'cite' them and frown
> upon Fixes: trailer on this project).
Okay.
> Also, I do not know what tests were broken and on what platforms for
> you that triggered you to do this patch, so take the second paragraph
> above as all made up example that only illustrates the level of
> detail expected in a proposed log message.
I run into this while debugging t4014-format-patch.sh due to another
issue.
> After the "observation of the current state of the problematic code"
> is given, we'd describe the solution.
>
> Make sure that we detect byte order of the platform first and
> override ntohll only on little endian platfgorms with bswap64
> by moving things around.
>
> or something, perhaps.
Okay.
> > #endif
>
> It is a bit hard to see as the original does not indent the
> #directives consistently, but this "#endif" closes the
> #if..#elif..#endif to define bswap32/bswap64 for some platforms. We
> are only inside the top-level "#ifdef COMPAT_BSWAP_H" at this point,
> so ...
>
> > -#if defined(bswap32)
> > -
> > -#undef ntohl
> > -#undef htonl
> > -#define ntohl(x) bswap32(x)
> > -#define htonl(x) bswap32(x)
> > -
> > -#endif
> > -
> > -#if defined(bswap64)
> > -
> > -#undef ntohll
> > -#undef htonll
> > -#define ntohll(x) bswap64(x)
> > -#define htonll(x) bswap64(x)
> > -
> > -#else
>
> ... we undefine these two macros for _everybody_ here. Also let me
> take a mental note that we only undef these 64-bit functions and
> leave ntohl/htonl intact.
How so? The ntohl/ htonl are also replaced with bswap32 Or do I miss
something.
> > #undef ntohll
> > #undef htonll
>
> This is related to the "oddity" I'll mention at the end.
>
> After this part, there is a #if..#elif..#endif cascade to ensure
> GIT_BYTE_ORDER is defined, which is unchanged and not shown in the
> context.
>
> > @@ -151,10 +133,23 @@ static inline uint64_t git_bswap64(uint64_t x)
> > # define ntohll(n) (n)
> > # define htonll(n) (n)
> > #else
> > -# define ntohll(n) default_bswap64(n)
> > -# define htonll(n) default_bswap64(n)
> > -#endif
>
> "#if GIT_BYTE_ORDER == GIT_BIGENDIAN" is before the pre-context of
> this hunk. We are extending the else clause (i.e. little endian
> support) with the following:
>
> > +# if defined(bswap32)
> > +# undef ntohl
> > +# undef htonl
> > +# define ntohl(x) bswap32(x)
> > +# define htonl(x) bswap32(x)
> > +# endif
> > +
> > +# if defined(bswap64)
> > +# undef ntohll
> > +# undef htonll
> > +# define ntohll(x) bswap64(x)
> > +# define htonll(x) bswap64(x)
> > +# else
> > +# define ntohll(n) default_bswap64(n)
> > +# define htonll(n) default_bswap64(n)
> > +# endif
> > #endif
>
> I think the patch is an improvement from the current state, but the
> resulting code is still somewhat odd in that ntohll() and htonll()
> are overridden for everybody (even for big endian boxes we make sure
> it is identity function), but we override ntohl() and htonl() only
> on platforms where bswap32 is defined.
Ah, the ntohll/ htonll gets undef and defined later. That is the
"oddity" as you put it.
Do you want this reposted with an improved commit message or do you want
also the undef for ntohll and the identity define removed since it is
not required? I could add it as a follow-up not merge the fix and the
improvement in one patch but as you wish.
We migh also remove this file because it appears that ntohl() comes
already as the built-in:
| $ cat a.c
| #include <arpa/inet.h>
|
| unsigned int nto32(unsigned int x)
| {
| return ntohl(x);
| }
| $ gcc -o a.i -E a.c -O2
| $ grep -A4 nto32 a.i
| unsigned int nto32(unsigned int x)
| {
| return
| # 5 "a.c" 3 4
| __bswap_32 (
| $ grep -A3 "__bswap_32 (_" a.i
| __bswap_32 (__uint32_t __bsx)
| {
|
| return __builtin_bswap32 (__bsx);
This is from the glibc 2.41 header file. But then ntohll() is
non-standard and needs manual care.
> Thanks.
Sebastian
