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. 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. Thanks, -Stolee
