[PATCH 0/5] Fix --recurse-submodules for submodule worktree changes

[PATCH 0/5] Fix --recurse-submodules for submodule worktree changes

[Stefan Beller]

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 (5):
  t/lib-submodule-update.sh: clarify test
  t/lib-submodule-update.sh: Fix test ignoring ignored files in
    submodules
  t/lib-submodule-update.sh: add new test for submodule internal change
  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

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

[Stefan Beller]

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

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

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

[Stefan Beller]

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(+)

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

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

Please clarify "buggy test".  Without the original discussion (I am
imagining there is something that happened on the list recently),
here is my guess:

    We tried to make sure that an ignored file is $distimmed by
    $gostak, but forgot to tell the exclude mechanism that the path
    'sub1'ignored' we use for the test is actually ignored, so the
    fact that the file was not $distimmed when $gostak did its thing
    meant nothing---mark any path whose name is 'ignored' as ignored
    to really test the condition we want to test.

but I do not exactly know what verb to replace $distim and what noun
to replace $gostak above ;-)

Also, wouldn't this expose a bug in the implementation if that is
the case?

Thanks.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/lib-submodule-update.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> 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 &&

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

[Stefan Beller]

On Fri, Dec 22, 2017 at 11:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> It turns out that this buggy test passed due to the buggy implementation,
>> which will soon be corrected.  Let's fix the test first.
>
> Please clarify "buggy test".  Without the original discussion (I am
> imagining there is something that happened on the list recently),
> here is my guess:
>
>     We tried to make sure that an ignored file is $distimmed by
>     $gostak, but forgot to tell the exclude mechanism that the path
>     'sub1'ignored' we use for the test is actually ignored, so the
>     fact that the file was not $distimmed when $gostak did its thing
>     meant nothing---mark any path whose name is 'ignored' as ignored
>     to really test the condition we want to test.
>
> but I do not exactly know what verb to replace $distim and what noun
> to replace $gostak above ;-)

Just rewrote that commit message without distims and gostaking.

> Also, wouldn't this expose a bug in the implementation if that is
> the case?

Yes it is, which will be fixed. The bug is:

If the submodule HEAD is at the superprojects recorded object
name, we don't bother looking into the submodule at all, neither for
deleting untracked files, nor resetting changed tracked files.

Thanks,

Stefan

[PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change

[Stefan Beller]

The test is marked as a failure as the fix comes in a later patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/lib-submodule-update.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index fb0173ea87..15cf3e0b8b 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_failure "$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

Re: [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> The test is marked as a failure as the fix comes in a later patch.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  t/lib-submodule-update.sh | 11 +++++++++++
>  1 file changed, 11 insertions(+)

I think I'd find this easier to undrestand if it were squashed with the
patch that fixes it.

This is part of test_submodule_foced_switch --- does that mean it
affects both checkout -f and reset --hard, or only the latter?

Thanks,
Jonathan

Re: [PATCH 3/5] t/lib-submodule-update.sh: add new test for submodule internal change

[Stefan Beller]

On Tue, Dec 19, 2017 at 2:31 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Stefan Beller wrote:
>
>> The test is marked as a failure as the fix comes in a later patch.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  t/lib-submodule-update.sh | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>
> I think I'd find this easier to undrestand if it were squashed with the
> patch that fixes it.

ok, let's squash this into the last patch then.

> This is part of test_submodule_foced_switch --- does that mean it
> affects both checkout -f and reset --hard, or only the latter?

All of
  checkout -f
  reset --hard (and not reset --keep ;)
  read-tree -u --reset

Thanks,
Stefan

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

[Stefan Beller]

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(+)

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

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

[Stefan Beller]

With the previous patch applied (fix of the same() function), the
function `submodule_move_head` may be invoked with the same argument
for the `old` and `new` state of a submodule, for example when you
run `reset --hard --recurse-submodules` in the superproject that has no
change in the gitlink entry, but only worktree related change in the
submodule. The read-tree call in the submodule is not amused about
the duplicate argument.

It turns out that we can omit the duplicate old argument in all forced
cases anyway, so let's do that.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c               | 4 +++-
 t/lib-submodule-update.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

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 15cf3e0b8b..7b6661cc84 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -1016,7 +1016,7 @@ test_submodule_forced_switch_recursing_with_args () {
 		)
 	'
 
-	test_expect_failure "$command: changed submodule worktree is reset" '
+	test_expect_success "$command: changed submodule worktree is reset" '
 		prolog &&
 		reset_work_tree_to_interested add_sub1 &&
 		(
-- 
2.15.1.620.gb9897f4670-goog

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

[Jonathan Nieder]

Hi,

I had trouble understanding what this fixes, so I'll try nitpicking a
bit as a sideways way to address that.

Stefan Beller wrote:

> With the previous patch applied (fix of the same() function),

This tripped me up a bit.  Usually commits assume that all previous
patches have already been applied, since that allows the maintainer to
apply the early part of a series on one day and the later part another
day without losing too much context.

I think this intends to say something like

	Now that we allow recursing into an unchanged submodule (see
	"unpack-trees: Fix same() for submodules", 2017-12-19), it is
	possible for ...

>                                                               the
> function `submodule_move_head` may be invoked with the same argument
> for the `old` and `new` state of a submodule, for example when you
> run `reset --hard --recurse-submodules` in the superproject that has no
> change in the gitlink entry, but only worktree related change in the
> submodule. The read-tree call in the submodule is not amused about
> the duplicate argument.

What is the symptom of read-tree being unamused?  Is that a sign that
these patches are out of order (i.e. that we should prepare to handle an
unchanged submodule first, and then adjust the caller to pass in
unchanged submodules)?

> It turns out that we can omit the duplicate old argument in all forced
> cases anyway, so let's do that.

What is the end-user visibile effect of such a change?  E.g. what
becomes possible to a user that wasn't possible before?

Among the commands you mentioned before:

  checkout -f
	I think I would expect this not to touch a submodule that
	hasn't changed, since that would be consistent with its
	behavior on files that haven't changed.

  reset --hard
	Nice!  Yes, recursing into unchanged submodules is a big part
	of the psychological comfort of being able to say "you can
	always use reset --hard <commit> to get back to a known
	state".

  read-tree -u --reset
	This is essentially the plumbing equivalent of reset --hard,
	so it makes sense for them to be consistent.  Ok.

Based on the checkout -f case, if I've understood correctly then patch
4/5 goes too far on it (but I could easily be convinced otherwise).

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c               | 4 +++-
>  t/lib-submodule-update.sh | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> 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);

Interesting.  What is the effect when old != new?

Thanks,
Jonathan

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

[Stefan Beller]

On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> I had trouble understanding what this fixes, so I'll try nitpicking a
> bit as a sideways way to address that.
>
> Stefan Beller wrote:
>
>> With the previous patch applied (fix of the same() function),
>
> This tripped me up a bit.  Usually commits assume that all previous
> patches have already been applied, since that allows the maintainer to
> apply the early part of a series on one day and the later part another
> day without losing too much context.
>
> I think this intends to say something like
>
>         Now that we allow recursing into an unchanged submodule (see
>         "unpack-trees: Fix same() for submodules", 2017-12-19), it is
>         possible for ...


yup

>
>>                                                               the
>> function `submodule_move_head` may be invoked with the same argument
>> for the `old` and `new` state of a submodule, for example when you
>> run `reset --hard --recurse-submodules` in the superproject that has no
>> change in the gitlink entry, but only worktree related change in the
>> submodule. The read-tree call in the submodule is not amused about
>> the duplicate argument.
>
> What is the symptom of read-tree being unamused?  Is that a sign that
> these patches are out of order (i.e. that we should prepare to handle an
> unchanged submodule first, and then adjust the caller to pass in
> unchanged submodules)?
>
>> It turns out that we can omit the duplicate old argument in all forced
>> cases anyway, so let's do that.
>
> What is the end-user visibile effect of such a change?  E.g. what
> becomes possible to a user that wasn't possible before?
>
> Among the commands you mentioned before:
>
>   checkout -f
>         I think I would expect this not to touch a submodule that
>         hasn't changed, since that would be consistent with its
>         behavior on files that haven't changed.
>
>   reset --hard
>         Nice!  Yes, recursing into unchanged submodules is a big part
>         of the psychological comfort of being able to say "you can
>         always use reset --hard <commit> to get back to a known
>         state".

I may have a different understanding of git commands than you do,
but a plain "checkout -f" with no further arguments is the same as
a hard reset IMHO, and could be used interchangeably?

Rehashing the "What is a submodule?" discussion, I would claim
that when its worktree is changed, we'd want checkout to restore
the submodules worktree back, so I disagree with your assessment
of checkout -f.

>   read-tree -u --reset
>         This is essentially the plumbing equivalent of reset --hard,
>         so it makes sense for them to be consistent.  Ok.
>
> Based on the checkout -f case, if I've understood correctly then patch
> 4/5 goes too far on it (but I could easily be convinced otherwise).

Without 4/5 we cannot implement hard reset recursing into submodules
as it is functionally the same as forced checkout except when we
differentiate them
on a higher layer?

>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  submodule.c               | 4 +++-
>>  t/lib-submodule-update.sh | 2 +-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> 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);
>
> Interesting.  What is the effect when old != new?

when the force flag is set we mostly pass in old="HEAD", which is
technically correct,
but not a sha1. (I assume you want to know what happens when two unequal sha1s
are given; for that it will perform a 2 way merge instead of a complete reset)

Thanks,
Stefan

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

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>   checkout -f
>>         I think I would expect this not to touch a submodule that
>>         hasn't changed, since that would be consistent with its
>>         behavior on files that haven't changed.
[...]
> I may have a different understanding of git commands than you do,
> but a plain "checkout -f" with no further arguments is the same as
> a hard reset IMHO, and could be used interchangeably?

Ah, good catch.  Filed https://crbug.com/git/5 to clarify this in
documentation.  Thanks for clarifying.

Jonathan

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

[Jonathan Nieder]

Stefan Beller wrote:
> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>   checkout -f
>>         I think I would expect this not to touch a submodule that
>>         hasn't changed, since that would be consistent with its
>>         behavior on files that haven't changed.
[...]
> I may have a different understanding of git commands than you do,
> but a plain "checkout -f" with no further arguments is the same as
> a hard reset IMHO, and could be used interchangeably?

A kind person pointed out privately that you were talking about
"git checkout -f" with no further arguments, not "git checkout -f
<commit>".  In that context, the competing meanings I mentioned in
https://crbug.com/git/5 don't exist: either a given entry in the
worktree matches the index or it doesn't.

So plain "git checkout -f" is similar to plain "git reset --hard"
in that it means "make the worktree (and index, in the reset case)
look just like this".  checkout -f makes the worktree look like the
index; reset --hard makes the worktree and index look like HEAD.

In that context, more aggressively making the submodule in the
worktree get into the defined state sounds to me like a good change.

Hopefully my confusion helps illustrate what a commit message focusing
on the end user experience might want to mention.

Thanks again,
Jonathan

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

[Stefan Beller]

On Tue, Dec 19, 2017 at 4:01 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Stefan Beller wrote:
>> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>>   checkout -f
>>>         I think I would expect this not to touch a submodule that
>>>         hasn't changed, since that would be consistent with its
>>>         behavior on files that haven't changed.
> [...]
>> I may have a different understanding of git commands than you do,
>> but a plain "checkout -f" with no further arguments is the same as
>> a hard reset IMHO, and could be used interchangeably?
>
> A kind person pointed out privately that you were talking about
> "git checkout -f" with no further arguments, not "git checkout -f
> <commit>".  In that context, the competing meanings I mentioned in
> https://crbug.com/git/5 don't exist: either a given entry in the
> worktree matches the index or it doesn't.
>
> So plain "git checkout -f" is similar to plain "git reset --hard"
> in that it means "make the worktree (and index, in the reset case)
> look just like this".

with "this" == the argument that was given, if the argument
was omitted, HEAD is assumed.

>  checkout -f makes the worktree look like the index;

No, here is what my installation of git (recent master) does:

  $ git --version
git version 2.15.1.389.g52015aaf9d

  $ cat test.sh

  rm -rf tmp
  git init tmp

  cd tmp
  git commit --allow-empty -m initial
  echo commit >a
  echo commit >b
  echo commit >c
  git add .
  git commit -m commit

  echo index >a
  git add a
  echo worktree >a

  echo index >b
  git add b

  echo worktree>c

  $ sh test.sh
Initialized empty Git repository in /u/tmp/.git/
[master (root-commit) c109fb5] initial
[master fcc21ea] commit
 3 files changed, 3 insertions(+)
 create mode 100644 a
 create mode 100644 b
 create mode 100644 c
  $ cd tmp
  $ git status
On branch master
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

modified:   a
modified:   b

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

modified:   a
modified:   c

  $ git checkout -f
  $ git status
On branch master
nothing to commit, working tree clean

# Let's test the other commands as well:
  $ cd ..
  $ sh test.sh
   ...
  $ cd tmp
  $ git checkout -f HEAD
  $ git status
On branch master
nothing to commit, working tree clean

  # See, there is no difference between giving HEAD as an argument
  # or not! Try again with reset just for completeness:

  $ cd ..
  $ sh test.sh
   ...
  $ cd tmp
  $ git reset --hard
HEAD is now at b71ae70 commit
  $ git status
On branch master
nothing to commit, working tree clean

  # The only difference between reset and the checkout is that reset
  # tells me where we are.

> reset --hard makes the worktree and index look like HEAD.

I agree.

> In that context, more aggressively making the submodule in the
> worktree get into the defined state sounds to me like a good change.
>
> Hopefully my confusion helps illustrate what a commit message focusing
> on the end user experience might want to mention.

I'll try to come up with a better commit message. Thanks for bearing
with me here.

Stefan


>
> Thanks again,
> Jonathan

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

[Stefan Beller]

On Wed, Dec 20, 2017 at 11:36 AM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Dec 19, 2017 at 4:01 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Stefan Beller wrote:
>>> On Tue, Dec 19, 2017 at 2:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>
>>>>   checkout -f
>>>>         I think I would expect this not to touch a submodule that
>>>>         hasn't changed, since that would be consistent with its
>>>>         behavior on files that haven't changed.
>> [...]
>>> I may have a different understanding of git commands than you do,
>>> but a plain "checkout -f" with no further arguments is the same as
>>> a hard reset IMHO, and could be used interchangeably?
>>
>> A kind person pointed out privately that you were talking about
>> "git checkout -f" with no further arguments, not "git checkout -f
>> <commit>".  In that context, the competing meanings I mentioned in
>> https://crbug.com/git/5 don't exist: either a given entry in the
>> worktree matches the index or it doesn't.
>>
>> So plain "git checkout -f" is similar to plain "git reset --hard"
>> in that it means "make the worktree (and index, in the reset case)
>> look just like this".
>
> with "this" == the argument that was given, if the argument
> was omitted, HEAD is assumed.
>
>>  checkout -f makes the worktree look like the index;
>
> No, here is what my installation of git (recent master) does:

Well, you are technically correct, the worktree is made look like the index,
but prior to that the index is reset to the HEAD, so for me it is
easier to understand
as "make worktree and index like HEAD"

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

[Stefan Beller]

I dropped the patch to `same()` as I realized we only need to fix the
oneway_merge function, the others (two, three way merge) are fine as
they have the checks already in place.

The test added in the last patch got slightly larger as now we also test for
newly staged files to be blown away in the submodule.

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

Stefan Beller (5):
  t/helper/test-lazy-name-hash: fix compilation
  t/lib-submodule-update.sh: clarify test
  t/lib-submodule-update.sh: Fix test ignoring ignored files in
    submodules
  unpack-trees: oneway_merge to update submodules
  submodule: submodule_move_head omits old argument in forced case

 submodule.c                         |  4 +++-
 t/helper/test-lazy-init-name-hash.c |  2 +-
 t/lib-submodule-update.sh           | 19 +++++++++++++++++--
 unpack-trees.c                      |  3 +++
 4 files changed, 24 insertions(+), 4 deletions(-)

-- 
2.15.1.620.gb9897f4670-goog

[PATCHv2 1/5] t/helper/test-lazy-name-hash: fix compilation

[Stefan Beller]

I was compiling origin/master today with stricter compiler flags today
and was greeted by

t/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’:
t/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     printf("avg [size %8d] [single %f] %c [multi %f %d]\n",
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         nr,
         ~~~
         (double)avg_single/1000000000,
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         (avg_single < avg_multi ? '<' : '>'),
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         (double)avg_multi/1000000000,
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         nr_threads_used);
         ~~~~~~~~~~~~~~~~
t/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was declared here
  int nr_threads_used;
      ^~~~~~~~~~~~~~~

I do not see how we can arrive at that line without having `nr_threads_used`
initialized, as we'd have `count > 1`  (which asserts that we ran the
loop above at least once, such that it *should* be initialized).

I do not have time to dive into further analysis.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/helper/test-lazy-init-name-hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c
index 6368a89345..297fb01d61 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -112,7 +112,7 @@ static void analyze_run(void)
 {
 	uint64_t t1s, t1m, t2s, t2m;
 	int cache_nr_limit;
-	int nr_threads_used;
+	int nr_threads_used = 0;
 	int i;
 	int nr;
 
-- 
2.15.1.620.gb9897f4670-goog

[PATCHv2 2/5] t/lib-submodule-update.sh: clarify test

[Stefan Beller]

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

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

[PATCHv2 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules

[Stefan Beller]

It turns out that the test replacing a submodule with a file with
the submodule containing an ignored file is incorrectly titled,
because the test put the file in place, but never ignored that file.
When having an untracked file Instead of an ignored file in the
submodule, git should refuse to remove the submodule, but that is
a bug in the implementation of recursing into submodules, such that
the test just passed, removing the untracked file.

Fix the test first; in a later patch we'll fix gits behavior,
that will make sure untracked files are not deleted.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/lib-submodule-update.sh | 1 +
 1 file changed, 1 insertion(+)

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

[PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

When there is a one way merge, each submodule needs to be one way merged
as well, if we're asked to recurse into submodules.

In case of a submodule, check if it is up-to-date, otherwise set the
flag CE_UPDATE, which will trigger an update of it in the phase updating
the tree later.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 unpack-trees.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/unpack-trees.c b/unpack-trees.c
index bf8b602901..ac59524a7f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
 				update |= CE_UPDATE;
 		}
+		if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
+		    !verify_uptodate(old, o))
+			update |= CE_UPDATE;
 		add_entry(o, old, update, 0);
 		return 0;
 	}
-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/unpack-trees.c b/unpack-trees.c
> index bf8b602901..ac59524a7f 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
>  			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>  				update |= CE_UPDATE;
>  		}
> +		if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
> +		    !verify_uptodate(old, o))
> +			update |= CE_UPDATE;
>  		add_entry(o, old, update, 0);
>  		return 0;
>  	}

This is more sensible than the previous one that broke same() by
unconditionally checking the working tree state, even when the
machinery is told not to look at the working tree.

The code in the pre-context of this hunk, (i.e. the original one you
are half-way mimicking), we realize that the original cache entry
and the cache entry we are checking out are exactly the same and we
overwrite when we know that the path in the working tree is dirty,
and we are not told to "skip" that path in the working tree
(i.e. sparse checkout).  That happens only when we are told to
o->update and o->reset.

Shouldn't we be setting the update bit only when o->update is in
effect, just like we can see in the pre-context of this hunk?  I do
not know if we need to require o->reset and/or need to pay attention
to the skip worktree bit in this new case.

Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

On Tue, Jan 2, 2018 at 11:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index bf8b602901..ac59524a7f 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
>>                           ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>>                               update |= CE_UPDATE;
>>               }
>> +             if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
>> +                 !verify_uptodate(old, o))
>> +                     update |= CE_UPDATE;
>>               add_entry(o, old, update, 0);
>>               return 0;
>>       }
>
> This is more sensible than the previous one that broke same() by
> unconditionally checking the working tree state, even when the
> machinery is told not to look at the working tree.
>
> The code in the pre-context of this hunk, (i.e. the original one you
> are half-way mimicking), we realize that the original cache entry
> and the cache entry we are checking out are exactly the same and we
> overwrite when we know that the path in the working tree is dirty,
> and we are not told to "skip" that path in the working tree
> (i.e. sparse checkout).  That happens only when we are told to
> o->update and o->reset.
>
> Shouldn't we be setting the update bit only when o->update is in
> effect,

Yes, we should.

> just like we can see in the pre-context of this hunk?  I do
> not know if we need to require o->reset and/or need to pay attention
> to the skip worktree bit in this new case.

verify_uptodate takes care of the worktree bits
("o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))")

o->reset is taken care of inside the nested verify_uptodate_1 function via

    ...
    else if (o->reset || ce_uptodate(ce))
        return 0;

So I'd think we only need to toss in the o->update check.

And that additional check would only be a performance optimization
as it would omit the check in case we do not want to update.
(verify_uptodate is expensive for submodules)

[PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes

[Stefan Beller]

Thanks Junio for review of this series!
The only change in this version of the series is

--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,
                                update |= CE_UPDATE;
                }
                if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
-                   !verify_uptodate(old, o))
+                   o->update && !verify_uptodate(old, o))
                        update |= CE_UPDATE;
                add_entry(o, old, update, 0);


v2:
I dropped the patch to `same()` as I realized we only need to fix the
oneway_merge function, the others (two, three way merge) are fine as
they have the checks already in place.

The test added in the last patch got slightly larger as now we also test for
newly staged files to be blown away in the submodule.

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


Stefan Beller (5):
  t/helper/test-lazy-name-hash: fix compilation
  t/lib-submodule-update.sh: clarify test
  t/lib-submodule-update.sh: Fix test ignoring ignored files in
    submodules
  unpack-trees: oneway_merge to update submodules
  submodule: submodule_move_head omits old argument in forced case

 submodule.c                         |  4 +++-
 t/helper/test-lazy-init-name-hash.c |  2 +-
 t/lib-submodule-update.sh           | 19 +++++++++++++++++--
 unpack-trees.c                      |  3 +++
 4 files changed, 24 insertions(+), 4 deletions(-)

-- 
2.15.1.620.gb9897f4670-goog

[PATCH 1/5] t/helper/test-lazy-name-hash: fix compilation

[Stefan Beller]

I was compiling origin/master today with stricter compiler flags today
and was greeted by

t/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’:
t/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     printf("avg [size %8d] [single %f] %c [multi %f %d]\n",
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         nr,
         ~~~
         (double)avg_single/1000000000,
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         (avg_single < avg_multi ? '<' : '>'),
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         (double)avg_multi/1000000000,
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         nr_threads_used);
         ~~~~~~~~~~~~~~~~
t/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was declared here
  int nr_threads_used;
      ^~~~~~~~~~~~~~~

I do not see how we can arrive at that line without having `nr_threads_used`
initialized, as we'd have `count > 1`  (which asserts that we ran the
loop above at least once, such that it *should* be initialized).

I do not have time to dive into further analysis.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/helper/test-lazy-init-name-hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c
index 6368a89345..297fb01d61 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -112,7 +112,7 @@ static void analyze_run(void)
 {
 	uint64_t t1s, t1m, t2s, t2m;
 	int cache_nr_limit;
-	int nr_threads_used;
+	int nr_threads_used = 0;
 	int i;
 	int nr;
 
-- 
2.15.1.620.gb9897f4670-goog

[PATCH 2/5] t/lib-submodule-update.sh: clarify test

[Stefan Beller]

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

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

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

[Stefan Beller]

It turns out that the test replacing a submodule with a file with
the submodule containing an ignored file is incorrectly titled,
because the test put the file in place, but never ignored that file.
When having an untracked file Instead of an ignored file in the
submodule, git should refuse to remove the submodule, but that is
a bug in the implementation of recursing into submodules, such that
the test just passed, removing the untracked file.

Fix the test first; in a later patch we'll fix gits behavior,
that will make sure untracked files are not deleted.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/lib-submodule-update.sh | 1 +
 1 file changed, 1 insertion(+)

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

[PATCH 4/5] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

When there is a one way merge, each submodule needs to be one way merged
as well, if we're asked to recurse into submodules.

In case of a submodule, check if it is up-to-date, otherwise set the
flag CE_UPDATE, which will trigger an update of it in the phase updating
the tree later.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 unpack-trees.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/unpack-trees.c b/unpack-trees.c
index bf8b602901..7657d6ecdd 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
 				update |= CE_UPDATE;
 		}
+		if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
+		    o->update && !verify_uptodate(old, o))
+			update |= CE_UPDATE;
 		add_entry(o, old, update, 0);
 		return 0;
 	}
-- 
2.15.1.620.gb9897f4670-goog

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

[Stefan Beller]

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 | 14 ++++++++++++++
 2 files changed, 17 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..1f38a85371 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -1015,4 +1015,18 @@ 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 &&
+			: >sub1/new_file &&
+			git -C sub1 add new_file &&
+			$command HEAD &&
+			test_path_is_file sub1/file1 &&
+			test_path_is_missing sub1/new_file
+		)
+	'
 }
-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Thanks Junio for review of this series!
> The only change in this version of the series is
>
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,
>                                 update |= CE_UPDATE;
>                 }
>                 if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
> -                   !verify_uptodate(old, o))
> +                   o->update && !verify_uptodate(old, o))
>                         update |= CE_UPDATE;
>                 add_entry(o, old, update, 0);
>

Sounds OK.  

I wonder why o->update is not at the very beginning of the &&-chain,
though.  After all, the one above this addition begins with o->reset
&& o->update *not* because of the performance concern, but primarily
due to logic flow.  I.e. "if we are resetting and updating the
working tree, then..." comes first before saying "we may need to
flip CE_UPDATE bit in update variable if the file in the working
tree is not up to date and it is within a narrow checkout area".

Of course, because verify_uptodate() is rather expensive, checking
o->update before that makes sense from micro-optimization's point of
view, too.

So after thinking aloud like the above, I am reasonably sure that
you want to check o->update as the very first thing in this new if
statement.

> v2:
> I dropped the patch to `same()` as I realized we only need to fix the
> oneway_merge function, the others (two, three way merge) are fine as
> they have the checks already in place.

This is a bit flawed argument, no?  Checking working tree paths
unconditionally in same(), which does not even know if we are
touching the working tree paths, is broken.  Unless "they have the
checks already in place" refers to checks that bypasses calls to
same() when we are not touching working tree paths, that is, but
obviously that is not what is going on.

Will queue.  Thanks for working on this.

Re: [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes

[Stefan Beller]

On Wed, Jan 3, 2018 at 12:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Thanks Junio for review of this series!
>> The only change in this version of the series is
>>
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,
>>                                 update |= CE_UPDATE;
>>                 }
>>                 if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
>> -                   !verify_uptodate(old, o))
>> +                   o->update && !verify_uptodate(old, o))
>>                         update |= CE_UPDATE;
>>                 add_entry(o, old, update, 0);
>>
>
> Sounds OK.
>
> I wonder why o->update is not at the very beginning of the &&-chain,
> though.  After all, the one above this addition begins with o->reset
> && o->update *not* because of the performance concern, but primarily
> due to logic flow.  I.e. "if we are resetting and updating the
> working tree, then..." comes first before saying "we may need to
> flip CE_UPDATE bit in update variable if the file in the working
> tree is not up to date and it is within a narrow checkout area".

It shows that I work too much with submodules. ;)
"If we have a submodule and ..." seemed to be the important
part when writing the patch.

> Of course, because verify_uptodate() is rather expensive, checking
> o->update before that makes sense from micro-optimization's point of
> view, too.

I would think S_ISGITLINK, should_update_submodules as well
as o->update are all on the same order of magnitude of costs
(some couple number of operations)  when
compared to verify_uptodate (spawning processes),
so as long as verify_uptodate goes last we'd be fine.

>
> So after thinking aloud like the above, I am reasonably sure that
> you want to check o->update as the very first thing in this new if
> statement.

Thanks for double checking and thinking about the code base with
a less submodule centric point of view.

Mind to squash it locally or want me to resend?
For a resend I'll wait a couple of days to see if there are more
comments needing to be addressed.


>
>> v2:
>> I dropped the patch to `same()` as I realized we only need to fix the
>> oneway_merge function, the others (two, three way merge) are fine as
>> they have the checks already in place.
>
> This is a bit flawed argument, no?  Checking working tree paths
> unconditionally in same(), which does not even know if we are
> touching the working tree paths, is broken.  Unless "they have the
> checks already in place" refers to checks that bypasses calls to
> same() when we are not touching working tree paths, that is, but
> obviously that is not what is going on.
>
> Will queue.  Thanks for working on this.
>
>

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

[Stefan Beller]

v4:
* dropped the patch "t/helper/test-lazy-name-hash: fix compilation"
  (that snuck into v3 by accident, it is already present at
   remotes/origin/jh/memihash-opt, but I am carrying it locally in
   most branches currently.)
* Junio pointed out the micro-optimisation below, both in terms of readability
  as well as performance

diff --git a/unpack-trees.c b/unpack-trees.c
index 7657d6ecdd..96c3327f19 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2139,8 +2139,8 @@ int oneway_merge(const struct cache_entry * const *src,
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
 				update |= CE_UPDATE;
 		}
-		if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
-		    o->update && !verify_uptodate(old, o))
+		if (o->update && S_ISGITLINK(old->ce_mode) &&
+		    should_update_submodules() && !verify_uptodate(old, o))
 			update |= CE_UPDATE;
 		add_entry(o, old, update, 0);
 		return 0;

v3:
Thanks Junio for review of this series!
The only change in this version of the series is

--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,
                                update |= CE_UPDATE;
                }
                if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
-                   !verify_uptodate(old, o))
+                   o->update && !verify_uptodate(old, o))
                        update |= CE_UPDATE;
                add_entry(o, old, update, 0);


v2:
I dropped the patch to `same()` as I realized we only need to fix the
oneway_merge function, the others (two, three way merge) are fine as
they have the checks already in place.

The test added in the last patch got slightly larger as now we also test for
newly staged files to be blown away in the submodule.

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

Stefan Beller (4):
  t/lib-submodule-update.sh: clarify test
  t/lib-submodule-update.sh: Fix test ignoring ignored files in
    submodules
  unpack-trees: oneway_merge to update submodules
  submodule: submodule_move_head omits old argument in forced case

 submodule.c               |  4 +++-
 t/lib-submodule-update.sh | 19 +++++++++++++++++--
 unpack-trees.c            |  3 +++
 3 files changed, 23 insertions(+), 3 deletions(-)

-- 
2.16.0.rc0.223.g4a4ac83678-goog

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

[Stefan Beller]

Keep the local branch name as the upstream branch name to avoid confusion.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-submodule-update.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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.16.0.rc0.223.g4a4ac83678-goog

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

[Stefan Beller]

It turns out that the test replacing a submodule with a file with
the submodule containing an ignored file is incorrectly titled,
because the test put the file in place, but never ignored that file.
When having an untracked file Instead of an ignored file in the
submodule, git should refuse to remove the submodule, but that is
a bug in the implementation of recursing into submodules, such that
the test just passed, removing the untracked file.

Fix the test first; in a later patch we'll fix gits behavior,
that will make sure untracked files are not deleted.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-submodule-update.sh | 1 +
 1 file changed, 1 insertion(+)

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.16.0.rc0.223.g4a4ac83678-goog

[PATCHv4 3/4] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

When there is a one way merge, each submodule needs to be one way merged
as well, if we're asked to recurse into submodules.

In case of a submodule, check if it is up-to-date, otherwise set the
flag CE_UPDATE, which will trigger an update of it in the phase updating
the tree later.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 unpack-trees.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/unpack-trees.c b/unpack-trees.c
index bf8b602901..96c3327f19 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
 			    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
 				update |= CE_UPDATE;
 		}
+		if (o->update && S_ISGITLINK(old->ce_mode) &&
+		    should_update_submodules() && !verify_uptodate(old, o))
+			update |= CE_UPDATE;
 		add_entry(o, old, update, 0);
 		return 0;
 	}
-- 
2.16.0.rc0.223.g4a4ac83678-goog

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

[Stefan Beller]

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 submodule.c               |  4 +++-
 t/lib-submodule-update.sh | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 2967704317..47ddc9b273 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1657,7 +1657,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..1f38a85371 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -1015,4 +1015,18 @@ 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 &&
+			: >sub1/new_file &&
+			git -C sub1 add new_file &&
+			$command HEAD &&
+			test_path_is_file sub1/file1 &&
+			test_path_is_missing sub1/new_file
+		)
+	'
 }
-- 
2.16.0.rc0.223.g4a4ac83678-goog

Re: [PATCHv4 0/4] Fix --recurse-submodules for submodule worktree changes

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> v4:
> Stefan Beller (4):
>   t/lib-submodule-update.sh: clarify test
>   t/lib-submodule-update.sh: Fix test ignoring ignored files in
>     submodules
>   unpack-trees: oneway_merge to update submodules
>   submodule: submodule_move_head omits old argument in forced case
>
>  submodule.c               |  4 +++-
>  t/lib-submodule-update.sh | 19 +++++++++++++++++--
>  unpack-trees.c            |  3 +++
>  3 files changed, 23 insertions(+), 3 deletions(-)

Thanks.  This one looks excellent.

Let's merge it to 'next' and cook it there.

[PATCHv2 5/5] submodule: submodule_move_head omits old argument in forced case

[Stefan Beller]

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 | 14 ++++++++++++++
 2 files changed, 17 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..1f38a85371 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -1015,4 +1015,18 @@ 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 &&
+			: >sub1/new_file &&
+			git -C sub1 add new_file &&
+			$command HEAD &&
+			test_path_is_file sub1/file1 &&
+			test_path_is_missing sub1/new_file
+		)
+	'
 }
-- 
2.15.1.620.gb9897f4670-goog