{"thread":{"id":"15991","subject":"[PATCH] rehabilitate 'git index-pack' inside the object store","startedAt":"2008-10-21T01:17:07Z","lastAt":"2008-10-28T13:52:25Z","messageCount":11,"participants":["Nicolas Pitre","Junio C Hamano","Nguyen Thai Ngoc Duy","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"93571","messageId":"alpine.LFD.2.00.0810202110380.26244@xanadu.home","threadId":"15991","inReplyTo":null,"subject":"[PATCH] rehabilitate 'git index-pack' inside the object store","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-21T01:17:07Z","receivedAt":"2008-10-21T01:17:07Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Before commit d0b92a3f6e it was possible to run 'git index-pack'\ndirectly in the .git/objects/pack/ directory. Restore that ability.\n\nSigned-off-by: Nicolas Pitre \n---\n\ndiff --git a/index-pack.c b/index-pack.c\nindex 0a917d7..79f6fd6 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -878,10 +877,26 @@ int main(int argc, char **argv)\n \tchar *index_name_buf = NULL, *keep_name_buf = NULL;\n \tstruct pack_idx_entry **idx_objects;\n \tunsigned char pack_sha1[20];\n-\tint nongit = 0;\n \n-\tsetup_git_directory_gently(&nongit);\n-\tgit_config(git_index_pack_config, NULL);\n+\t/*\n+\t * We wish to read the repository's config file if any, and\n+\t * for that it is necessary to call setup_git_directory_gently().\n+\t * However if the cwd was inside .git/objects/pack/ then we need\n+\t * to go back there or all the pack name arguments will be wrong.\n+\t * And in that case we cannot rely on any prefix returned by \n+\t * setup_git_directory_gently() either.\n+\t */\n+\t{\n+\t\tchar cwd[PATH_MAX+1];\n+\t\tint nongit;\n+\n+\t\tif (!getcwd(cwd, sizeof(cwd)-1))\n+\t\t\tdie(\"Unable to get current working directory\");\n+\t\tsetup_git_directory_gently(&nongit);\n+\t\tgit_config(git_index_pack_config, NULL);\n+\t\tif (chdir(cwd))\n+\t\t\tdie(\"Cannot come back to cwd\");\n+\t}\n \n \tfor (i = 1; i < argc; i++) {\n \t\tchar *arg = argv[i];\n"},{"id":"93575","messageId":"7vzlkyzgiw.fsf@gitster.siamese.dyndns.org","threadId":"15991","inReplyTo":"alpine.LFD.2.00.0810202110380.26244@xanadu.home","subject":"[PATCH] (squash) add index-pack with git-dir test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-21T05:03:19Z","receivedAt":"2008-10-21T05:03:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This tries to make sure that the recent fix to d0b92a3 (index-pack: setup\ngit repository, 2008-08-26) does not introduce regression, and protects\nthe fix from getting broken again.\n\nSigned-off-by: Junio C Hamano \n---\n\n * And unfortunately the first one fails with the fix and succeeds\n without; the second one succeeds with your fix and fails without.\n\n t/t5302-pack-index.sh | 20 ++++++++++++++++++++\n 1 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex 6424db1..2edab73 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -177,4 +177,24 @@ test_expect_success \\\n \".git/objects/pack/pack-${pack1}.pack\" 2>&1) &&\n echo \"$err\" | grep \"CRC mismatch\"'\n \n+test_expect_success 'running index-pack from a subdirectory' '\n+ mkdir subdir &&\n+ cp test-1-${pack1}.pack subdir/. &&\n+ (\n+\tcd subdir &&\n+\tgit index-pack test-1-${pack1}.pack\n+ ) &&\n+ test -f subdir/test-1-${pack1}.idx\n+'\n+\n+test_expect_success 'running index-pack in the object store' '\n+ rm -f .git/objects/pack/* &&\n+ cp test-1-${pack1}.pack .git/objects/pack/pack-${pack1}.pack &&\n+ (\n+\tcd .git/objects/pack\n+\tgit index-pack pack-${pack1}.pack\n+ ) &&\n+ test -f .git/objects/pack/pack-${pack1}.idx\n+'\n+\n test_done\n-- \n1.6.0.2.588.g31023\n"},{"id":"93606","messageId":"fcaeb9bf0810210757w1c14e0a3x1eb61a589a089f10@mail.gmail.com","threadId":"15991","inReplyTo":"alpine.LFD.2.00.0810202110380.26244@xanadu.home","subject":"Re: [PATCH] rehabilitate 'git index-pack' inside the object store","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-10-21T14:57:04Z","receivedAt":"2008-10-21T14:57:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 10/21/08, Nicolas Pitre wrote:\n> Before commit d0b92a3f6e it was possible to run 'git index-pack'\n> directly in the .git/objects/pack/ directory. Restore that ability.\n\nI am sorry I did not catch this in the first place. While the fix\nshould be fine for \"git index-pack\". I wonder what can happen for\nother setup_*_gently()'s consumers. Other commands may be affected too\n(e.g. running \"git apply\" or \"git bundle\" from inside .git). Maybe we\nshould let setup_*_gently() do read-only stuff only and let its\nconsumers to handle cwd. I recall Jeff has plan about worktree setup\nrework, though could not find the thread. Will think more of it\ntomorrow.\n-- \nDuy\n"},{"id":"93607","messageId":"20081021150206.GA20263@coredump.intra.peff.net","threadId":"15991","inReplyTo":"fcaeb9bf0810210757w1c14e0a3x1eb61a589a089f10@mail.gmail.com","subject":"Re: [PATCH] rehabilitate 'git index-pack' inside the object store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-21T15:02:07Z","receivedAt":"2008-10-21T15:02:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 21, 2008 at 09:57:04PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> (e.g. running \"git apply\" or \"git bundle\" from inside .git). Maybe we\n> should let setup_*_gently() do read-only stuff only and let its\n> consumers to handle cwd. I recall Jeff has plan about worktree setup\n> rework, though could not find the thread. Will think more of it\n> tomorrow.\n\nIf by \"plan\" you mean \"intense desire to see it fixed\", then yes, I have\na plan. But there isn't a concrete plan yet, and I'm not sure when I'll\nget to it. I have a vague assumption that we should move in the\ndirection of just setting up as much of the environment as possible\n(finding git dir, finding work tree, reading config, etc) as soon as\npossible, but not producing any error messages or dying as a result.\nThen those commands which want to enforce that some setup has occurred\ncan easily do so.\n\n-Peff\n"},{"id":"93609","messageId":"alpine.LFD.2.00.0810211153130.26244@xanadu.home","threadId":"15991","inReplyTo":"fcaeb9bf0810210757w1c14e0a3x1eb61a589a089f10@mail.gmail.com","subject":"Re: [PATCH] rehabilitate 'git index-pack' inside the object store","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-21T15:54:16Z","receivedAt":"2008-10-21T15:54:16Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 21 Oct 2008, Nguyen Thai Ngoc Duy wrote:\n\n> On 10/21/08, Nicolas Pitre wrote:\n> > Before commit d0b92a3f6e it was possible to run 'git index-pack'\n> > directly in the .git/objects/pack/ directory. Restore that ability.\n> \n> I am sorry I did not catch this in the first place. While the fix\n> should be fine for \"git index-pack\". I wonder what can happen for\n> other setup_*_gently()'s consumers. Other commands may be affected too\n> (e.g. running \"git apply\" or \"git bundle\" from inside .git).\n\nNormally you should not be running such commands inside the .git \ndirectory. The index-pack command is a bit special in that regard.\n\n\nNicolas\n"},{"id":"93614","messageId":"alpine.DEB.1.00.0810211856090.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15991","inReplyTo":"fcaeb9bf0810210757w1c14e0a3x1eb61a589a089f10@mail.gmail.com","subject":"Re: [PATCH] rehabilitate 'git index-pack' inside the object store","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-21T17:02:48Z","receivedAt":"2008-10-21T17:02:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 21 Oct 2008, Nguyen Thai Ngoc Duy wrote:\n\n> Maybe we should let setup_*_gently() do read-only stuff only and let its \n> consumers to handle cwd.\n\nI think that makes sense. This would allow setup*gently() to be much \ncleaner, and only setup_git_directory() itself would do the \nchdir(worktree).\n\nHowever, let's think this really through this time, so that that darned \nworktree stuff is fixed for good.\n\nSo I propose this change in semantics:\n\n- setup_git_directory_gently(): rename to discover_git_directory(), \n and avoid any chdir() at all.\n- setup_git_directory(): keep the semantics that it chdir()s to the\n worktree, or to the git directory for bare repositories.\n\nUsing _gently() even for RUN_SETUP builtins should solve the long standing \npager problem, too.\n\nCiao,\nDscho\n"},{"id":"93616","messageId":"20081021174303.GA25827@coredump.intra.peff.net","threadId":"15991","inReplyTo":"alpine.DEB.1.00.0810211856090.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH] rehabilitate 'git index-pack' inside the object store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-21T17:43:04Z","receivedAt":"2008-10-21T17:43:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 21, 2008 at 07:02:48PM +0200, Johannes Schindelin wrote:\n\n> So I propose this change in semantics:\n> \n> - setup_git_directory_gently(): rename to discover_git_directory(), \n> and avoid any chdir() at all.\n> - setup_git_directory(): keep the semantics that it chdir()s to the\n> worktree, or to the git directory for bare repositories.\n> \n> Using _gently() even for RUN_SETUP builtins should solve the long standing \n> pager problem, too.\n\nI'm not sure there aren't hidden problems lurking in that strategy\n(every time I look at this area of code, something unexpected prevents\nwhat I think should Just Work from Just Working), but I think that is a\npromising direction to go for clearing up some of the long-standing\nissues.\n\nI think you will need to do something for a few commands which use\n_gently() but then, if we _are_ in a git repo, assume we are chdir'd\nproperly.\n\nWe may also be able to just call setup_git_directory_gently() as the\nfirst thing in the wrapper, which should help us more consistently find\nconfig before the 'exec' stage (see 4e10738a for a discussion of some of\nthe issues).\n\n-Peff\n"},{"id":"93618","messageId":"alpine.DEB.1.00.0810211955250.22125@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15991","inReplyTo":"20081021174303.GA25827@coredump.intra.peff.net","subject":"new plan for cleaning up the worktree mess, was Re: [PATCH] rehabilitate 'git index-pack' inside the object store","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-10-21T17:59:12Z","receivedAt":"2008-10-21T17:59:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 21 Oct 2008, Jeff King wrote:\n\n> On Tue, Oct 21, 2008 at 07:02:48PM +0200, Johannes Schindelin wrote:\n> \n> > So I propose this change in semantics:\n> > \n> > - setup_git_directory_gently(): rename to discover_git_directory(), \n> > and avoid any chdir() at all.\n> > - setup_git_directory(): keep the semantics that it chdir()s to the\n> > worktree, or to the git directory for bare repositories.\n> > \n> > Using _gently() even for RUN_SETUP builtins should solve the long \n> > standing pager problem, too.\n> \n> I'm not sure there aren't hidden problems lurking in that strategy \n> (every time I look at this area of code, something unexpected prevents \n> what I think should Just Work from Just Working), but I think that is a \n> promising direction to go for clearing up some of the long-standing \n> issues.\n\nSame here. I grew a pretty strong opinion about the whole worktree thing, \nbut maybe that is only because it was done trying to change as little as \npossible.\n\n> I think you will need to do something for a few commands which use \n> _gently() but then, if we _are_ in a git repo, assume we are chdir'd \n> properly.\n\nRight.\n\n> We may also be able to just call setup_git_directory_gently() as the \n> first thing in the wrapper, which should help us more consistently find \n> config before the 'exec' stage (see 4e10738a for a discussion of some of \n> the issues).\n\nYep, that's what I referred to as \"pager\" problems. Thanks for the \npointer! For other people's convenience I quote it here :-)\n\n-- snip --\n Allow per-command pager config\n\n There is great debate over whether some commands should set\n up a pager automatically. This patch allows individuals to\n set their own pager preferences for each command, overriding\n the default. For example, to disable the pager for git\n status:\n\n git config pager.status false\n\n If \"--pager\" or \"--no-pager\" is specified on the command\n line, it takes precedence over the config option.\n\n There are two caveats:\n\n - you can turn on the pager for plumbing commands.\n Combined with \"core.pager = always\", this will probably\n break a lot of things. Don't do it.\n\n - This only works for builtin commands. The reason is\n somewhat complex:\n\n Calling git_config before we do setup_git_directory\n has bad side effects, because it wants to know where\n the git_dir is to find \".git/config\". Unfortunately,\n we cannot call setup_git_directory indiscriminately,\n because some builtins (like \"init\") break if we do.\n\n For builtins, this is OK, since we can just wait until\n after we call setup_git_directory. But for aliases, we\n don't know until we expand (recursively) which command\n we're doing. This should not be a huge problem for\n aliases, which can simply use \"--pager\" or \"--no-pager\"\n in the alias as appropriate.\n\n For external commands, however, we don't know we even\n have an external command until we exec it, and by then\n it is too late to check the config.\n\n An alternative approach would be to have a config mode\n where we don't bother looking at .git/config, but only\n at the user and system config files. This would make the\n behavior consistent across builtins, aliases, and\n external commands, at the cost of not allowing per-repo\n pager config for at all.\n-- snap --\n\nCiao,\nDscho\n"},{"id":"93686","messageId":"fcaeb9bf0810220704o7a0ed177u48dcada9391ec626@mail.gmail.com","threadId":"15991","inReplyTo":"alpine.DEB.1.00.0810211856090.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH] rehabilitate 'git index-pack' inside the object store","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-10-22T14:04:34Z","receivedAt":"2008-10-22T14:04:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 10/22/08, Johannes Schindelin wrote:\n> So I propose this change in semantics:\n>\n> - setup_git_directory_gently(): rename to discover_git_directory(),\n> and avoid any chdir() at all.\n> - setup_git_directory(): keep the semantics that it chdir()s to the\n> worktree, or to the git directory for bare repositories.\n>\n> Using _gently() even for RUN_SETUP builtins should solve the long standing\n> pager problem, too.\n\nOne more thing: \"git foo -h\" with RUN_SETUP won't run if repository is\nnot found. Maybe just drop RUN_SETUP and let subcommands call\nsetup_git_directory()\n-- \nDuy\n"},{"id":"93690","messageId":"fcaeb9bf0810220757w678e303cl6237eb8908261cba@mail.gmail.com","threadId":"15991","inReplyTo":"alpine.DEB.1.00.0810211856090.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH] rehabilitate 'git index-pack' inside the object store","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-10-22T14:57:50Z","receivedAt":"2008-10-22T14:57:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 10/22/08, Johannes Schindelin wrote:\n> So I propose this change in semantics:\n>\n> - setup_git_directory_gently(): rename to discover_git_directory(),\n> and avoid any chdir() at all.\n> - setup_git_directory(): keep the semantics that it chdir()s to the\n> worktree, or to the git directory for bare repositories.\n>\n> Using _gently() even for RUN_SETUP builtins should solve the long standing\n> pager problem, too.\n\nAlso, from [1] may be only set default git_dir in setup_git_env()\nafter setup_git_directory() has been called and before\nsetup_work_tree() is called (if any)? If not, die().\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/98849\n-- \nDuy\n"},{"id":"94106","messageId":"fcaeb9bf0810280652x7f883e35h157f94144263c5fa@mail.gmail.com","threadId":"15991","inReplyTo":"alpine.DEB.1.00.0810211955250.22125@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: new plan for cleaning up the worktree mess, was Re: [PATCH] rehabilitate 'git index-pack' inside the object store","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2008-10-28T13:52:25Z","receivedAt":"2008-10-28T13:52:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On 10/22/08, Johannes Schindelin wrote:\n> Hi,\n>\n> On Tue, 21 Oct 2008, Jeff King wrote:\n>\n> > On Tue, Oct 21, 2008 at 07:02:48PM +0200, Johannes Schindelin wrote:\n> >\n> > > So I propose this change in semantics:\n> > >\n> > > - setup_git_directory_gently(): rename to discover_git_directory(),\n> > > and avoid any chdir() at all.\n> > > - setup_git_directory(): keep the semantics that it chdir()s to the\n> > > worktree, or to the git directory for bare repositories.\n> > >\n> > > Using _gently() even for RUN_SETUP builtins should solve the long\n> > > standing pager problem, too.\n> >\n> > I'm not sure there aren't hidden problems lurking in that strategy\n> > (every time I look at this area of code, something unexpected prevents\n> > what I think should Just Work from Just Working), but I think that is a\n> > promising direction to go for clearing up some of the long-standing\n> > issues.\n>\n> Same here. I grew a pretty strong opinion about the whole worktree thing,\n> but maybe that is only because it was done trying to change as little as\n> possible.\n\nI played a bit with code, extracted discover_git_directory() from\nsetup_git_directory_gently() then made the latter a wrapper of the\nformer with chdir(). Some more for thoughts from the experiment.\n\n1. Because discover_git_directory() does not do chdir() until later in\nsetup_git_directory_gently(), setting GIT_DIR to a relative path seems\nunsafe (or worse unset at all, in case .git is found in parent\ndirectories). But making GIT_DIR absolute path breaks tests because\nsome of them expect \"git rev-parse --git-dir\" to return a relative\npath. The approach used in 044bbbc (Make git_dir a path relative to\nwork_tree in setup_work_tree()) can be reused to performance loss, but\nthat won't solve the issue.\n\n2. 044bbbc also brings up another issue: code duplication between\nsetup_git_directory_gently() and setup_work_tree(). The new\nsetup*gently() can be roughly like this if setup_work_tree() can\ncalculate prefix too:\n\nconst char *setup_git_directory_gently(int *nongit_ok)\n{\n int nonworktree_ok;\n /*\n * Let's assume that we are in a git repository.\n * If it turns out later that we are somewhere else, the value will be\n * updated accordingly.\n */\n if (nongit_ok)\n *nongit_ok = 0;\n\n if (!discover_git_directory()) {\n if (nongit_ok) {\n *nongit_ok = 1;\n return NULL;\n }\n die(\"Not a git repository\");\n }\n\n return setup_work_tree_gently(&nonworktree_ok); // gentle version\n}\n\nSo I propose to make setup_work_tree() return a prefix, relative to\ncurrent cwd. The setup procedure then would become:\n\nif (discover_git_directory())\n die(\"Git repository needed\");\nprefix = setup_work_tree(); // die() inside if cannot setup worktree\n\n3. Dealing with cwd outside worktree. If cwd is inside a worktree,\nprefix will be calculated correctly. If it is outside, the current\nbehavior (with both GIT_DIR and GIT_WORK_TREE set) is leave prefix as\nNULL. I think that is not right. With a wrong prefix, git commands\nwill not be able to access on-disk files. I would propose either:\n - die() if cwd is outside worktree\n - setup*gently() discovers the situation and gives up, then lets git\ncommands handle themselves. Some commands, like git-archive, don't\ncare about on-disk files at all, they could just simply ignore the\nprefix and keep going. Others may die() or handle it properly.\n\nAgain, this breaks things.\n-- \nDuy\n"}]}