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

From: Yasushi SHOJI <yashi@atmark-techno.com>
Date: Feb 9, 2010, 13:10 UTC
Message-ID: <87wrymwo5a.wl@dns1.atmark-techno.com>
In-Reply-To: <fcaeb9bf1002090358v3d7f69d5ra80c186d30a1304d@mail.gmail.com>

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