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

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

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

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.

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

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

Hi,

On Tue, May 08, 2012 at 08:36:36AM -0700, Junio C Hamano wrote:

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

Heiko Voigt <hvoigt@hvoigt.net> writes:

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.

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

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.

Hi,

On Wed, May 09, 2012 at 03:53:34PM -0700, Junio C Hamano 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
)

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

On Sun, May 13, 2012 at 07:23:25PM +0200, Heiko Voigt wrote:

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

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

Heiko Voigt <hvoigt@hvoigt.net> writes:

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

Hi,

On Mon, May 14, 2012 at 09:49:28AM -0700, Junio C Hamano wrote:

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

Heiko Voigt <hvoigt@hvoigt.net> writes:

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.