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

From: Glen Choo <chooglen@google.com>
To: git@vger.kernel.org
Cc: Glen Choo <chooglen@google.com>
Subject: [PATCH v2 00/13] submodule: convert parts of 'update' to C
Date: Mon, 28 Feb 2022 20:41:19 -0800	[thread overview]
Message-ID: <20220301044132.39474-1-chooglen@google.com> (raw)
In-Reply-To: <20220301000816.56177-1-chooglen@google.com>

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

= Changes

Since v1:
- Fix compilation error due to bad rebase
- Remove accidentally included tests
- Fix a NEEDSWORK (it was a leftover from ar/submodule-update)

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    | 246 +++++++++++++++++++--------------
 git-submodule.sh               |  54 +-------
 t/t7406-submodule-update.sh    |  27 +++-
 t/t7408-submodule-reference.sh |  14 +-
 4 files changed, 182 insertions(+), 159 deletions(-)

Range-diff against v1:
 1:  6138f4682c =  1:  6138f4682c submodule tests: test for init and update failure output
 2:  6c83c78819 =  2:  6c83c78819 submodule--helper: remove update-module-mode
 3:  9524986096 =  3:  9524986096 submodule--helper: reorganize code for sh to C conversion
 4:  f42f3de2b7 =  4:  f42f3de2b7 submodule--helper run-update-procedure: remove --suboid
 5:  b0a0cae633 =  5:  b0a0cae633 submodule--helper: remove ensure-core-worktree
 6:  976cfa6d24 !  6:  3bde7ccd61 submodule--helper: get remote names from any repository
    @@ builtin/submodule--helper.c
      	struct strbuf sb = STRBUF_INIT;
     -	const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
     +	struct ref_store *store = get_main_ref_store(repo);
    -+	int ignore_errno;
     +	const char *refname = refs_resolve_ref_unsafe(store, "HEAD", 0, NULL,
    -+						      NULL, &ignore_errno);
    ++						      NULL);

      	if (!refname)
      		die(_("No such ref: %s"), "HEAD");
 7:  4a3c07aa4f =  7:  3b2caf7a35 submodule--helper: don't use bitfield indirection for parse_options()
 8:  bbfc2278c5 !  8:  81e9da8d42 submodule--helper run-update-procedure: learn --remote
    @@ Commit message

         Teach run-update-procedure to handle --remote instead of parsing
         --remote in git-submodule.sh. As a result, "git submodule--helper
    -    print-default-remote" has no more callers, so remove it.
    +    [print-default-remote|remote-branch]" have no more callers, so remove
    +    them.

         Signed-off-by: Glen Choo <chooglen@google.com>

    @@ builtin/submodule--helper.c: static int run_update_procedure(int argc, const cha
      		OPT_END()
      	};

    +@@ builtin/submodule--helper.c: static const char *remote_submodule_branch(const char *path)
    + 	return branch;
    + }
    + 
    +-static int resolve_remote_submodule_branch(int argc, const char **argv,
    +-		const char *prefix)
    +-{
    +-	const char *ret;
    +-	struct strbuf sb = STRBUF_INIT;
    +-	if (argc != 2)
    +-		die("submodule--helper remote-branch takes exactly one arguments, got %d", argc);
    +-
    +-	ret = remote_submodule_branch(argv[1]);
    +-	if (!ret)
    +-		die("submodule %s doesn't exist", argv[1]);
    +-
    +-	printf("%s", ret);
    +-	strbuf_release(&sb);
    +-	return 0;
    +-}
    +-
    + static int push_check(int argc, const char **argv, const char *prefix)
    + {
    + 	struct remote *remote;
     @@ builtin/submodule--helper.c: static int update_submodule2(struct update_data *update_data)
      		die(_("Unable to find current revision in submodule path '%s'"),
      			update_data->displaypath);
    @@ builtin/submodule--helper.c: static struct cmd_struct commands[] = {
      	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
      	{"deinit", module_deinit, 0},
      	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
    -+	/* NEEDSWORK: remote-branch is also obsolete */
    - 	{"remote-branch", resolve_remote_submodule_branch, 0},
    +-	{"remote-branch", resolve_remote_submodule_branch, 0},
      	{"push-check", push_check, 0},
      	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
    + 	{"is-active", is_active, 0},

      ## git-submodule.sh ##
     @@ git-submodule.sh: cmd_deinit()
 9:  765cd21505 =  9:  122da84ce4 submodule--helper: refactor get_submodule_displaypath()
10:  0c5e922f32 = 10:  fd52d6a2c3 submodule--helper: allow setting superprefix for init_submodule()
11:  287f36122a = 11:  9422c2ecac submodule--helper update-clone: learn --init
12:  91cac6e236 ! 12:  0a3e93998d submodule update: add tests for --filter
    @@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --quiet passe
      	)
      '

    -+test_expect_success 'submodule update adds superproject gitdir to older repos' '
    -+	(cd super &&
    -+	 git -C submodule config --unset submodule.superprojectGitdir &&
    -+	 git submodule update &&
    -+	 test-tool path-utils relative_path \
    -+		"$(git rev-parse --absolute-git-dir)" \
    -+		"$(git -C submodule rev-parse --absolute-git-dir)" >expect &&
    -+	 git -C submodule config submodule.superprojectGitdir >actual &&
    -+	 test_cmp expect actual
    -+	)
    -+'
    -+
    -+test_expect_success 'submodule update uses config.worktree if applicable' '
    -+	(cd super &&
    -+	 git -C submodule config --unset submodule.superprojectGitDir &&
    -+	 git -C submodule config extensions.worktreeConfig true &&
    -+	 git submodule update &&
    -+	 test-tool path-utils relative_path \
    -+		"$(git rev-parse --absolute-git-dir)" \
    -+		"$(git -C submodule rev-parse --absolute-git-dir)" >expect &&
    -+	 git -C submodule config submodule.superprojectGitdir >actual &&
    -+	 test_cmp expect actual &&
    -+
    -+	 test_file_not_empty "$(git -C submodule rev-parse --absolute-git-dir)/config.worktree"
    -+	)
    -+'
    -+
     +test_expect_success 'submodule update --filter requires --init' '
     +	test_must_fail git -C super submodule update --filter blob:none 2>err &&
     +	grep "usage:" err
13:  f637c23a48 = 13:  6e1ef27191 submodule--helper update-clone: check for --filter and --init

base-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7
-- 
2.33.GIT