Glen Choo <chooglen@xxxxxxxxxx> writes: > Calvin Wan <calvinwan@xxxxxxxxxx> writes: > >> Remove the serial implementation of status inside of >> is_submodule_modified since the parallel implementation of status with >> one job accomplishes the same task. > > I see that this is in direct response to Jonathan's earlier comment [1] > that we should have only one implementation. Thanks, this is helpful. > Definitely a step in the right direction. > > That said, I don't think this patch's position in the series makes > sense. I would have expected a patch like this to come before 5/6. I.e. > this series duplicates code in 5/6 and deletes it in 6/6 so that we only > have one implementation for both serial and parallel submodule status. > > Instead, I would have expected we would refactor out the serial > implementation, then use the refactored code for the parallel > implementation. Not having duplicated code in 5/6 would shrink the line > count a lot and make it easier to review. > > [1] https://lore.kernel.org/git/20221128210125.2751300-1-jonathantanmy@xxxxxxxxxx/ Ah, I realize I completely misunderstood this patch. I thought that this was deleting code that was duplicated between the serial and parallel implementations in 5/6 such that both ended up sharing just one copy of the code. Instead, this patch deletes the serial implementation altogether and replaces it with the parallel one. As such, this patch can't come earlier than 5/6, because we need the parallel implementation to exist before we can use it. For reviewability of 5/6, I'd still strongly prefer that we refactor out functions (I'll leave more specific comments on that patch). We could still consider replacing the serial implementation with "parallel with a single job", though I suspect that it will be unnecessary if we do the refactoring well. I'm also not sure how idiomatic it is to call run_processes_parallel() with a hardcoded value of 1, but I don't feel too strongly about that.
