Re: [PATCH v5 9/9] submodule: move core cmd_update() logic to C

On Thu, Feb 03 2022, Ævar Arnfjörð Bjarmason wrote:

On Wed, Feb 02 2022, Glen Choo wrote:

quoted

- Junio pointed out that this conflicts with
  es/superproject-aware-submodules [2]. I'm not sure which should be
  based on which. If this does end up being based on
  es/superproject-aware-submodules, it would probably be easier to
  rebase as a series of smaller patches. Atharva noted that the
  conflicts are mild though, so maybe it's not so bad.

I think it makes sense to get this series through first, i.e. the
(supposedly) no-behavior-changing one, and then one that introduces new
submodule behavior.

Particularly because for es/superproject-aware-submodules the main
selling point is a performance improvement, which as I noted in the
review for it I've been unable to observe once the C<->sh layer goes
away.

I'm not saying it's not there, just that I don't think it's been shown
so far, IIRC there was some reference to some Google-internal network FS
that might or might not be helped by it...

quoted

- Besides making sure that the sh -> c is faithful, a thorough review
  should hopefully catch unintentional mistakes. The size of this patch
  makes such mistakes difficult to spot. For instance, here's something
  I spotted only after trying to split the patch myself..

  > +static int module_update(int argc, const char **argv, const char *prefix)
  > +{
  > +	const char *update = NULL;
  > +	struct pathspec pathspec;
  > +	struct update_data opt = UPDATE_DATA_INIT;
  > +
  > +	struct option module_update_clone_options[] = {
  [...]
  > +	};
  > +
  > +	const char *const git_submodule_helper_usage[] = {
  > +		N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
  > +		NULL
  > +	};
  > +
  > +	update_clone_config_from_gitmodules(&opt.max_jobs);
  > +	git_config(git_update_clone_config, &opt.max_jobs);

  Notice that we copy-pasted the option parsing from update-clone into
  module_update() but forgot to update the names.

My ideal patch organization would be something like:

- wrap some existing command in "git submodule--helper update" (probably
  run-update-procedure)
- absorb the surrounding sh code into "git submodule--helper
  update" one command at-a-time i.e. deprecating and removing the
  commands one at a time - instead of deprecating and removing them all
  at once (like this patch), or deprecating all at once and removing
  them one at a time (like v1).

I do think atomic changes that don't leave dead code for removal later
are easier to read & reason about, whatever else is reorganized.

I.e. not to have something where we replace all the running code, and
then remove already-unused code later.

On that topic, I noticed this series could/should have [1] fixed up into
it.

quoted

- If you think this alternative organization would be helpful for you
  too, I will attempt it. This will take a while, but by the end you and
  I will have effectively reviewed all of the code, so it should be easy
  to finish up the review.

I think it might, but I really don't know. We'll just have to see, so if
you want to take a stab at it that would be great.

Maybe it's a good way forward. E.g. as af first small step we could turn:

    while read -r quickabort sha1 just_cloned sm_path
    [...]
    die_if_unmatched "$quickabort" "$sha1"

into version where we fold that die_if_unmatched() logic into the C
code, and then ensure-core-worktree etc.

Sorry, that one makes no sense since it's an artifact of the shellscript
implementation.

But I tested the below on top of master, and it passes all tests, which
isn't very promising...

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4a0890954e9..e749008f13a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -2783,40 +2783,6 @@ static int push_check(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static int ensure_core_worktree(int argc, const char **argv, const char *prefix)
-{
-	const char *path;
-	const char *cw;
-	struct repository subrepo;
-
-	if (argc != 2)
-		BUG("submodule--helper ensure-core-worktree <path>");
-
-	path = argv[1];
-
-	if (repo_submodule_init(&subrepo, the_repository, path, null_oid()))
-		die(_("could not get a repository handle for submodule '%s'"), path);
-
-	if (!repo_config_get_string_tmp(&subrepo, "core.worktree", &cw)) {
-		char *cfg_file, *abs_path;
-		const char *rel_path;
-		struct strbuf sb = STRBUF_INIT;
-
-		cfg_file = repo_git_path(&subrepo, "config");
-
-		abs_path = absolute_pathdup(path);
-		rel_path = relative_path(abs_path, subrepo.gitdir, &sb);
-
-		git_config_set_in_file(cfg_file, "core.worktree", rel_path);
-
-		free(cfg_file);
-		free(abs_path);
-		strbuf_release(&sb);
-	}
-
-	return 0;
-}
-
 static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 {
 	int i;

@@ -3391,7 +3357,6 @@ static struct cmd_struct commands[] = {
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"run-update-procedure", run_update_procedure, 0},
-	{"ensure-core-worktree", ensure_core_worktree, 0},
 	{"relative-path", resolve_relative_path, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},

diff --git a/git-submodule.sh b/git-submodule.sh
index 652861aa66a..460cbd4e265 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -387,8 +387,6 @@ cmd_update()
 	do
 		die_if_unmatched "$quickabort" "$sha1"
 
-		git submodule--helper ensure-core-worktree "$sm_path" || exit 1
-
 		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
 
 		if test $just_cloned -eq 1