On Fri, Mar 29, 2024 at 11:36:33AM -0700, Junio C Hamano wrote:
> "Justin Tobler via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Justin Tobler <jltobler@xxxxxxxxx>
> >
> > For a reftable segment, the start of the range is inclusive and the end
> > is exclusive. In practice we increment the end when creating the
> > compaction segment only to decrement the segment end when using it.
> >
> > Simplify by making the segment end inclusive. The corresponding test,
> > `test_suggest_compaction_segment()`, is updated to show that the segment
> > end is now inclusive.
> >
> > Signed-off-by: Justin Tobler <jltobler@xxxxxxxxx>
> > ---
> > reftable/stack.c | 4 ++--
> > reftable/stack_test.c | 2 +-
> > 2 files changed, 3 insertions(+), 3 deletions(-)
>
> I'd defer it to Patrick (and Han-Wen, if he wants to comment on it),
> but isn't it a natural expectation shared among CS folks that it is
> the most usual way to express a range to use inclusive lower-end and
> exclusive upper-end?
>
> After all, that is how an array works, i.e. msg[n] is NULL and
> beyond the end where n == strlen(msg).
>
> So, I dunno.
I don't really have a strong opinion here, to be honest. I think the
previous way to handle this was fine, the new way is okay, too. Which
may indicate that we can just drop this patch to avoid needless churn
unless somebody feels strongly about this.
Patrick
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index e7b9a1de5a4..0973c47dd92 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -1237,7 +1237,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n)
> > */
> > for (i = n - 1; i > 0; i--) {
> > if (sizes[i - 1] < sizes[i] * 2) {
> > - seg.end = i + 1;
> > + seg.end = i;
> > bytes = sizes[i];
> > break;
> > }
>
>
>
> > @@ -1291,7 +1291,7 @@ int reftable_stack_auto_compact(struct reftable_stack *st)
> > suggest_compaction_segment(sizes, st->merged->stack_len);
> > reftable_free(sizes);
> > if (segment_size(&seg) > 0)
> > - return stack_compact_range_stats(st, seg.start, seg.end - 1,
> > + return stack_compact_range_stats(st, seg.start, seg.end,
> > NULL);
> >
> > return 0;
> > diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> > index 21541742fe5..4d7305623a0 100644
> > --- a/reftable/stack_test.c
> > +++ b/reftable/stack_test.c
> > @@ -723,7 +723,7 @@ static void test_suggest_compaction_segment(void)
> > struct segment min =
> > suggest_compaction_segment(sizes, ARRAY_SIZE(sizes));
> > EXPECT(min.start == 1);
> > - EXPECT(min.end == 10);
> > + EXPECT(min.end == 9);
> > }
> >
> > static void test_suggest_compaction_segment_nothing(void)
Attachment:
signature.asc
Description: PGP signature
