Re: [PATCH v3] sparse-checkout: optimize string_list construction

From: Amisha Chhajed <amishhhaaaa@gmail.com>
Date: Jan 16, 2026, 17:03 UTC
Message-ID: <CAPvEtrc4KuQhNhc966=bbMQUZw1Ne1eoG68mVoZiG6A3h4t=GQ@mail.gmail.com>
In-Reply-To: <20260115200903.GB1053259@coredump.intra.peff.net>

I was able to reproduce this, are we open to a patch adding a test that checks if duplicate entries are present in stdin the result should not have it? because the tests were passing even after removing all duplicates checks, and non duplicates enforcement is a part of the method's behaviour, if I am understanding correctly.

On Fri, 16 Jan 2026 at 01:39, Jeff King <peff@peff.net> wrote:

Show 47 quoted lines

>
> On Thu, Jan 15, 2026 at 06:45:35PM +0530, Amisha Chhajed wrote:
>
> > I was also very curious about the presence of
> > string_list_remove_duplicates in the original code, from my
> > understanding string_list_insert already removed duplicates and
> > string_list_remove_duplicates was still present with it.
>
> Yes, I don't think you could have duplicates when inserting with
> string_list_insert(). Of course your patch removes that, which means
> we're falling back on the notion that the hashmap cannot have
> duplicates, either.
>
> I think our hashmap _does_ allow duplicate entries, though. The
> insertion code in insert_recursive_pattern() avoids duplicates in
> parent_hashmap, but adds its arguments directly to recursive_hashmap.
>
> So I think you could get duplicates with something like:
>
>   git init
>   git sparse-checkout set --cone
>   git sparse-checkout add --stdin <<\EOF
>   foo
>   bar
>   foo
>   EOF
>
> Before your patch, that produces this .git/info/sparse-checkout file:
>
>   /*
>   !/*/
>   /bar/
>   /foo/
>
> and after we get:
>
>   /*
>   !/*/
>   /bar/
>   /foo/
>   /foo/
>
> So I think we do want to retain the duplicate suppression. Switching
> from insert() to append() is still good, as long as we keep the
> remove_duplicates() lines.
>
> -Peff