git-diff-tree does not use alternate objects for submodules

git-diff-tree does not use alternate objects for submodules

[Orgad and Raizel Shaneh]

I have a project with a submodule. Both have objects/info/alternate
(different ones).

After running 'git gc', running gitk on the superproject results in:
  Submodule sub 227e2b5...d8597e2 (commits not present)

This is the git-diff-tree command that gitk runs:
  git diff-tree -r -p --textconv --submodule -C --cc --no-commit-id
-U6 --root d117679ef4a4fb4143401d3a4bc4a484c9bf76ff

I'm using msysGit 1.7.10

- Orgad

Re: git-diff-tree does not use alternate objects for submodules

[Junio C Hamano]

Orgad and Raizel Shaneh <orgads@gmail.com> writes:

> I have a project with a submodule. Both have objects/info/alternate
> (different ones).
>
> After running 'git gc', running gitk on the superproject results in:
>   Submodule sub 227e2b5...d8597e2 (commits not present)

When "--submodule" option was implemented in 752c0c, it was done with an
premature and incomplete optimization, and I think you are seeing an
unfortunate side effect of it. The code attempts to link the object store
of the submodule repository into the in-core representation of the object
store of the superproject (in submodule.c::add_submodule_odb()), but does
not do a good job of it.  It does not take alternates into account, and
who knows what else is missing.  Sigh...

The right approach to implement this feature would have been to fork a
child process and perform the submodule operation inside the child, which
will chdir into the submodule and treat as if it is a freestanding git
repository, without contaminating the superproject process.

For now, an easiest workaround would be to rephrase the error message to
"commits not present" to "commit not present or missing", or something.

Re: git-diff-tree does not use alternate objects for submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> For now, an easiest workaround would be to rephrase the error message to
> "commits not present" to "commit not present or missing", or something.

I meant s/or missing/or borrowed/;  "not present" and "missing" are the
same thing ;-)

Re: git-diff-tree does not use alternate objects for submodules

[Heiko Voigt]

Hi,

On Tue, May 08, 2012 at 08:36:36AM -0700, Junio C Hamano wrote:
> Orgad and Raizel Shaneh <orgads@gmail.com> writes:
> 
> > I have a project with a submodule. Both have objects/info/alternate
> > (different ones).
> >
> > After running 'git gc', running gitk on the superproject results in:
> > ?? Submodule sub 227e2b5...d8597e2 (commits not present)
> 
> When "--submodule" option was implemented in 752c0c, it was done with an
> premature and incomplete optimization, and I think you are seeing an
> unfortunate side effect of it. The code attempts to link the object store
> of the submodule repository into the in-core representation of the object
> store of the superproject (in submodule.c::add_submodule_odb()), but does
> not do a good job of it.  It does not take alternates into account, and
> who knows what else is missing.  Sigh...
> 
> The right approach to implement this feature would have been to fork a
> child process and perform the submodule operation inside the child, which
> will chdir into the submodule and treat as if it is a freestanding git
> repository, without contaminating the superproject process.
> 
> For now, an easiest workaround would be to rephrase the error message to
> "commits not present" to "commit not present or missing", or something.

I will have a look if I can come up with something that reads the
submodules alternate config and uses it. Do you have other config
related things in mind that might be missing?

Cheers Heiko

Re: git-diff-tree does not use alternate objects for submodules

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:
> On Tue, May 08, 2012 at 08:36:36AM -0700, Junio C Hamano wrote:
> ...
>> ... The code attempts to link the object store
>> of the submodule repository into the in-core representation of the object
>> store of the superproject (in submodule.c::add_submodule_odb()), but does
>> not do a good job of it.  It does not take alternates into account, and
>> who knows what else is missing.  Sigh...
>> 
>> The right approach to implement this feature would have been to fork a
>> child process and perform the submodule operation inside the child, which
>> will chdir into the submodule and treat as if it is a freestanding git
>> repository, without contaminating the superproject process.
>
> I will have a look if I can come up with something that reads the
> submodules alternate config and uses it. Do you have other config
> related things in mind that might be missing?

No, I do not, and that is exactly the point.

Making the process that works in the top-level superproject to imitate
what would happen if the processing happened inside the submodule is what
invited a bug like this.  Who knows what other discrepancies remain there.

If we forked a separate process, and made it to chdir to the submodule
tree, and had it do its usual thing there, we do not have to worry about
how good the imitation is, exactly because there won't be any imitation.
There will only be a git processing happening in the usual way inside the
submodule directory, as if the end user cd'ed there and typed the "git
status" command to see if the HEAD matches the given commit or if the
working tree is dirty.

Re: git-diff-tree does not use alternate objects for submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Heiko Voigt <hvoigt@hvoigt.net> writes:
> ...
>> I will have a look if I can come up with something that reads the
>> submodules alternate config and uses it. Do you have other config
>> related things in mind that might be missing?
>
> No, I do not, and that is exactly the point.
>
> Making the process that works in the top-level superproject to imitate
> what would happen if the processing happened inside the submodule is what
> invited a bug like this.  Who knows what other discrepancies remain there.
>
> If we forked a separate process,...

Having said all that, we seem to have come too far and it is probably too
painful to revert the approach to contaminate the obj_hash (in object.c),
the set of refs (in refs.c) and the like in the top-level superproject
process with data borrowed from submodules repository [*1*].  So not only
I do not mind seeing you try solving it inside the superproject process, I
would appreciate and encourage the attempt.  One thing to be careful about
is relative paths stored in the objects/info/alternates; they are relative
to the object database of the repository the "alternates" is specified,
not relative to the superproject that happens to contain the submodule.

Thanks.


[Footnote]

*1* This is a wrong approach not only because it will invite this kind of
bug, but also because it will bloat the process working on the
superproject.  When taken alone, either of the superproject or the
submodule repository may be of comfortably workable size, but a single
process that is working on the superproject may ran out of a per-process
resource limit if it is made to also slurp data for the submodules,
especially when working on a project with dozens of submodules.

Re: git-diff-tree does not use alternate objects for submodules

[Heiko Voigt]

Hi,

On Wed, May 09, 2012 at 03:53:34PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Heiko Voigt <hvoigt@hvoigt.net> writes:
> > ...
> >> I will have a look if I can come up with something that reads the
> >> submodules alternate config and uses it. Do you have other config
> >> related things in mind that might be missing?
> >
> > No, I do not, and that is exactly the point.
> >
> > Making the process that works in the top-level superproject to imitate
> > what would happen if the processing happened inside the submodule is what
> > invited a bug like this.  Who knows what other discrepancies remain there.
> >
> > If we forked a separate process,...
> 
> Having said all that, we seem to have come too far and it is probably too
> painful to revert the approach to contaminate the obj_hash (in object.c),
> the set of refs (in refs.c) and the like in the top-level superproject
> process with data borrowed from submodules repository [*1*].  So not only
> I do not mind seeing you try solving it inside the superproject process, I
> would appreciate and encourage the attempt.  One thing to be careful about
> is relative paths stored in the objects/info/alternates; they are relative
> to the object database of the repository the "alternates" is specified,
> not relative to the superproject that happens to contain the submodule.
> 
> Thanks.

Here is the simplest approach I could think of (not sure if its maybe
too simple). On first sight it seems to work (even if I exchange the
absolute path in alternates with a relative one.

Please see below.

Cheers Heiko

I used the following setup to test:

mkdir sub_alt &&
(cd sub_alt &&
git init &&
echo a >a &&
git add a &&
git commit -m a) &&
mkdir super &&
(cd super &&
git clone -s ../sub_alt sub &&
git init &&
git add sub &&
git commit -m "sub a") &&
(cd sub_alt &&
echo b >b &&
git add b &&
git commit -m b) &&
(cd super &&
	(cd sub &&
	 git fetch &&
	 git checkout origin/master) &&
 git diff --submodule
)


-8<--
From: Heiko Voigt <hvoigt@hvoigt.net>
Subject: [PATCH] teach add_submodule_odb() to look for alternates

Since we allow to link other object databases when loading a submodules
database we should also load possible alternates.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 cache.h     | 1 +
 sha1_file.c | 3 +--
 submodule.c | 3 +++
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index e14ffcd..cc5048c 100644
--- a/cache.h
+++ b/cache.h
@@ -947,6 +947,7 @@ extern struct alternate_object_database {
 	char base[FLEX_ARRAY]; /* more */
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
+extern void read_info_alternates(const char * relative_base, int depth);
 extern void add_to_alternates_file(const char *reference);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern void foreach_alt_odb(alt_odb_fn, void*);
diff --git a/sha1_file.c b/sha1_file.c
index 3c4f165..4ccaf7a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -229,7 +229,6 @@ char *sha1_pack_index_name(const unsigned char *sha1)
 struct alternate_object_database *alt_odb_list;
 static struct alternate_object_database **alt_odb_tail;
 
-static void read_info_alternates(const char * alternates, int depth);
 static int git_open_noatime(const char *name);
 
 /*
@@ -354,7 +353,7 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 	}
 }
 
-static void read_info_alternates(const char * relative_base, int depth)
+void read_info_alternates(const char * relative_base, int depth)
 {
 	char *map;
 	size_t mapsz;
diff --git a/submodule.c b/submodule.c
index 784b580..959d349 100644
--- a/submodule.c
+++ b/submodule.c
@@ -63,6 +63,9 @@ static int add_submodule_odb(const char *path)
 	alt_odb->name[40] = '\0';
 	alt_odb->name[41] = '\0';
 	alt_odb_list = alt_odb;
+
+	/* add possible alternates from the submodule */
+	read_info_alternates(objects_directory.buf, 0);
 	prepare_alt_odb();
 done:
 	strbuf_release(&objects_directory);
-- 
1.7.10.1.491.gc66153e

Re: git-diff-tree does not use alternate objects for submodules

[Heiko Voigt]

On Sun, May 13, 2012 at 07:23:25PM +0200, Heiko Voigt wrote:
> Here is the simplest approach I could think of (not sure if its maybe
> too simple). On first sight it seems to work (even if I exchange the
> absolute path in alternates with a relative one.
> 
> Please see below.
> 
> Cheers Heiko
> 
> I used the following setup to test:
> 
> mkdir sub_alt &&
> (cd sub_alt &&
> git init &&
> echo a >a &&
> git add a &&
> git commit -m a) &&
> mkdir super &&
> (cd super &&
> git clone -s ../sub_alt sub &&
> git init &&
> git add sub &&
> git commit -m "sub a") &&
> (cd sub_alt &&
> echo b >b &&
> git add b &&
> git commit -m b) &&
> (cd super &&
> 	(cd sub &&
> 	 git fetch &&
> 	 git checkout origin/master) &&
>  git diff --submodule
> )

Junio in case you decide to queue this: I would like to rework the above
code into a proper testcase and add that to the patch. The previous
patch is the state as far as I got today so interested people can test
whether it fixes their issue.

Cheers Heiko

[PATCH v2] teach add_submodule_odb() to look for alternates

[Heiko Voigt]

Since we allow to link other object databases when loading a submodules
database we should also load possible alternates.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
I had a look over the code path in read_info_alternates() and it seems
that it uses the relative_base parameter for relative paths in the
alternates file which is the correct behavior.

So for me this looks like the correct solution. The testsuite passes.

Cheers Heiko

 cache.h                          |  1 +
 sha1_file.c                      |  3 +--
 submodule.c                      |  3 +++
 t/t4041-diff-submodule-option.sh | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index e14ffcd..cc5048c 100644
--- a/cache.h
+++ b/cache.h
@@ -947,6 +947,7 @@ extern struct alternate_object_database {
 	char base[FLEX_ARRAY]; /* more */
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
+extern void read_info_alternates(const char * relative_base, int depth);
 extern void add_to_alternates_file(const char *reference);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern void foreach_alt_odb(alt_odb_fn, void*);
diff --git a/sha1_file.c b/sha1_file.c
index 3c4f165..4ccaf7a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -229,7 +229,6 @@ char *sha1_pack_index_name(const unsigned char *sha1)
 struct alternate_object_database *alt_odb_list;
 static struct alternate_object_database **alt_odb_tail;
 
-static void read_info_alternates(const char * alternates, int depth);
 static int git_open_noatime(const char *name);
 
 /*
@@ -354,7 +353,7 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 	}
 }
 
-static void read_info_alternates(const char * relative_base, int depth)
+void read_info_alternates(const char * relative_base, int depth)
 {
 	char *map;
 	size_t mapsz;
diff --git a/submodule.c b/submodule.c
index 784b580..959d349 100644
--- a/submodule.c
+++ b/submodule.c
@@ -63,6 +63,9 @@ static int add_submodule_odb(const char *path)
 	alt_odb->name[40] = '\0';
 	alt_odb->name[41] = '\0';
 	alt_odb_list = alt_odb;
+
+	/* add possible alternates from the submodule */
+	read_info_alternates(objects_directory.buf, 0);
 	prepare_alt_odb();
 done:
 	strbuf_release(&objects_directory);
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index bf9a752..6c01d0c 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -458,4 +458,38 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'diff --submodule with objects referenced by alternates' '
+	mkdir sub_alt &&
+	(cd sub_alt &&
+		git init &&
+		echo a >a &&
+		git add a &&
+		git commit -m a
+	) &&
+	mkdir super &&
+	(cd super &&
+		git clone -s ../sub_alt sub &&
+		git init &&
+		git add sub &&
+		git commit -m "sub a"
+	) &&
+	(cd sub_alt &&
+		sha1_before=$(git rev-parse --short HEAD)
+		echo b >b &&
+		git add b &&
+		git commit -m b
+		sha1_after=$(git rev-parse --short HEAD)
+		echo "Submodule sub $sha1_before..$sha1_after:
+  > b" >../expected
+	) &&
+	(cd super &&
+		(cd sub &&
+			git fetch &&
+			git checkout origin/master
+		) &&
+		git diff --submodule > ../actual
+	)
+	test_cmp expected actual
+'
+
 test_done
-- 
1.7.10.1.488.ga84c0c8

Re: git-diff-tree does not use alternate objects for submodules

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> Here is the simplest approach I could think of (not sure if its maybe
> too simple). On first sight it seems to work (even if I exchange the
> absolute path in alternates with a relative one.

Conceptually looks very sound and trivially correct as long as the
objets_directory.buf is always absolute path (I didn't check, and I do not
offhand know if read_info_alternates() was designed to work when called
with a relative path in relative_base).

Re: git-diff-tree does not use alternate objects for submodules

[Heiko Voigt]

Hi,

On Mon, May 14, 2012 at 09:49:28AM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > Here is the simplest approach I could think of (not sure if its maybe
> > too simple). On first sight it seems to work (even if I exchange the
> > absolute path in alternates with a relative one.
> 
> Conceptually looks very sound and trivially correct as long as the
> objets_directory.buf is always absolute path (I didn't check, and I do not
> offhand know if read_info_alternates() was designed to work when called
> with a relative path in relative_base).

It looks quite so:

In read_info_alternates() the alternates file underneath the passed
relative_base is opened. It then passes it on to link_alt_odb_entries()
which itself passes it unmodified to link_alt_odb_entry().

link_alt_odb_entry() uses realpath to combine relative entries with
relative_base before recursively passing the entry to
read_info_alternates().

For entries with absolute path relative_base does not seem to be
relevant.

Cheers Heiko

Re: git-diff-tree does not use alternate objects for submodules

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> link_alt_odb_entry() uses realpath to combine relative entries with
> relative_base before recursively passing the entry to
> read_info_alternates().
>
> For entries with absolute path relative_base does not seem to be
> relevant.

I wasn't worried about "entries with absolute paths".  I was worried about
relative_base that the caller passes may not be absolute but relative to
something (perhaps the processe's getcwd()); it seems that the call to
real_path() to turn it into an absolute one at the very beginning of the
link_alt_odb_entry() function does indeed take care of that case.

Thanks.