bug: checkout --recurse-submodules discard uncommited changes in submodule

bug: checkout --recurse-submodules discard uncommited changes in submodule

[Kevin Bernard]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)
Make a git repository with a submodule in it.
Make two different commits in the main repository with two different versions of
the submodule, one of them is the head of the branch.
Checkout the head of the branch in the main repository, and make a submodule update.
Make a modification, do not commit it, in a submodule file that will not result
in a "error: Your local changes to the following files would be overwritten by
checkout" when the other version of the library will be checked out.
Go back to the main repository and make a checkout of the other commit with the
switch --recurse-submodules.

What did you expect to happen? (Expected behavior)
The checkout with the switch --recurse-submodules should fail when there are
uncommitted changes in the submodule.

What happened instead? (Actual behavior)
Uncommitted changes in the submodule are discarded without any notifications.

What's different between what you expected and what actually happened?
Loss of the uncommitted changes in the submodule.

Anything else you want to add:
I stay at your disposal if you need more information.

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.30.0.windows.1
cpu: x86_64
built from commit: 18da6dbba950f8cc7b7d07057f7c30bf7cf207b6
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
uname: Windows 10.0 19041 
compiler info: gnuc: 10.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe


[Enabled Hooks]

Sincerely yours,

Kevin Bernard

Re: bug: checkout --recurse-submodules discard uncommited changes in submodule

[Philippe Blain]

Hi Kevin,

Le 2021-01-14 à 03:35, Kevin Bernard a écrit :
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
> 
> What did you do before the bug happened? (Steps to reproduce your issue)
> Make a git repository with a submodule in it.
> Make two different commits in the main repository with two different versions of
> the submodule, one of them is the head of the branch.
> Checkout the head of the branch in the main repository, and make a submodule update.
> Make a modification, do not commit it, in a submodule file that will not result
> in a "error: Your local changes to the following files would be overwritten by
> checkout" when the other version of the library will be checked out.

I assume that you confirmed that by running 'git checkout <other version>'
in the submodule, right ?

> Go back to the main repository and make a checkout of the other commit with the
> switch --recurse-submodules.
> 
> What did you expect to happen? (Expected behavior)
> The checkout with the switch --recurse-submodules should fail when there are
> uncommitted changes in the submodule.
> 
> What happened instead? (Actual behavior)
> Uncommitted changes in the submodule are discarded without any notifications.
> 
> What's different between what you expected and what actually happened?
> Loss of the uncommitted changes in the submodule.
> 
> Anything else you want to add:
> I stay at your disposal if you need more information.


Thanks for the report. This is a known problem that was reported in [1] and [2].
I'm currently working on fixing that bug. I'm not quite finished yet as I want
to polish the end-user experience a bit, but if you want to compile from source,
the heart of the fix is this change:

diff --git a/unpack-trees.c b/unpack-trees.c
index 323280dd48..a3e3d98de1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1872,7 +1872,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,
  
  		if (submodule_from_ce(ce)) {
  			int r = check_submodule_move_head(ce,
-				"HEAD", oid_to_hex(&ce->oid), o);
+				"HEAD", empty_tree_oid_hex(), o);
  			if (r)
  				return add_rejected_path(o, error_type, ce->name);
  			return 0;


This should disallow switching branches with '--recurse-submodules' if *any* file
in the submodule is modified.

However, I'm actually thinking that maybe it would
be better to let the checkout succeed in the exact case you mention, i.e. when
the modified files in the submodules are at the same version in both submodule
commits (i.e. git checkout <other version> in the submodule succeeds and the
modified files stay modified), and also carry over the modified files in the
submodule. This would mimic the 'git checkout' experience
without submodules, i.e. modified files that do not conflict are carried over
to the branch you are checking out.

If you can write a complete reproducer (complete steps starting with 'git init',
etc.) I will make sure that your scenario is adequately tested in my patch series.


Cheers,

Philippe.


[1] https://lore.kernel.org/git/CAHsG2VT4YB_nf8PrEmrHwK-iY-AQo0VDcvXGVsf8cEYXws4nig@mail.gmail.com/
[2] https://lore.kernel.org/git/20200525094019.22padbzuk7ukr5uv@overdrive.tratt.net/

RE: bug: checkout --recurse-submodules discard uncommited changes in submodule

[Kevin Bernard]

Hi Philippe, 

I have no preferences whether the checkout should fail if any files are modified in the submodule
or allow the checkout to succeed if the modification can be carried out. 

I wrote a step by step procedure to reproduce the bug.

Step to reproduce my issue

git init  
git checkout -b master  
git submodule add https://github.com/kbernard-perso/recurse-bug.git  
git submodule init  
git add .  
git commit -m "Add submodule"  
cd recurse-bug/  
echo "file1 modified" >> file1.txt  
git add .  
git commit -m "file1.txt modified"  
cd ..  
git add .  
git commit -m "Modified file1 in submodule"  
cd recurse-bug/  
echo "file2 modified" >> file2.txt  
cd ..  
git status  
git checkout --recurse-submodules HEAD^  
git status  

The uncommitted modifications made with "echo "file2 modified" >> file2.txt" are  
discarded by git checkout --recurse-submodules HEAD^  

Thank you very much for making git a wonderful tool to use.

Cheers, 

Kevin

-----Message d'origine-----
De : Philippe Blain <levraiphilippeblain@gmail.com> 
Envoyé : jeudi, 14 janvier 2021 14:55
À : Kevin Bernard <kevin.bernard@fiveco.ch>; git@vger.kernel.org
Objet : Re: bug: checkout --recurse-submodules discard uncommited changes in submodule

Hi Kevin,

Le 2021-01-14 à 03:35, Kevin Bernard a écrit :
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
> 
> What did you do before the bug happened? (Steps to reproduce your 
> issue) Make a git repository with a submodule in it.
> Make two different commits in the main repository with two different 
> versions of the submodule, one of them is the head of the branch.
> Checkout the head of the branch in the main repository, and make a submodule update.
> Make a modification, do not commit it, in a submodule file that will 
> not result in a "error: Your local changes to the following files 
> would be overwritten by checkout" when the other version of the library will be checked out.

I assume that you confirmed that by running 'git checkout <other version>'
in the submodule, right ?

> Go back to the main repository and make a checkout of the other commit 
> with the switch --recurse-submodules.
> 
> What did you expect to happen? (Expected behavior) The checkout with 
> the switch --recurse-submodules should fail when there are uncommitted 
> changes in the submodule.
> 
> What happened instead? (Actual behavior) Uncommitted changes in the 
> submodule are discarded without any notifications.
> 
> What's different between what you expected and what actually happened?
> Loss of the uncommitted changes in the submodule.
> 
> Anything else you want to add:
> I stay at your disposal if you need more information.


Thanks for the report. This is a known problem that was reported in [1] and [2].
I'm currently working on fixing that bug. I'm not quite finished yet as I want to polish the end-user experience a bit, but if you want to compile from source, the heart of the fix is this change:

diff --git a/unpack-trees.c b/unpack-trees.c index 323280dd48..a3e3d98de1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1872,7 +1872,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,
  
  		if (submodule_from_ce(ce)) {
  			int r = check_submodule_move_head(ce,
-				"HEAD", oid_to_hex(&ce->oid), o);
+				"HEAD", empty_tree_oid_hex(), o);
  			if (r)
  				return add_rejected_path(o, error_type, ce->name);
  			return 0;


This should disallow switching branches with '--recurse-submodules' if *any* file in the submodule is modified.

However, I'm actually thinking that maybe it would be better to let the checkout succeed in the exact case you mention, i.e. when the modified files in the submodules are at the same version in both submodule commits (i.e. git checkout <other version> in the submodule succeeds and the modified files stay modified), and also carry over the modified files in the submodule. This would mimic the 'git checkout' experience without submodules, i.e. modified files that do not conflict are carried over to the branch you are checking out.

If you can write a complete reproducer (complete steps starting with 'git init',
etc.) I will make sure that your scenario is adequately tested in my patch series.


Cheers,

Philippe.


[1] https://lore.kernel.org/git/CAHsG2VT4YB_nf8PrEmrHwK-iY-AQo0VDcvXGVsf8cEYXws4nig@mail.gmail.com/
[2] https://lore.kernel.org/git/20200525094019.22padbzuk7ukr5uv@overdrive.tratt.net/