On 10/8/2020 10:15 AM, Taylor Blau wrote: > On Thu, Oct 08, 2020 at 01:56:52PM +0000, Derrick Stolee via GitGitGadget wrote: >> Thus, this die() is too aggignoring the duplicates. > > s/aggignoring/aggressively ignoring ? > >> >> This leads to some additional complication that we did no have before: > > s/no/not, but I am more wondering about what "This" is. I think what > you're saying is: "Suppose we didn't die on duplicates, what would > happen? Well, there'd be some additional problems, but here's a way that > we can fix them (storing the de-duplicated OIDs separately)". Thanks. The message will be edited to fix these brain farts. >> I still don't have a grasp on how this happened in the first place, but >> will keep looking. > > I'm looking as well, but I haven't found any smoking guns yet. I could > imagine that this is a problem that existed before 0bd52e27e3 > (commit-graph.h: store an odb in 'struct write_commit_graph_context', > 2020-02-03), and simply couldn't be tickled because of how brittle > comparing ODB paths is. I could equally imagine that 0bd52e27e3 did > introduce this problem. Thanks. >> + ALLOC_ARRAY(deduped_commits.list, deduped_commits.alloc); > > I'm not sure that this deduped_commits list is actually necessary. > > It would be nice for this caller if ctx->commits were a linked list > since it would make deleting duplicates easy, but I think that it would > be a burden for other callers. So, that's a dead end. > > But what about marking the duplicate positions by NULL-ing them out, and > then taking another pass over the list to (1) compact it (i.e., push > everything down so that all of the NULLs occur at the end), and then (2) > truncate the length to the number of unique commits. > > I could imagine that something like that is a little trickier, but it > seems worth it to avoid doubling the memory cost of this function. You are correct that we can just re-use the commits.list by "collapsing" the list on top of duplicate entries. I'll send a new version that does exactly that. Thanks, -Stolee
