threads / discuss / 22586

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

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

## tl;dr

4 messages between Feb 9, 2010 and Feb 11, 2010.

replies: 3people: 2as markdown or json

Yasushi SHOJI· Feb 9, 2010, 11:05 UTC · lore

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

Nguyen Thai Ngoc Duy· Feb 9, 2010, 11:58 UTC · re: Yasushi SHOJI · lore

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

On 2/9/10, Yasushi SHOJI <yashi@atmark-techno.com> wrote:

Show 11 quoted lines

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

Yasushi SHOJI· Feb 9, 2010, 13:10 UTC · re: Nguyen Thai Ngoc Duy · lore

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

At Tue, 9 Feb 2010 18:58:51 +0700, Nguyen Thai Ngoc Duy wrote:

Show 16 quoted lines

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

Show 7 quoted lines

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

Nguyen Thai Ngoc Duy· Feb 11, 2010, 10:35 UTC · re: Yasushi SHOJI · lore

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

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.

Show 12 quoted lines

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