On Thu, Sep 09, 2021 at 11:34:16AM +0200, Ævar Arnfjörð Bjarmason wrote: > In similar spirit as my > https://lore.kernel.org/git/87v93bidhn.fsf@xxxxxxxxxxxxxxxxxxx/ I > started seeing if not doing the flags via getopt but instead variables & > setting the flags later was better, and came up with this on top. Not > for this series, more to muse on how we can write these subcommands in a > simpler manner (or not). Sure, I think that everything you wrote here is correct. I don't have a strong opinion, really, but one benefit of not manipulating a single 'flags' int is that we don't have to do stuff like: if (git_config_bool(var, value)) opts.flags |= FLAG_FOO; else opts.flags &= ~FLAG_FOO; and instead can write `opts.foo = git_config_bool(var, value)`. Of course, the trade-off is that you later have to turn `opts.foo` into flags at some point (or pass each option as an individual parameter). So nothing's free, and I see it as a toss-up between which is easier to read and write. > I may have discovered a subtle bug in the process, in > cmd_multi_pack_index_repack() we end up calling write_midx_internal(), > which cares about MIDX_WRITE_REV_INDEX, but only > cmd_multi_pack_index_write() will set that flag, both before & after my > patch. Are we using the wrong flags during repack as a result? Only the `write` sub-command would ever want to set that flag, since we don't support writing a bitmap after `expire`. So that part seems right, but perhaps there is a another problem you're seeing? Thanks, Taylor
