On Thu, Feb 03 2022, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Feb 02 2022, Glen Choo wrote:
>
>> - 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...
>
>> - 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.
>
>> - 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
