Re: [PATCH v7 00/20] submodule: convert the rest of 'update' to C

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Feb 10 2022, Glen Choo wrote:

> This reroll contains another 'easy' preparatory patch and the fixups I
> alluded to in v6 [1]. This isn't the split-up I described in the
> footnote of v6, but it gets the big patch (patch 17) to what I think is
> a reviewable state.
>
> The diff between v7 and v5 is no longer just NEEDSWORK comments, but I
> think it is easier to reason about. Patch 17 resembles v5 the most (I
> will include a diff in a reply to that patch); everything after patch 17
> is fixups (I did not squash them in because they would grow the diff
> even more).
>
> I will also leave a review on patch 17 since the changes were not
> originally authored by me.
>
> [1] https://lore.kernel.org/git/20220208083952.35036-1-chooglen@google.com
>
> Changes in v7:
> - Split the last patch of v6 (the big one) into patches 16-17.
> - Patch 16 moves logic out of run_update_procedure() (because the
>   command is going away), removing some noise from patch 17. This makes
>   the update_strategy parsing easier to reason about, but at the cost of
>   growing the diff vis-a-vis v5
> - Patches 18-20 are fixups that address NEEDSWORK comments from earlier
>   patches. Once maintaining a small diff vis-a-vis v5 stops making
>   sense, I will squash them in.
>
> Atharva Raykar (6):
>   submodule--helper: get remote names from any repository
>   submodule--helper: refactor get_submodule_displaypath()
>   submodule--helper: allow setting superprefix for init_submodule()
>   submodule--helper: run update using child process struct
>   builtin/submodule--helper.c: reformat designated initializers
>   submodule: move core cmd_update() logic to C
>
> Glen Choo (11):
>   submodule--helper: remove update-module-mode
>   submodule--helper: reorganize code for sh to C conversion
>   submodule--helper run-update-procedure: remove --suboid
>   submodule--helper run-update-procedure: learn --remote
>   submodule--helper: remove ensure-core-worktree
>   submodule--helper update-clone: learn --init
>   submodule--helper: move functions around
>   submodule--helper: reduce logic in run_update_procedure()
>   fixup! submodule--helper run-update-procedure: remove --suboid
>   fixup! submodule--helper run-update-procedure: learn --remote
>   fixup! submodule: move core cmd_update() logic to C
>
> Ævar Arnfjörð Bjarmason (3):
>   builtin/submodule--helper.c: rename option variables to "opt"
>   submodule--helper: don't use bitfield indirection for parse_options()
>   submodule tests: test for init and update failure output

Thanks a lot for picking this up! This split-up is much easier to read
than my v5, particularly with the end diff-stat of the "main" patch
being:

 2 files changed, 201 insertions(+), 216 deletions(-)

Instead of:

 2 files changed, 356 insertions(+), 388 deletions(-)

I think sending a version of this with the fixups squashed in as a v8
would be good, and perhaps addressing some of my comments.

I don't know if my suggested split-up of "prep fixes" into another
series would be a good thing to pursue overall, perhaps Junio will chime
in on how he'd be most comfortable in merging this down. I'd think
splitting such trivial fixes into their own series be easier to review,
but perhaps not.

For the Signed-off-by question on v6, I think you should add your SOB to
all the patches you submit. See this in SubmittingPatches:
    
    Notice that you can place your own `Signed-off-by` trailer when
    forwarding somebody else's patch with the above rules for
    D-C-O.  Indeed you are encouraged to do so.

Just running "git rebase -i -x 'git commit --amend --no-edit -s'" should
do it.