Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list

From: Jeff King <peff@peff.net>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: "Martin Ågren" <martin.agren@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>
Subject: Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list
Date: Tue, 5 Sep 2017 05:04:33 -0400	[thread overview]
Message-ID: <20170905090433.elysfemrgxhze2xn@sigill.intra.peff.net> (raw)
In-Reply-To: <CAMy9T_F5HWF=hinxdp0C+pgyH_8MnR31p=XK1rjyMiM3UwST_w@mail.gmail.com>

On Tue, Sep 05, 2017 at 11:03:36AM +0200, Michael Haggerty wrote:

> > It feels pretty dirty, though. It would certainly be a bug if we ever
> > decided to switch affected_refnames to duplicate its strings.
> >
> > So given that your solution is only a constant-time factor worse in
> > efficiency, we should probably prefer it as the more maintainable
> > option.
> 
> This is clever, but I don't like that it requires outside code to
> change internal `string_list` structures in a way that is not
> documented to be OK.
> 
> If we cared about getting rid of the extra `O(lg N)` search (and I
> agree with you that it doesn't matter in this case), I think the clean
> way to do it would be for `string_list` to expose a method like
> 
>     struct string_list_item *string_list_insert_at_index(struct
> string_list *list, size_t index, const char *string);
> 
> and to use it, together with `string_list_find_insert_index()`, to
> avoid having to search twice.

Yes, agreed on all counts.

-Peff