Re: [PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Apr 14, 2017, 20:13 UTC
- Message-ID
- <5441277f-8d8a-23dc-2c1a-d637286f007c@google.com>
- In-Reply-To
- <20170411234923.1860-4-sbeller@google.com>
I think "harden" is the wrong word to use in the subject - to me, "harden" implies that you're defending against an invalid use. But here, not only is the use valid, but this patch also takes steps to support it.
On 04/11/2017 04:49 PM, Stefan Beller wrote:
Show 28 quoted lines
> Early on in submodule_move_head just after the check if the submodule is > initialized, we need to check if the submodule is populated correctly. > > If the submodule is initialized but doesn't look like populated, this > is a red flag and can indicate multiple sorts of failures: > (1) The submodule may be recorded at an object name, that is missing. > (2) The submodule '.git' file link may be broken and it is not pointing > at a repository. > > In both cases we want to complain to the user in the non-forced mode, > and in the forced mode ignoring the old state and just moving the > submodule into its new state with a fixed '.git' file link. > > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > submodule.c | 17 +++++++++++++++++ > t/lib-submodule-update.sh | 23 ++++++++++++++++++++--- > 2 files changed, 37 insertions(+), 3 deletions(-) > > diff --git a/submodule.c b/submodule.c > index 2fa42519a4..dda1ead854 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -1331,10 +1331,19 @@ int submodule_move_head(const char *path, > int ret = 0; > struct child_process cp = CHILD_PROCESS_INIT; > const struct submodule *sub; > + int *errorcode, error_code;
Maybe call it error_code_ptr?
Show 10 quoted lines
> > if (!is_submodule_initialized(path)) > return 0; > > + if (flags & SUBMODULE_MOVE_HEAD_FORCE) > + errorcode = &error_code; > + else > + errorcode = NULL; > + > + if (old && !is_submodule_populated_gently(path, errorcode))
This behavior of the null-ness of the pointer controlling whether the function dies upon error or not is quite confusing to me, so I would add a comment (e.g. under the "errorcode = &error_code" statement, saying "Pass a non-null pointer to make is_submodule_populated_gently report errors to use instead of die()-ing").
Show 16 quoted lines
> + return 0;
> +
> sub = submodule_from_path(null_sha1, path);
>
> if (!sub)
> @@ -1361,6 +1370,14 @@ int submodule_move_head(const char *path,
> /* make sure the index is clean as well */
> submodule_reset_index(path);
> }
> +
> + if (old && (flags & SUBMODULE_MOVE_HEAD_FORCE)) {
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_addf(&sb, "%s/modules/%s",
> + get_git_common_dir(), sub->name);
> + connect_work_tree_and_git_dir(path, sb.buf);
> + strbuf_release(&sb);xstrfmt might be simpler, but I don't have a strong preference either way.
Show 44 quoted lines
> + }
> }
>
> prepare_submodule_repo_env_no_git_dir(&cp.env_array);
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 22dd9e060c..f0b1b18206 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -1213,14 +1213,31 @@ test_submodule_forced_switch_recursing () {
> )
> '
> # Updating a submodule from an invalid sha1 updates
> - test_expect_success "$command: modified submodule does not update submodule work tree from invalid commit" '
> + test_expect_success "$command: modified submodule does update submodule work tree from invalid commit" '
> prolog &&
> reset_work_tree_to_interested invalid_sub1 &&
> (
> cd submodule_update &&
> git branch -t valid_sub1 origin/valid_sub1 &&
> - test_must_fail $command valid_sub1 &&
> - test_superproject_content origin/invalid_sub1
> + $command valid_sub1 &&
> + test_superproject_content origin/valid_sub1 &&
> + test_submodule_content sub1 origin/valid_sub1
> + )
> + '
> +
> + # Old versions of Git were buggy writing the .git link file
> + # (e.g. before f8eaa0ba98b and then moving the superproject repo
> + # whose submodules contained absolute paths)
> + test_expect_success "$command: updating submodules fixes .git links" '
> + prolog &&
> + reset_work_tree_to_interested add_sub1 &&
> + (
> + cd submodule_update &&
> + git branch -t modify_sub1 origin/modify_sub1 &&
> + echo "gitdir: bogus/path" >sub1/.git &&
> + $command modify_sub1 &&
> + test_superproject_content origin/modify_sub1 &&
> + test_submodule_content sub1 origin/modify_sub1
> )
> '
> }
>