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.
> - Otherwise e.g. maybe this is a huge waste of time, or you're already
> really confident in the correctness of the sh -> c when you reviewed
> the original patch, etc, I'll just review this patch as-is. I'd
> appreciate any tips and tricks that might help :)
I'm not really confident in it.
I've read it, tested it as well as I could manage etc. but it's still a
very large change.
1.
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index b93f39288ce..756c2a67c72 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -286,13 +286,6 @@ get_author_ident_from_commit () {
parse_ident_from_commit author AUTHOR
}
-# Clear repo-local GIT_* environment variables. Useful when switching to
-# another repository (e.g. when entering a submodule). See also the env
-# list in git_connect()
-clear_local_git_env() {
- unset $(git rev-parse --local-env-vars)
-}
-
# Generate a virtual base file for a two-file merge. Uses git apply to
# remove lines from $1 that are not in $2, leaving only common lines.
create_virtual_base() {
diff --git a/git-submodule.sh b/git-submodule.sh
index bcd8b92aabd..6c91b9e2403 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -50,30 +50,11 @@ single_branch=
jobs=
recommend_shallow=
-die_if_unmatched ()
-{
- if test "$1" = "#unmatched"
- then
- exit ${2:-1}
- fi
-}
-
isnumber()
{
n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
}
-# Sanitize the local git environment for use within a submodule. We
-# can't simply use clear_local_git_env since we want to preserve some
-# of the settings from GIT_CONFIG_PARAMETERS.
-sanitize_submodule_env()
-{
- save_config=$GIT_CONFIG_PARAMETERS
- clear_local_git_env
- GIT_CONFIG_PARAMETERS=$save_config
- export GIT_CONFIG_PARAMETERS
-}
-
#
# Add a new submodule to the working tree, .gitmodules and the index
#
