Re: [PATCH v6 6/6] submodule: call parallel code from serial status

From: Glen Choo <chooglen@google.com>
Date: Jan 26, 2023, 08:09 UTC
Message-ID: <kl6lv8ktvhrp.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230117193041.708692-7-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

Show 7 quoted lines

> Remove the serial implementation of status inside of
> is_submodule_modified since the parallel implementation of status with
> one job accomplishes the same task.
>
> Combine parse_status_porcelain and parse_status_porcelain_strbuf since
> the only other caller of parse_status_porcelain was in
> is_submodule_modified

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@google.com/