{"thread":{"id":"22586","subject":"git diff-index with relative git-dir does not work","startedAt":"2010-02-09T11:05:28Z","lastAt":"2010-02-11T10:35:25Z","messageCount":4,"participants":["Yasushi SHOJI","Nguyen Thai Ngoc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"134027","messageId":"871vguy8hz.wl@dns1.atmark-techno.com","threadId":"22586","inReplyTo":null,"subject":"git diff-index with relative git-dir does not work","fromName":"Yasushi SHOJI","fromEmail":"yashi@atmark-techno.com","sentAt":"2010-02-09T11:05:28Z","receivedAt":"2010-02-09T11:05:28Z","isPatch":false,"sender":{"key":"yashi@atmark-techno.com","avatar":"https://gravatar.com/avatar/4817e8703ac4379935834d87453faa9d0c94b9dc19d83fcc54c67875eb133e59?d=mp&s=160"},"body":"Hi all,\n\nI was just testing --git-dir and --work-tree options and found that\nthe following test code fails with the current next (2ac040d3).\n\n\tdiff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh\n\tindex 9df3012..1f90f45 100755\n\t--- a/t/t1501-worktree.sh\n\t+++ b/t/t1501-worktree.sh\n\t@@ -195,4 +195,8 @@ test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '\n\t \tgit --git-dir=\"$(pwd)//repo.git\" --work-tree=\"$(pwd)\" add dummy_file\n\t '\n\t \n\t+test_expect_success 'git diff-index' '\n\t+\tgit --git-dir repo.git --work-tree repo.git/work diff-index HEAD\n\t+'\n\t+\n\t test_done\n\nThis is because static variable 'base' in sha1_file_name is already\nassigned _before_ setup_work_tree() from cmd_diff_index() is\ncalled. setup_work_tree() eventually chdir to the given work tree dir,\nbut we use the old base to generate object file path. And that cause\nopen(2) to fail because the object file path and the current dir is\nnot in sync any more.\n\nSo, is it correct to assume that we must call setup_work_tree()\n_before_ any function which call getter/setter in environment.c?  This\nincluding open_sha1_file, in this case.\n\nAlso, would it be a good idea to make all builtin command to\n_explicitly_ call setup_* functions, so that we can find calling order\nbug? In that case, we must change the setup functions signature to\nallow marking \"not interested\" or something.\n\nAny thoughts?\n-- \n           yashi\n"},{"id":"134028","messageId":"fcaeb9bf1002090358v3d7f69d5ra80c186d30a1304d@mail.gmail.com","threadId":"22586","inReplyTo":"871vguy8hz.wl@dns1.atmark-techno.com","subject":"Re: git diff-index with relative git-dir does not work","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-02-09T11:58:51Z","receivedAt":"2010-02-09T11:58:51Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 2/9/10, Yasushi SHOJI <yashi@atmark-techno.com> wrote:\n>  ...\n>  This is because static variable 'base' in sha1_file_name is already\n>  assigned _before_ setup_work_tree() from cmd_diff_index() is\n>  called. setup_work_tree() eventually chdir to the given work tree dir,\n>  but we use the old base to generate object file path. And that cause\n>  open(2) to fail because the object file path and the current dir is\n>  not in sync any more.\n>\n>  So, is it correct to assume that we must call setup_work_tree()\n>  _before_ any function which call getter/setter in environment.c?  This\n>  including open_sha1_file, in this case.\n\nWe must if gitdir is relative to cwd (and will be moved by\nsetup_work_tree). Or just make gitdir absolute path.\n\n>  Also, would it be a good idea to make all builtin command to\n>  _explicitly_ call setup_* functions, so that we can find calling order\n>  bug?\n\nIf you agree that writing \"RUN_SETUP\" in git.c is explicit, then all\nbuiltin commands do explictly call setup_*. It's about relative\ndirectories and cwd being moved around.\n\n>  In that case, we must change the setup functions signature to\n>  allow marking \"not interested\" or something.\n\nI'm not sure I get your idea.\n-- \nDuy\n"},{"id":"134035","messageId":"87wrymwo5a.wl@dns1.atmark-techno.com","threadId":"22586","inReplyTo":"fcaeb9bf1002090358v3d7f69d5ra80c186d30a1304d@mail.gmail.com","subject":"Re: git diff-index with relative git-dir does not work","fromName":"Yasushi SHOJI","fromEmail":"yashi@atmark-techno.com","sentAt":"2010-02-09T13:10:25Z","receivedAt":"2010-02-09T13:10:25Z","isPatch":false,"sender":{"key":"yashi@atmark-techno.com","avatar":"https://gravatar.com/avatar/4817e8703ac4379935834d87453faa9d0c94b9dc19d83fcc54c67875eb133e59?d=mp&s=160"},"body":"At Tue, 9 Feb 2010 18:58:51 +0700,\nNguyen Thai Ngoc Duy wrote:\n> \n> On 2/9/10, Yasushi SHOJI <yashi@atmark-techno.com> wrote:\n> >  ...\n> >  This is because static variable 'base' in sha1_file_name is already\n> >  assigned _before_ setup_work_tree() from cmd_diff_index() is\n> >  called. setup_work_tree() eventually chdir to the given work tree dir,\n> >  but we use the old base to generate object file path. And that cause\n> >  open(2) to fail because the object file path and the current dir is\n> >  not in sync any more.\n> >\n> >  So, is it correct to assume that we must call setup_work_tree()\n> >  _before_ any function which call getter/setter in environment.c?  This\n> >  including open_sha1_file, in this case.\n> \n> We must if gitdir is relative to cwd (and will be moved by\n> setup_work_tree). Or just make gitdir absolute path.\n\nok. thanks.\n\n> >  Also, would it be a good idea to make all builtin command to\n> >  _explicitly_ call setup_* functions, so that we can find calling order\n> >  bug?\n> \n> If you agree that writing \"RUN_SETUP\" in git.c is explicit, then all\n> builtin commands do explictly call setup_*. It's about relative\n> directories and cwd being moved around.\n\noops.  I had omitted too much words.\n\nIn the diff-index case, it, indeed, has RUN_SETUP explicitly\nset. however, it does not have NEED_WORK_TREE set.  And, this is\ncorrect in the current semantics because diff-index is a tool to\ncompare the index and the object store. it does not need a work tree.\n\nHowever, diff-index is used in describe which need a work tree if\n--dirty is given.  That means that diff-index might be called\nwith --work-tree.\n\n> >  In that case, we must change the setup functions signature to\n> >  allow marking \"not interested\" or something.\n> \n> I'm not sure I get your idea.\n\nGiven that in the current form of git, many built-in command is called\nby many other built-in commands. It is hard to predict what is needed\nand what's not.  Plus, --git-dir and --work-tree are options to git\nitself not built-in's.  So, I thought it might be a good idea to call,\nsay, setup_work_tree_with_abs_path(), regardless of NEED_WORK_TREE, to\nexplicitly setup run time environment before any other part of the\ncode call, say, open_sha1_file.\n\nIf calling those functions are not acceptable due to speed or other\nissues, making them debug / poison code might be enough.\n-- \n         yashi\n"},{"id":"134216","messageId":"fcaeb9bf1002110235p7fdb50a7we41715b795f76b99@mail.gmail.com","threadId":"22586","inReplyTo":"87wrymwo5a.wl@dns1.atmark-techno.com","subject":"Re: git diff-index with relative git-dir does not work","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-02-11T10:35:25Z","receivedAt":"2010-02-11T10:35:25Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 9, 2010 at 8:10 PM, Yasushi SHOJI > In the diff-index\ncase, it, indeed, has RUN_SETUP explicitly\n> set. however, it does not have NEED_WORK_TREE set.  And, this is\n> correct in the current semantics because diff-index is a tool to\n> compare the index and the object store. it does not need a work tree.\n\nUnless --cached is given, work tree is needed. I'm not saying that\ndiff-index is bug-free. But the bug you described is not relevant to\nthis.\n\n> However, diff-index is used in describe which need a work tree if\n> --dirty is given.  That means that diff-index might be called\n> with --work-tree.\n\nYes. And git-describe calls git-diff-index correctly, i.e. without --cached.\n\n>> >  In that case, we must change the setup functions signature to\n>> >  allow marking \"not interested\" or something.\n>>\n>> I'm not sure I get your idea.\n>\n> Given that in the current form of git, many built-in command is called\n> by many other built-in commands. It is hard to predict what is needed\n> and what's not.  Plus, --git-dir and --work-tree are options to git\n> itself not built-in's.  So, I thought it might be a good idea to call,\n> say, setup_work_tree_with_abs_path(), regardless of NEED_WORK_TREE, to\n> explicitly setup run time environment before any other part of the\n> code call, say, open_sha1_file.\n\nThe thing is not every command expect cwd to be moved to top\ndirectory. In other words, they don't care about the prefix argument\nbeing passed to it. So you would need go go through all commands\nbefore doing that.\n\nBy the way, are you working on a patch for the diff-index bug?\n-- \nDuy\n"}]}