From: Jeff King <email@example.com> To: Stefan Beller <firstname.lastname@example.org> Cc: git <email@example.com>, firstname.lastname@example.org, Junio C Hamano <email@example.com>, Duy Nguyen <firstname.lastname@example.org> Subject: Re: ordered string-list considered harmful, was Re: [PATCH v3] Allow aliases that include other aliases Date: Thu, 6 Sep 2018 23:12:53 -0400 [thread overview] Message-ID: <20180907031253.GA31728@sigill.intra.peff.net> (raw) In-Reply-To: <CAGZ79kbFe8WMswWy+SorYUvEj2r5rUQdjx=zbVK5BfeU+Mgx9A@mail.gmail.com> On Thu, Sep 06, 2018 at 01:54:15PM -0700, Stefan Beller wrote: > > > It turns out we make never use of a custom compare function in > > > the stringlist, which helps gaining confidence this use case is nowhere > > > to be found in the code. > > > > Plenty of code uses the default strcmp. You can find users which assume > > sorting by their use of string_list_insert() versus _append(). Or ones > > that call string_list_sort(), of course. > > Here comes my reading-between-the-lines assumption: > > When using the default comparison function, you probably only care > about the efficient lookup as described above, but if you had a non-default > order, then we'd have strong evidence of the contrary as the author of such > code would have found reasons why that order is superior than default order > (and don't tell me a different order helps making lookups even more efficient, > this must be another reason). That's a reasonable hypothesis. It looks like there are a few cases where we assign to a string_list.cmp, so I picked one arbitrarily to look at: split_maildir() uses a custom filename comparison. Despite having no recollection of this code, it appears to come from my commit 18505c3423. :) And yes, we really do care about order there, as we're trying to read the files in "maildir order". And I don't think a hash would do. That said, I don't think we care about using string-list's sorted operations here (like its binary-search lookup). It would be enough for us to generate the list (whether as string-list or no; we'd actually be happy with any array), sort it, and then iterate over the result. So I think some of these are definitely going to require some thoughtful conversion to the correct data type. Mostly what I was asking in the beginning was: does this seem like an overtly terrible line of thinking to anyone. And so far I think the answer is no. So the next step is to proceed to actually trying some conversions, and seeing what kinds of snags I hit. The devil, as usual, is in the details. :) -Peff
prev parent reply other threads:[~2018-09-07 3:12 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-09-06 19:12 Jeff King 2018-09-06 19:20 ` Jeff King 2018-09-06 23:50 ` Jonathan Nieder 2018-09-07 3:24 ` Jeff King 2018-09-07 6:32 ` Jonathan Nieder 2018-09-07 7:20 ` Ævar Arnfjörð Bjarmason 2018-09-07 7:23 ` Jonathan Nieder 2018-09-08 16:49 ` brian m. carlson 2018-09-07 14:48 ` Jeff King 2018-09-06 20:04 ` Stefan Beller 2018-09-06 20:49 ` Jeff King 2018-09-06 20:54 ` Stefan Beller 2018-09-07 3:12 ` Jeff King [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180907031253.GA31728@sigill.intra.peff.net \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: ordered string-list considered harmful, was Re: [PATCH v3] Allow aliases that include other aliases' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).