git diff-index with relative git-dir does not work

git diff-index with relative git-dir does not work

[Yasushi SHOJI]

Hi all,

I was just testing --git-dir and --work-tree options and found that
the following test code fails with the current next (2ac040d3).

	diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
	index 9df3012..1f90f45 100755
	--- a/t/t1501-worktree.sh
	+++ b/t/t1501-worktree.sh
	@@ -195,4 +195,8 @@ test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
	 	git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
	 '
	 
	+test_expect_success 'git diff-index' '
	+	git --git-dir repo.git --work-tree repo.git/work diff-index HEAD
	+'
	+
	 test_done

This is because static variable 'base' in sha1_file_name is already
assigned _before_ setup_work_tree() from cmd_diff_index() is
called. setup_work_tree() eventually chdir to the given work tree dir,
but we use the old base to generate object file path. And that cause
open(2) to fail because the object file path and the current dir is
not in sync any more.

So, is it correct to assume that we must call setup_work_tree()
_before_ any function which call getter/setter in environment.c?  This
including open_sha1_file, in this case.

Also, would it be a good idea to make all builtin command to
_explicitly_ call setup_* functions, so that we can find calling order
bug? In that case, we must change the setup functions signature to
allow marking "not interested" or something.

Any thoughts?
-- 
           yashi

Re: git diff-index with relative git-dir does not work

[Nguyen Thai Ngoc Duy]

On 2/9/10, Yasushi SHOJI <yashi@atmark-techno.com> wrote:
>  ...
>  This is because static variable 'base' in sha1_file_name is already
>  assigned _before_ setup_work_tree() from cmd_diff_index() is
>  called. setup_work_tree() eventually chdir to the given work tree dir,
>  but we use the old base to generate object file path. And that cause
>  open(2) to fail because the object file path and the current dir is
>  not in sync any more.
>
>  So, is it correct to assume that we must call setup_work_tree()
>  _before_ any function which call getter/setter in environment.c?  This
>  including open_sha1_file, in this case.

We must if gitdir is relative to cwd (and will be moved by
setup_work_tree). Or just make gitdir absolute path.

>  Also, would it be a good idea to make all builtin command to
>  _explicitly_ call setup_* functions, so that we can find calling order
>  bug?

If you agree that writing "RUN_SETUP" in git.c is explicit, then all
builtin commands do explictly call setup_*. It's about relative
directories and cwd being moved around.

>  In that case, we must change the setup functions signature to
>  allow marking "not interested" or something.

I'm not sure I get your idea.
-- 
Duy

Re: git diff-index with relative git-dir does not work

[Yasushi SHOJI]

At Tue, 9 Feb 2010 18:58:51 +0700,
Nguyen Thai Ngoc Duy wrote:
> 
> On 2/9/10, Yasushi SHOJI <yashi@atmark-techno.com> wrote:
> >  ...
> >  This is because static variable 'base' in sha1_file_name is already
> >  assigned _before_ setup_work_tree() from cmd_diff_index() is
> >  called. setup_work_tree() eventually chdir to the given work tree dir,
> >  but we use the old base to generate object file path. And that cause
> >  open(2) to fail because the object file path and the current dir is
> >  not in sync any more.
> >
> >  So, is it correct to assume that we must call setup_work_tree()
> >  _before_ any function which call getter/setter in environment.c?  This
> >  including open_sha1_file, in this case.
> 
> We must if gitdir is relative to cwd (and will be moved by
> setup_work_tree). Or just make gitdir absolute path.

ok. thanks.

> >  Also, would it be a good idea to make all builtin command to
> >  _explicitly_ call setup_* functions, so that we can find calling order
> >  bug?
> 
> If you agree that writing "RUN_SETUP" in git.c is explicit, then all
> builtin commands do explictly call setup_*. It's about relative
> directories and cwd being moved around.

oops.  I had omitted too much words.

In the diff-index case, it, indeed, has RUN_SETUP explicitly
set. however, it does not have NEED_WORK_TREE set.  And, this is
correct in the current semantics because diff-index is a tool to
compare the index and the object store. it does not need a work tree.

However, diff-index is used in describe which need a work tree if
--dirty is given.  That means that diff-index might be called
with --work-tree.

> >  In that case, we must change the setup functions signature to
> >  allow marking "not interested" or something.
> 
> I'm not sure I get your idea.

Given that in the current form of git, many built-in command is called
by many other built-in commands. It is hard to predict what is needed
and what's not.  Plus, --git-dir and --work-tree are options to git
itself not built-in's.  So, I thought it might be a good idea to call,
say, setup_work_tree_with_abs_path(), regardless of NEED_WORK_TREE, to
explicitly setup run time environment before any other part of the
code call, say, open_sha1_file.

If calling those functions are not acceptable due to speed or other
issues, making them debug / poison code might be enough.
-- 
         yashi

Re: git diff-index with relative git-dir does not work

[Nguyen Thai Ngoc Duy]

On Tue, Feb 9, 2010 at 8:10 PM, Yasushi SHOJI > In the diff-index
case, it, indeed, has RUN_SETUP explicitly
> set. however, it does not have NEED_WORK_TREE set.  And, this is
> correct in the current semantics because diff-index is a tool to
> compare the index and the object store. it does not need a work tree.

Unless --cached is given, work tree is needed. I'm not saying that
diff-index is bug-free. But the bug you described is not relevant to
this.

> However, diff-index is used in describe which need a work tree if
> --dirty is given.  That means that diff-index might be called
> with --work-tree.

Yes. And git-describe calls git-diff-index correctly, i.e. without --cached.

>> >  In that case, we must change the setup functions signature to
>> >  allow marking "not interested" or something.
>>
>> I'm not sure I get your idea.
>
> Given that in the current form of git, many built-in command is called
> by many other built-in commands. It is hard to predict what is needed
> and what's not.  Plus, --git-dir and --work-tree are options to git
> itself not built-in's.  So, I thought it might be a good idea to call,
> say, setup_work_tree_with_abs_path(), regardless of NEED_WORK_TREE, to
> explicitly setup run time environment before any other part of the
> code call, say, open_sha1_file.

The thing is not every command expect cwd to be moved to top
directory. In other words, they don't care about the prefix argument
being passed to it. So you would need go go through all commands
before doing that.

By the way, are you working on a patch for the diff-index bug?
-- 
Duy