Re: [PATCH 00/13] submodule: convert parts of 'update' to C

From: Glen Choo <chooglen@google.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Atharva Raykar" <raykar.ath@gmail.com>,
	"Emily Shaffer" <emilyshaffer@google.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Josh Steadmon" <steadmon@google.com>
Subject: Re: [PATCH 00/13] submodule: convert parts of 'update' to C
Date: Mon, 28 Feb 2022 17:29:10 -0800	[thread overview]
Message-ID: <kl6l35k2pijd.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <20220301000816.56177-1-chooglen@google.com>

Glen Choo <chooglen@google.com> writes:

> Original series: https://lore.kernel.org/git/20220210092833.55360-1-chooglen@google.com
> (I've trimmed the cc list down to the 'most interested' parties)
>
> = Overview
>
> This is part 1 of 2 series that will supersede ar/submodule-update (as laid out
> in [1]). This series prepares for the eventual conversion of "git submodule
> update" to C by doing 'obvious' conversions first, and leaving more involved
> conversions for later.
>
> Part 1 is a lot simpler than the original series in its entirety, and should
> play better with topics that Junio identified:
>
> - This series is based off a later version of 'master' that already has
>   'js/apply-partial-clone-filters-recursively' merged in [2].
> - There is only one, trivial, conflict with 'es/superproject-aware-submodules'
>   (both add tests to the end of t7406) [3].
>
> Most of these patches were originally from ar/submodule-update, but because of
> the new organization, some commit messages have been amended to make more sense
> in context. However, patches 12 and 13 are new - they were added to handle the
> "--filter" option introduced by 'js/apply-partial-clone-filters-recursively'.
>
> Cc-ed Josh, who might be interested in "--filter" changes e.g. the new
> tests.
>
> [1] https://lore.kernel.org/git/kl6lmtig40l4.fsf@chooglen-macbookpro.roam.corp.google.com
> [2] This also fixes some trivial merge conflicts with 'master'.
> [3] Part 2 has nontrival conflicts though. Offline, Emily mentioned that
>     conflicts might go away in the next iteration of
>     'es/superproject-aware-submodules', but if not, the next round of patches
>     will probably be based on a merge of this series +
>     'es/superproject-aware-submodules'.
>
> = Patch summary
>
> I'm not certain whether to keep patch 13, see the extra discussion in
> the --- description for details.
>
> - Patch 1 adds extra tests to "git submodule update" to make sure we
>   don't break anything
> - Patch 2 removes dead code that used to be part of "git submodule
>   update"
> - Patch 3 prepares for later changes by introducing the C function that
>   will hold most of the newly converted code
> - Patch 4 moves run-update-procedure's --suboid option into C
> - Patch 5 moves ensure-core-worktree into C
> - Patches 6-8 move run-update-procedure's --remote option into C
> - Patches 9-11 move "git submodule update"'s --init into C
> - Patches 12-13 move "git submodule update"'s --filter option into C
>
> Atharva Raykar (3):
>   submodule--helper: get remote names from any repository
>   submodule--helper: refactor get_submodule_displaypath()
>   submodule--helper: allow setting superprefix for init_submodule()
>
> Glen Choo (8):
>   submodule--helper: remove update-module-mode
>   submodule--helper: reorganize code for sh to C conversion
>   submodule--helper run-update-procedure: remove --suboid
>   submodule--helper: remove ensure-core-worktree
>   submodule--helper run-update-procedure: learn --remote
>   submodule--helper update-clone: learn --init
>   submodule update: add tests for --filter
>   submodule--helper update-clone: check for --filter and --init
>
> Ævar Arnfjörð Bjarmason (2):
>   submodule tests: test for init and update failure output
>   submodule--helper: don't use bitfield indirection for parse_options()
>
>  builtin/submodule--helper.c    | 230 ++++++++++++++++++++-------------
>  git-submodule.sh               |  54 +-------
>  t/t7406-submodule-update.sh    |  54 +++++++-
>  t/t7408-submodule-reference.sh |  14 +-
>  4 files changed, 211 insertions(+), 141 deletions(-)
>
>
> base-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7
> -- 
> 2.33.GIT

Ugh, this version doesn't compile (I rebased it onto 'master' and forgot
to take into account ce14de03db (refs API: remove "failure_errno" from
refs_resolve_ref_unsafe(), 2022-01-26)).

This version is still reviewable, but I'll run this through CI again
before I send out v2.