On Wed, May 8, 2019 at 10:52 PM Derrick Stolee <stolee@xxxxxxxxx> wrote: > > On 5/8/2019 10:41 AM, Duy Nguyen wrote: > > On Wed, May 8, 2019 at 9:07 PM Derrick Stolee <stolee@xxxxxxxxx> wrote: > >> > >> On 5/8/2019 7:12 AM, Nguyễn Thái Ngọc Duy wrote: > >>> Bitfield addresses cannot be passed around in a pointer. This makes it > >>> hard to use parse-options to set/unset them. Turn this struct to > >>> normal integers. This of course increases the size of this struct > >>> multiple times, but since we only have a handful of rev_info variables > >>> around, memory consumption is not at all a concern. > >> > >> I think you are right that this memory trade-off shouldn't be a problem. > >> > >> What worries me instead is that we are using an "internal" data structure > >> for option parsing. Would it make more sense to create a struct for use > >> in the parse_opts method and a method that translates those options into > >> the bitfield in struct rev_info? > > > > But we are doing that now (option parsing) using the same data > > structure. Why would changing from a custom parser to parse_options() > > affect what fields it should or should not touch in rev_info? Genuine > > question. Maybe you could elaborate more about "internal". I probably > > missed something. Or maybe this is a good opportunity to separate > > intermediate option parsing variables from the rest of rev_info? > > You're right. I was unclear. > > rev_info stores a lot of data. Some of the fields are important > in-memory structures that are crucial to the workings of revision.c > and are never used by builtin/rev-list.c. Combining this purpose > with the option parsing seems smelly to me. > > Thinking more on it, I would prefer a more invasive change that may > pay off in the long term. These options, along with the "starting list" > values, could be extracted to a 'struct rev_options' that contains these > integers and the commit list. Then your option parsing changes could be > limited to a rev_options struct, which is later inserted into a rev_info > struct during setup_revisions(). > > Generally, the rev_info struct has too many members and could be split > into smaller pieces according to purpose. I created the topo_walk_info > struct as a way to not make the situation worse, but doesn't fix existing > pain. > > My ramblings are mostly complaining about old code that grew organically > across many many quality additions. It is definitely hard to understand > the revision-walking code, and perhaps it would be easier to understand > with a little more structure. Thanks. This is much better. > The biggest issue with my suggestion is that it requires changing the > consumers of the options, as they would no longer live directly on the > rev_info struct. That would be a big change, even if it could be done > with string replacement. I agree rev_info has grown "wild". It's quite ancient code. As you noted, it's a big change. And since my series is already long (76 patches), I would rather focus on just one thing, rewriting the parsing code with minimum changes to anything else, preferably retain the exact same old behavior. After that work is done (and no regression found), we could focus on reorganizing rev_info, which could be quite "interesting". Some fields may be overloaded with different purposes, which I just can't spend time investigating now. There's also the problem with freeing resources after rev-list is done, which I think we have ignored so far. -- Duy
