Re: [PATCH v4 00/13] Add struct strmap and associated utility functions

From: Elijah Newren <newren@gmail.com>
Date: Nov 5, 2020, 21:22 UTC
Message-ID: <CABPp-BGFa_e-nncnCH5GSjhn47Sxf7Tpn7prsJCWiJo-SWoYNA@mail.gmail.com>
In-Reply-To: <xmqq8sbfk0ut.fsf@gitster.c.googlers.com>

On Thu, Nov 5, 2020 at 12:25 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

>
> Jeff King <peff@peff.net> writes:
>
> > Subject: [PATCH] shortlog: drop custom strset implementation
> >
> > We can use the strset recently added in strmap.h instead. Note that this
> > doesn't have a "check_and_add" function. We can easily write the same
> > thing using separate "contains" and "adds" calls. This is slightly less
> > efficient, in that it hashes the string twice, but for our use here it
> > shouldn't be a big deal either way.
> >
> > I did leave it as a separate helper function, though, since we use it in
> > three separate spots (some of which are in the middle of a conditional).
>
> It makes sense, but check_dup() sounds like its use pattern would be
>
>         if (check_dup(it) == NO_DUP)
>                 add(it);
>
> where it is more like "add it but just once".
>
> By the way, is a strset a set or a bag?  If it makes the second add

strset is a set; there is no way to get duplicate entries.

> an no-op, perhaps your check_dup() is what strset_add() should do
> itself?  What builtin/shortlog.c::check_dup() does smells like it is
> a workaround for the lack of a naturally-expected feature.

Is the expectation that strset_add() would return a boolean for whether a new entry was added?