Re: [PATCH v5 9/9] submodule: move core cmd_update() logic to C
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Feb 3, 2022, 02:26 UTC
- Message-ID
- <220203.865ypw7jw6.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <kl6l4k5g246p.fsf@chooglen-macbookpro.roam.corp.google.com>
On Wed, Feb 02 2022, Glen Choo wrote:
Show 6 quoted lines
> - 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...
Show 35 quoted lines
> - 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 #