Re: [PATCH v2] commit-graph: fix writing generations with dates exceeding 34 bits

[Patrick Steinhardt <ps@pks.im>]

On Tue, Mar 24, 2026 at 08:42:31AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Changes in v2:
> >   - Account for platforms where `timestamp_t` has 32 bit precision. This
> >     matches logic in `write_graph_chunk_data()`, where we also depend on
> >     the size of the commit timestamps.
> 
> > +static timestamp_t compute_generation_offset(struct commit *c)
> > +{
> > +	timestamp_t masked_date;
> > +
> > +	if (sizeof(timestamp_t) > 4)
> > +		masked_date = c->date & (((timestamp_t) 1 << 34) - 1);
> > +	else
> > +		masked_date = c->date;
> 
> It is a bit surprising that on a platform where timestamp_t is only
> 32-bit wide, a smart-enough compiler would not find (1<<34) as
> suspicious. 

It probably would, but we don't test on any system where that's the case
to the best of my knowledge. `timestamp_t` is defined as `uintmax_t`,
and that should be 64 bit on almost all platforms. There's going to be
exceptions though, even though I'm not sure whether such platforms even
matter to us. But I later realized that we had similar checks elsewhere,
so I decided to adapt accordingly.

> IOW, I would have expected this to be done not with runtime switch but
> with conditional compilation.

Well, it's written as a runtime switch, but I would expect all compilers
to essentially turn this into conditional compilation. They should note
that the condition is always true (or false) on a given platform. And
checking Godbolt my assumption seems to hold. On x86-64 using GCC [1]:

    compute_generation_offset(unsigned long, unsigned long):
            movabs  rax, 17179869183
            and     rdi, rax
            mov     rax, rsi
            sub     rax, rdi
            ret

Thanks!

Patrick

[1]: https://godbolt.org/z/oa4xPohGb