Re: [PATCH v9 3/6] submodule: move status parsing into function

From: Glen Choo <chooglen@google.com>
Date: Mar 17, 2023, 20:42 UTC
Message-ID: <kl6l7cvfun3b.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230302220251.1474923-3-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

> A future patch requires the ability to parse the output of git
> status --porcelain=2. Move parsing code from is_submodule_modified to
> parse_status_porcelain.

If my mental model is correct [1], i.e. that we are implementing a parallel version of is_submodule_modified(). I think we should be more explicit in this patch and the next, e.g.:

  In a later patch, we will implement a parallel version of
  is_submodule_modified(). Refactor its "git status --porcelain=2"
  parsing code so that we can reuse it both the parallel and
  non-parallel versions.

If so, then this is pretty much doing the same thing as the next patch, so if the --color-moved diff isn't too bad, I think we can squash them, which will make the commit message easier to write too:

  In a later patch, we will implement a parallel version of
  is_submodule_modified(). Refactor its setup and parsing code so that
  we can reuse it both the parallel and non-parallel versions.

  - Setting up the subprocess is moved to prepare_status_porcelain()
  - XYZ is moved to verify_submodule_git_directory()
  - ABC is moved to parse_foobarbaz()

Just an idea. I don't think squashing is necessarily better, but being explciit that we want a parallel version of is_submodule_modified() will make this easier to follow.

[1] https://lore.kernel.org/git/kl6ljzzguqss.fsf@chooglen-macbookpro.roam.corp.google.com