threads / patch / 47475

patch, 4 partst/lib-submodule-update.sh: clarify test

Subject: [PATCH 1/4] t/lib-submodule-update.sh: clarify test

## tl;dr

7 messages between Dec 20, 2017 and Dec 22, 2017. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Stefan Beller· Dec 20, 2017, 22:17 UTC · lore

[PATCHv2 0/4] Fix --recurse-submodules for submodule worktree changes

v2:
* squashed the test patch and the bugfix, rewriting the commit message entirely.
  This might not the way Jonathan imagined how I address the potential user
  confusion[1], but I think it does the job.
[1] https://public-inbox.org/git/20171220000148.GJ240141@aiede.mtv.corp.google.com/
v1:

The fix is in the last patch, the first patches are just massaging the code base to make the fix easy.

The second patch fixes a bug in the test, which was ineffective at testing. The third patch shows the problem this series addresses, the fourth patch is a little refactoring, which I want to keep separate as I would expect it to be a performance regression[1]. The first patch is unrelated, but improves the readability of submodule test cases, which we'd want to improve further.

Thanks, Stefan

[1] The performance should improve once we don't use so many processes
    any more, but that repository series is stalled for now.
Stefan Beller (4):
  t/lib-submodule-update.sh: clarify test
  t/lib-submodule-update.sh: Fix test ignoring ignored files in
    submodules
  unpack-trees: Fix same() for submodules
  submodule: submodule_move_head omits old argument in forced case
 submodule.c               |  4 +++-
 t/lib-submodule-update.sh | 16 ++++++++++++++--
 unpack-trees.c            |  2 ++
 3 files changed, 19 insertions(+), 3 deletions(-)
-- 
2.15.1.620.gb9897f4670-goog
Stefan Beller· Dec 20, 2017, 22:17 UTC · re: Stefan Beller · lore
Keep the local branch name as the upstream branch name to avoid confusion.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/lib-submodule-update.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/lib-submodule-update.sh +2 −2
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 38dadd2c29..d7699046f6 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -664,8 +664,8 @@ test_submodule_recursing_with_args_common() {
 			cd submodule_update &&
 			git -C sub1 checkout -b keep_branch &&
 			git -C sub1 rev-parse HEAD >expect &&
-			git branch -t check-keep origin/modify_sub1 &&
-			$command check-keep &&
+			git branch -t modify_sub1 origin/modify_sub1 &&
+			$command modify_sub1 &&
 			test_superproject_content origin/modify_sub1 &&
 			test_submodule_content sub1 origin/modify_sub1 &&
 			git -C sub1 rev-parse keep_branch >actual &&

-- 
2.15.1.620.gb9897f4670-goog
Stefan Beller· Dec 20, 2017, 22:17 UTC · re: Stefan Beller · lore

[PATCH 2/4] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules

It turns out that this buggy test passed due to the buggy implementation, which will soon be corrected. Let's fix the test first.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/lib-submodule-update.sh | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/lib-submodule-update.sh +1 −0
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index d7699046f6..fb0173ea87 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
+			echo ignored >.git/modules/sub1/info/exclude &&
 			: >sub1/ignored &&
 			$command replace_sub1_with_file &&
 			test_superproject_content origin/replace_sub1_with_file &&

-- 
2.15.1.620.gb9897f4670-goog
Stefan Beller· Dec 20, 2017, 22:17 UTC · re: Stefan Beller · lore

[PATCH 3/4] unpack-trees: Fix same() for submodules

The function same(a, b) is used to check if two entries a and b are the same. As the index contains the staged files the same() function can be used to check if files between a given revision and the index are the same.

In case of submodules, the gitlink entry is showing up as not modified despite potential changes inside the submodule.

Fix the function to examine submodules by looking inside the submodule. This patch alone doesn't affect any correctness garantuees, but in combination with the next patch this fixes the new test introduced earlier in this series.

This may be a slight performance regression as now we have to inspect any submodule thouroughly.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 unpack-trees.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to unpack-trees.c +2 −0
diff --git a/unpack-trees.c b/unpack-trees.c
index bf8b602901..4d839e8edb 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1427,6 +1427,8 @@ static int same(const struct cache_entry *a, const struct cache_entry *b)
 		return 1;
 	if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED)
 		return 0;
+	if (S_ISGITLINK(b->ce_mode) && should_update_submodules())
+		return !oidcmp(&a->oid, &b->oid) && !is_submodule_modified(b->name, 0);
 	return a->ce_mode == b->ce_mode &&
 	       !oidcmp(&a->oid, &b->oid);
 }

-- 
2.15.1.620.gb9897f4670-goog
Junio C Hamano· Dec 22, 2017, 19:57 UTC · re: Stefan Beller · lore

Re: [PATCH 3/4] unpack-trees: Fix same() for submodules

Stefan Beller <sbeller@google.com> writes:
Show 9 quoted lines
> The function same(a, b) is used to check if two entries a and b are the
> same.  As the index contains the staged files the same() function can be
> used to check if files between a given revision and the index are the same.
>
> In case of submodules, the gitlink entry is showing up as not modified
> despite potential changes inside the submodule.
>
> Fix the function to examine submodules by looking inside the submodule.
> This patch alone doesn't affect any correctness garantuees, but in
guarantees?  I somehow misread this as orangutan ;-)
Show 21 quoted lines
> combination with the next patch this fixes the new test introduced
> earlier in this series.
>
> This may be a slight performance regression as now we have to
> inspect any submodule thouroughly.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  unpack-trees.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index bf8b602901..4d839e8edb 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1427,6 +1427,8 @@ static int same(const struct cache_entry *a, const struct cache_entry *b)
>  		return 1;
>  	if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED)
>  		return 0;
> +	if (S_ISGITLINK(b->ce_mode) && should_update_submodules())
> +		return !oidcmp(&a->oid, &b->oid) && !is_submodule_modified(b->name, 0);
Isn't this is working at the wrong level?

It is technically correct to say "the function is used to check if two entries a and be are the same", but that statement misses a more important aspect of the function. It is asked if these two things are the same and does not get involved in seeing if the working tree is dirty with respect to (one of) the entries it was passed.

For example, a virtual merge in a recursive merge would not (and should not) care if local changes exist in the working tree, and this function does not get any hint if it is to look at the working tree to tell such a case and the outermost merge apart.

Somebody has to be careful not to stomp on local changes in the outmost merge in a recursive merge, but I do not think this function is designed to be the place to do so. Isn't a submodule with "potential changes" the same way?

>  	return a->ce_mode == b->ce_mode &&
>  	       !oidcmp(&a->oid, &b->oid);
>  }
Stefan Beller· Dec 20, 2017, 22:17 UTC · re: Stefan Beller · lore

[PATCH 4/4] submodule: submodule_move_head omits old argument in forced case

When using hard reset or forced checkout with the option to recurse into submodules, the submodules need to be reset, too.

It turns out that we need to omit the duplicate old argument to read-tree in all forced cases to omit the 2 way merge and use the more assertive behavior of reading the specific new tree into the index and updating the working tree.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c               |  4 +++-
 t/lib-submodule-update.sh | 11 +++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
Show changes to 2 files +14 −1

submodule.c, t/lib-submodule-update.sh

diff --git a/submodule.c b/submodule.c
index fa25888783..db0f7ac51e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path,
 	else
 		argv_array_push(&cp.args, "-m");
 
-	argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);
+	if (!(flags & SUBMODULE_MOVE_HEAD_FORCE))
+		argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);
+
 	argv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX);
 
 	if (run_command(&cp)) {
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index fb0173ea87..380ef4b4ae 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -1015,4 +1015,15 @@ test_submodule_forced_switch_recursing_with_args () {
 			test_submodule_content sub1 origin/modify_sub1
 		)
 	'
+
+	test_expect_success "$command: changed submodule worktree is reset" '
+		prolog &&
+		reset_work_tree_to_interested add_sub1 &&
+		(
+			cd submodule_update &&
+			rm sub1/file1 &&
+			$command HEAD &&
+			test_path_is_file sub1/file1
+		)
+	'
 }

-- 
2.15.1.620.gb9897f4670-goog
Junio C Hamano· Dec 22, 2017, 20:01 UTC · re: Stefan Beller · lore

Re: [PATCH 4/4] submodule: submodule_move_head omits old argument in forced case

Stefan Beller <sbeller@google.com> writes:
Show 7 quoted lines
> When using hard reset or forced checkout with the option to recurse into
> submodules, the submodules need to be reset, too.
>
> It turns out that we need to omit the duplicate old argument to read-tree
> in all forced cases to omit the 2 way merge and use the more assertive
> behavior of reading the specific new tree into the index and updating
> the working tree.

The phrase "the more assertive" made me imagine something like "reset --hard", which resurrect lost paths and also get rid of added paths. "reading the specific new tree into the index" smells more like "checkout $tree-ish $paths" that has an overlay semantics, that resurrects lost paths but does not get rid of added paths.

Perhaps not just "rm sub1/file1" but also add a new file that is not in HEAD to ensure that it will be blown away when $command is run to ensure that we got the distinction between the two right?

Show 42 quoted lines
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c               |  4 +++-
>  t/lib-submodule-update.sh | 11 +++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/submodule.c b/submodule.c
> index fa25888783..db0f7ac51e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path,
>  	else
>  		argv_array_push(&cp.args, "-m");
>  
> -	argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);
> +	if (!(flags & SUBMODULE_MOVE_HEAD_FORCE))
> +		argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);
> +
>  	argv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX);
>  
>  	if (run_command(&cp)) {
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index fb0173ea87..380ef4b4ae 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -1015,4 +1015,15 @@ test_submodule_forced_switch_recursing_with_args () {
>  			test_submodule_content sub1 origin/modify_sub1
>  		)
>  	'
> +
> +	test_expect_success "$command: changed submodule worktree is reset" '
> +		prolog &&
> +		reset_work_tree_to_interested add_sub1 &&
> +		(
> +			cd submodule_update &&
> +			rm sub1/file1 &&
> +			$command HEAD &&
> +			test_path_is_file sub1/file1
> +		)
> +	'
>  }

← back to recent threads