Re: [PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules

[Jonathan Tan]

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:

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 
---
 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?



if (!is_submodule_initialized(path))
return 0;

+   if (flags & SUBMODULE_MOVE_HEAD_FORCE)
+   errorcode = _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 = _code" statement, saying 
"Pass a non-null pointer to make is_submodule_populated_gently report 
errors to use instead of die()-ing").



+   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(, "%s/modules/%s",
+   get_git_common_dir(), sub->name);
+   connect_work_tree_and_git_dir(path, sb.buf);
+   strbuf_release();


xstrfmt might be simpler, but I don't have a strong preference either way.


+   }
}

prepare_submodule_repo_env_no_git_dir(_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
)
'
 }

Re: [PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules

[Stefan Beller]

On Thu, Apr 13, 2017 at 12:08 PM, Brandon Williams  wrote:
> On 04/11, Stefan Beller wrote:
>> 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.
>
> What about the case where you have marked a submodule as active but
> don't have its respective .gitdir yet?  For now i think it would be
> acceptable to complain and do nothing/ignore it, in the future we may
> want to actually clone and then check it out.

I agree. With this patch we'd complain in non-forced mode, and in
forced mode we'd also complain as we lack the object.

In both cases in the future we may want to fetch the contents instead.

Re: [PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules

[Brandon Williams]

On 04/11, Stefan Beller wrote:
> 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.

What about the case where you have marked a submodule as active but
don't have its respective .gitdir yet?  For now i think it would be
acceptable to complain and do nothing/ignore it, in the future we may
want to actually clone and then check it out.

-- 
Brandon Williams

Re: [PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules

[Philip Oakley]

From: "Stefan Beller" 

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

s/like//   or s/like/like it is/


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 
---

--
Philip


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;

 if (!is_submodule_initialized(path))
 return 0;

+ if (flags & SUBMODULE_MOVE_HEAD_FORCE)
+ errorcode = _code;
+ else
+ errorcode = NULL;
+
+ if (old && !is_submodule_populated_gently(path, errorcode))
+ 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(, "%s/modules/%s",
+ get_git_common_dir(), sub->name);
+ connect_work_tree_and_git_dir(path, sb.buf);
+ strbuf_release();
+ }
 }

 prepare_submodule_repo_env_no_git_dir(_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
 )
 '
}
--
2.12.2.603.g7b28dc31ba

[PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules

[Stefan Beller]

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 
---
 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;
 
if (!is_submodule_initialized(path))
return 0;
 
+   if (flags & SUBMODULE_MOVE_HEAD_FORCE)
+   errorcode = _code;
+   else
+   errorcode = NULL;
+
+   if (old && !is_submodule_populated_gently(path, errorcode))
+   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(, "%s/modules/%s",
+   get_git_common_dir(), sub->name);
+   connect_work_tree_and_git_dir(path, sb.buf);
+   strbuf_release();
+   }
}
 
prepare_submodule_repo_env_no_git_dir(_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
)
'
 }
-- 
2.12.2.603.g7b28dc31ba