Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out

Sven Verdoolaege [off-list ref] writes:

If you have a submodule checked out and you go back (or forward)
to a revision of the supermodule that contains a different
revision of the submodule and then switch to another revision,
it will complain that the submodule is not uptodate, because
git simply didn't update the submodule in the first move.

Now, you may say that I simply need to run 'git submodule update'
after every such move, but this is very inconvenient, especially
if you're doing a bisect or a rebase.

How do other people deal with this problem?

How about just replacing the body of ce_compare_gitlink
with "return 0" until git actually (optionally) updates
the submodules during an update of the supermodule?

Let me understand the problem first.  If your first checkout
does not check out the submodule, switching between revisions
that has different commit of the submodule there would not fail,
but once you checkout the submodule, switching without updating
the submodule would be Ok (because by design updating the
submodule is optional) but then further switching out of that
state will fail because submodule in the supermodule tree and
checked-out submodule repository are now out of sync.  Is that
the problem?

In any case, I doubt ce_compare_gitlink() is the right layer to
work this around -- it is not about "can we switch" but is about
"is it different".  It is at too low a level.

The current policy is to consider it is perfectly normal that
checked-out submodule is out-of-sync wrt the supermodule index,
if I am reading you right.  I think it is a good policy, at
least until we introduce a superproject repository configuration
option that says "in this repository, I do care about this
submodule and at any time I move around in the superproject,
recursively check out the submodule to match".  The most extreme
case of this policy is that the superproject index knows about
the submodule but the subdirectory does not even have to be
checked out, which is what we have now.

Where does the "No you are not up-to-date, I wouldn't let you
switch" come from?  Is that verify_uptodate() called from
merged_entry() called from twoway_merge()?  I think the right
approach to deal with this is to teach verify_uptodate() about
the policy.  The function is about "make sure the filesystem
entity that corresponds to this cache entry is up to date, lest
we lose the local modifications".  As we explicitly allow
submodule checkout to drift from the supermodule index entry,
the check should say "Ok, for submodules, not matching is the
norm" for now.  Later when we have the ability to mark "I care
about this submodule to be always in sync with the superproject"
(thereby implementing automatic recursive checkout and perhaps
diff, among other things), we should check if the submodule in
question is marked as such and perform the current test.

How about doing something like this instead?

 unpack-trees.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 3b32718..dfd985b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -407,6 +407,15 @@ static void verify_uptodate(struct cache_entry *ce,
 		unsigned changed = ce_match_stat(ce, &st, 1);
 		if (!changed)
 			return;
+		/*
+		 * NEEDSWORK: the current default policy is to allow
+		 * submodule to be out of sync wrt the supermodule
+		 * index.  This needs to be tightened later for
+		 * submodules that are marked to be automatically
+		 * checked out.
+		 */
+		if (S_ISGITLINK(ntohl(ce->ce_mode)))
+			return;
 		errno = 0;
 	}
 	if (errno == ENOENT)

On 8/4/07, Junio C Hamano [off-list ref] wrote:

As we explicitly allow
submodule checkout to drift from the supermodule index entry,
the check should say "Ok, for submodules, not matching is the
norm" for now.  Later when we have the ability to mark "I care
about this submodule to be always in sync with the superproject"
(thereby implementing automatic recursive checkout and perhaps
diff, among other things), we should check if the submodule in
question is marked as such and perform the current test.

Yes, this sounds like a sane plan (and a good explanation of the
current semantics: maybe something to include in the release notes for
1.5.3?)

Btw: I've applied your patch to rc-4 and tested the result in my cgit
repo: very nice, and very ack'd ;-)

--
larsh

On 2007-08-04 01:13, Junio C Hamano wrote:

Let me understand the problem first.  If your first checkout
does not check out the submodule, switching between revisions
that has different commit of the submodule there would not fail,
but once you checkout the submodule, switching without updating
the submodule would be Ok (because by design updating the
submodule is optional) but then further switching out of that
state will fail because submodule in the supermodule tree and
checked-out submodule repository are now out of sync.  Is that
the problem?

[snip]

Where does the "No you are not up-to-date, I wouldn't let you
switch" come from?  Is that verify_uptodate() called from
merged_entry() called from twoway_merge()?  I think the right
approach to deal with this is to teach verify_uptodate() about
the policy.  The function is about "make sure the filesystem
entity that corresponds to this cache entry is up to date, lest
we lose the local modifications".  As we explicitly allow
submodule checkout to drift from the supermodule index entry,
the check should say "Ok, for submodules, not matching is the
norm" for now.  Later when we have the ability to mark "I care
about this submodule to be always in sync with the superproject"
(thereby implementing automatic recursive checkout and perhaps
diff, among other things), we should check if the submodule in
question is marked as such and perform the current test.

How about doing something like this instead?

 unpack-trees.c |    9 +++++++++

Works here: it silences the check and allows switching branches. Still,
leaving the working tree dirty can inadvertently affect subsequent
commits. Consider the most ordinary of sequences:

$ git checkout experimental-death-ray
$ git submodules update
(return a week later, woozy from the vacation.)
$ git checkout master
(hack hack hack)
$ git commit -a -m "fixed typos"
$ git push
(Oops. You've just accidentally committed the wrong submodule heads.)

So to safely make new commits you must remember to always run "git
submodule update", or forgo use of "git commit -a", whenever submodules
might be involved.

I guess you can hack around this by excluding submodules from "commit
-a" and (for scripts) "ls-files -m" too...

Another approach is for pull, checkout etc. to automatically update the
submodule' head ref, but no more. In this case the supermodule always
sees a consistent state with traditional semantics, but the *submodule*
ends up with a dirty working tree and a head referring to a
possibly-missing commit; "git submodule update" would need to clean that up.

  Eran

On Sat, Aug 04, 2007 at 12:03:28PM -0400, Eran Tromer wrote:

Works here: it silences the check and allows switching branches. Still,
leaving the working tree dirty can inadvertently affect subsequent
commits. Consider the most ordinary of sequences:

$ git checkout experimental-death-ray
$ git submodules update
(return a week later, woozy from the vacation.)
$ git checkout master

Here, it'll warn that your submodule isn't up-to-date.

(hack hack hack)
$ git commit -a -m "fixed typos"

And if you run "git status" first, it'll tell you that the submodule
(still) isn't up-to-date.

$ git push
(Oops. You've just accidentally committed the wrong submodule heads.)

You always have to be careful when doing "git commit -a".

Another approach is for pull, checkout etc. to automatically update the
submodule' head ref, but no more.

Then everything, including "git submodule update", would assume
that the submodule is up-to-date.

skimo

On 2007-08-05 10:46, Sven Verdoolaege wrote:

quoted

$ git checkout experimental-death-ray
$ git submodules update
(return a week later, woozy from the vacation.)
$ git checkout master

Here, it'll warn that your submodule isn't up-to-date.

quoted

(hack hack hack)
$ git commit -a -m "fixed typos"

And if you run "git status" first, it'll tell you that the submodule
(still) isn't up-to-date.

quoted

$ git push
(Oops. You've just accidentally committed the wrong submodule heads.)

You always have to be careful when doing "git commit -a".

Exactly. You now have to be very careful, whereas previously
$ git checkout master && vi foo && git commit -a -m "fixed typos"
was perfectly safe.

Worse yet, it could also be a script making similar assumptions. For
example, consider the tree filter in git-filter-branch. It used to be
fine, but will now corrupt the rewritten trees when submodules are
involved. Here's the relevant code from git-filter-branch.sh:

-----------------------------------------------------------------
while read commit parents; do
...
		git read-tree -i -m $commit
...
		git checkout-index -f -u -a ||
			die "Could not checkout the index"
...
		eval "$filter_tree" < /dev/null ||
			die "tree filter failed: $filter_tree"

		git diff-index -r $commit | cut -f 2- | tr '\n' '\0' | \
			xargs -0 git update-index --add --replace --remove
...
	sh -c "$filter_commit" "git commit-tree" \
		$(git write-tree) $parentstr < ../message > ../map/$commit
done <../revs
-----------------------------------------------------------------

quoted

Another approach is for pull, checkout etc. to automatically update the
submodule' head ref, but no more.

Then everything, including "git submodule update", would assume
that the submodule is up-to-date.

With that approach, "git submodule update" would fetch the submodule's
head commit (which could be missing), and then check it against the
submodule's index (and maybe its work tree).

  Eran

On Mon, Aug 06, 2007 at 02:42:20PM -0400, Eran Tromer wrote:

On 2007-08-05 10:46, Sven Verdoolaege wrote:

quoted

You always have to be careful when doing "git commit -a".

Exactly. You now have to be very careful, whereas previously
$ git checkout master && vi foo && git commit -a -m "fixed typos"
was perfectly safe.

I don't see the difference.  If you forgot you changed something
(be it a submodule or a file) you will commit something you
didn't plan to commit.

    bash-3.00$ git init; touch a b c; git add .; git commit  -m 1
    Initialized empty Git repository in .git/
    Created initial commit 4e6da45: 1
     0 files changed, 0 insertions(+), 0 deletions(-)
     create mode 100644 a
     create mode 100644 b
     create mode 100644 c
    bash-3.00$ git checkout -b branch
    Switched to a new branch "branch"
    bash-3.00$ echo "foo" > a; git add a; git commit -m 2
    Created commit fe87123: 2
     1 files changed, 1 insertions(+), 0 deletions(-)
    bash-3.00$ echo "bar" > c
    bash-3.00$ git checkout master && echo "test" > b && git commit -a -m 'change b'
    M       c
    Switched to branch "master"
    Created commit 657c5b1: change b
     2 files changed, 2 insertions(+), 0 deletions(-)

quoted

quoted

Another approach is for pull, checkout etc. to automatically update the
submodule' head ref, but no more.

Then everything, including "git submodule update", would assume
that the submodule is up-to-date.

With that approach, "git submodule update" would fetch the submodule's
head commit (which could be missing), and then check it against the
submodule's index (and maybe its work tree).

And how is anyone supposed to figure out what HEAD the submodule's
index and working tree correspond to?
I can only hope that "git submodule update" would never blindly assume
that the submodule is clean and so the user would have to manually
sync the HEAD and the working tree.

skimo

On 2007-08-06 15:03, Sven Verdoolaege wrote:

I don't see the difference.  If you forgot you changed something
(be it a submodule or a file) you will commit something you
didn't plan to commit.

...

    bash-3.00$ git checkout master && echo "test" > b && git commit -a -m 'change b'
    M       c
    Switched to branch "master"
    Created commit 657c5b1: change b
     2 files changed, 2 insertions(+), 0 deletions(-)

Yes, you're right. (When I tried this, checkout complained about the
dirty working tree because a *merge* was needed.)

So let's try to explicitly reset the index and work tree:

$ git reset --hard master
$ vi foo
$ git commit -a -m 'fixed typos'

Oops, still a corrupt commit.

quoted

quoted

quoted

Another approach is for pull, checkout etc. to automatically update the
submodule' head ref, but no more.

Then everything, including "git submodule update", would assume
that the submodule is up-to-date.

With that approach, "git submodule update" would fetch the submodule's
head commit (which could be missing), and then check it against the
submodule's index (and maybe its work tree).

And how is anyone supposed to figure out what HEAD the submodule's
index and working tree correspond to?

What HEAD corresponds to any other dirty index or dirty working tree?
It's irrelevant and may not exist. You just have some random dirty state.

If it's the yet-to-exist submodule merging you're worried about, the
submodule's old head can be saved in ORIG_HEAD or some such during the
supermodule checkout.

I can only hope that "git submodule update" would never blindly assume
that the submodule is clean and so the user would have to manually
sync the HEAD and the working tree.

Why would it assume that? In this approach, and ignoring submodule
merging for now, "git submodule update" should mean roughly "cd
submodule && git fetch HEAD && git reset --hard HEAD". After all, this
is really the only way to end up with the prescribed commit sha1.

I agree that for safety it makes sense to warn or abort if the index
doesn't match ORIG_HEAD (saved by the supermodule checkout) or if the
index doesn't match the work tree.

  Eran

On Mon, Aug 06, 2007 at 11:24:46PM -0400, Eran Tromer wrote:

On 2007-08-06 15:03, Sven Verdoolaege wrote:

quoted

quoted

quoted

quoted

Another approach is for pull, checkout etc. to automatically update the
submodule' head ref, but no more.

Then everything, including "git submodule update", would assume
that the submodule is up-to-date.

With that approach, "git submodule update" would fetch the submodule's
head commit (which could be missing), and then check it against the
submodule's index (and maybe its work tree).

And how is anyone supposed to figure out what HEAD the submodule's
index and working tree correspond to?

What HEAD corresponds to any other dirty index or dirty working tree?
It's irrelevant and may not exist. You just have some random dirty state.

The only way to know that it's dirty is if you know the HEAD.
How can that not be relevant.

quoted

I can only hope that "git submodule update" would never blindly assume
that the submodule is clean and so the user would have to manually
sync the HEAD and the working tree.

Why would it assume that? In this approach, and ignoring submodule
merging for now, "git submodule update" should mean roughly "cd
submodule && git fetch HEAD && git reset --hard HEAD".

If you're doing that, then that is exactly what you are assuming.

After all, this
is really the only way to end up with the prescribed commit sha1.

That's the best way of losing all you precious changes in the submodule.
And there is no way to get them back!
Surely this is a lot worse than occasionally committing something you
didn't plan to commit, and only if you are performing a known "dangerous"
operation.

I agree that for safety it makes sense to warn or abort if the index
doesn't match ORIG_HEAD (saved by the supermodule checkout) or if the
index doesn't match the work tree.

You may have done several supermodule checkouts since you last changed
the submodule.

skimo

On 2007-08-07 04:51, Sven Verdoolaege wrote:

Surely this is a lot worse than occasionally committing something you
didn't plan to commit, and only if you are performing a known "dangerous"
operation.

Are you saying that
$ git reset --hard HEAD && vi foo && git commit -a
is a "known dangerous" operation that can record corrupted content even
though you didn't touch it? This is very bad news indeed! I don't see
any such warnings in the documentation.

So, when I'm sure all the edits I did in the work tree are fine, how
*do* I safely make a commit without manually inspecting the changed
files list, or manually listing the changed files for "git add", or
manually running "git submodule update", or manually checking whether
there happens to be some submodules in this project, some other such
cumbersome measure?

You may have done several supermodule checkouts since you last changed
the submodule.

True, that approach won't work. I can imagine some logic to
conditionally update ORIG_HEAD, but it gets messy and fragile. Looks
like brokenness is just inevitable when you let the state get stale and
then merrily read it out as if it's fresh.


So.... Maybe we can tackle this head-on? Let index entries be explicitly
marked as "adrift", meaning we just don't touch the work tree for these
entries -- neither reads nor writes. It's used when the piece of
content, say a submodule, is allowed to drift arbitrarily in the work
tree in a way that doesn't represent meaningful edits that should be
reflected in commits, diffs, etc.

For example:
- "git checkout" sets the "adrift" flag on all (modified?) submodules
- "git submodule update" undrifts ("moores?") the submodules
- "git commit -a" skips files that are adrift, and likewise "git add .",
  "git diff" etc. (perhaps with some warning?)
- "git add <path>" undrifts <path> and proceeds as usual
- "git status" reports drifting files as such and doesn't bother to
  check them in the work tree
- When merging into the work tree, drifting files are left as such

And why stop at submodules? If there's a large blob you don't want to
check out, just "git drift <path>" it. To set whole *directories*
adrift, we can piggybacking on the empty-directory support (i.e., add a
directory entry to the index and set it adrift). So this could be the
basis of partial-checkout support.

Does this sound reasonable?

  Eran

On Tue, Aug 07, 2007 at 09:41:34PM -0400, Eran Tromer wrote:

On 2007-08-07 04:51, Sven Verdoolaege wrote:

quoted

Surely this is a lot worse than occasionally committing something you
didn't plan to commit, and only if you are performing a known "dangerous"
operation.

Are you saying that
$ git reset --hard HEAD && vi foo && git commit -a
is a "known dangerous" operation that can record corrupted content even
though you didn't touch it?

I'm only saying that "git commit -a" will commit anything that has been
modified, so you have to be careful when using it and it just so happens
that git reset may leave submodules modified.  This should probably
be documented.
And I agree with you that this is not ideal (personally, I'd want
all checked-out submodules to get updated automatically), but it's
certainly better than your earlier proposal.

So, when I'm sure all the edits I did in the work tree are fine, how
*do* I safely make a commit without manually inspecting the changed
files list, or manually listing the changed files for "git add", or
manually running "git submodule update", or manually checking whether
there happens to be some submodules in this project, some other such
cumbersome measure?

If you've ever done a "git submodule update" in the project, you should
know that there are submodules and if you haven't then there are no
checked-out submodules that can get out of sync.
If you're talking about tools, then they should indeed be extra careful.

[another proposal]

Does this sound reasonable?

I'll leave it to others to comment on that one.

skimo