{"thread":{"id":"50073","subject":"A bug in git-add with GIT_DIR?","startedAt":"2018-12-20T08:37:36Z","lastAt":"2018-12-20T15:21:32Z","messageCount":3,"participants":["Orgad Shaneh","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"365619","messageId":"CAGHpTBKyBgPURYfuZgVwnskGSy9L1+3WMrYuPmziQ7VcGDkMcA@mail.gmail.com","threadId":"50073","inReplyTo":null,"subject":"A bug in git-add with GIT_DIR?","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2018-12-20T08:37:21Z","receivedAt":"2018-12-20T08:37:36Z","isPatch":false,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"Hi,\n\nI played around with t5403-post-checkout-hook, and noticed that its\nstate is not exactly what I'd expect it to be.\n\nThe test setup is:\necho Data for commit0. >a &&\necho Data for commit0. >b &&\ngit update-index --add a &&\ngit update-index --add b &&\ntree0=$(git write-tree) &&\ncommit0=$(echo setup | git commit-tree $tree0) &&\ngit update-ref refs/heads/master $commit0 &&\ngit clone ./. clone1 &&\ngit clone ./. clone2 &&\nGIT_DIR=clone2/.git git branch new2 &&\necho Data for commit1. >clone2/b &&\nGIT_DIR=clone2/.git git add clone2/b &&\nGIT_DIR=clone2/.git git commit -m new2\n\nNow, the line before the last one executes git add clone2/b with GIT_DIR set.\n\nI'd expect that to add b inside clone2, but instead it adds an\ninexistent clone2/clone2/b, and if I stop at this line, then the\nstatus shows:\n\nOn branch master\nYour branch is up to date with 'origin/master'.\n\nChanges to be committed:\n  (use \"git reset HEAD <file>...\" to unstage)\n\n        new file:   clone2/b\n\nChanges not staged for commit:\n  (use \"git add/rm <file>...\" to update what will be committed)\n  (use \"git checkout -- <file>...\" to discard changes in working directory)\n\n        modified:   b\n        deleted:    clone2/b\n\nIs this the intended behavior? It looks like that's not what the test\nmeant to do anyway...\n\nAnd if I change it to (cd clone2 && git add b), then the commits look\nreasonable, but step 6 fails.\n\n- Orgad\n"},{"id":"365645","messageId":"20181220151830.GD27361@sigill.intra.peff.net","threadId":"50073","inReplyTo":"CAGHpTBKyBgPURYfuZgVwnskGSy9L1+3WMrYuPmziQ7VcGDkMcA@mail.gmail.com","subject":"Re: A bug in git-add with GIT_DIR?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-20T15:18:30Z","receivedAt":"2018-12-20T15:18:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 20, 2018 at 10:37:21AM +0200, Orgad Shaneh wrote:\n\n> I played around with t5403-post-checkout-hook, and noticed that its\n> state is not exactly what I'd expect it to be.\n> \n> The test setup is:\n> echo Data for commit0. >a &&\n> echo Data for commit0. >b &&\n> git update-index --add a &&\n> git update-index --add b &&\n> tree0=$(git write-tree) &&\n> commit0=$(echo setup | git commit-tree $tree0) &&\n> git update-ref refs/heads/master $commit0 &&\n> git clone ./. clone1 &&\n> git clone ./. clone2 &&\n> GIT_DIR=clone2/.git git branch new2 &&\n> echo Data for commit1. >clone2/b &&\n> GIT_DIR=clone2/.git git add clone2/b &&\n> GIT_DIR=clone2/.git git commit -m new2\n> \n> Now, the line before the last one executes git add clone2/b with GIT_DIR set.\n\nWhen GIT_DIR is set but not GIT_WORK_TREE, the current directory is\ntaken as the working tree.\n\nSo that will find clone2/b (from the current directory, which is a real\nfile), and add an index entry with that path \"clone2/b\" and the sha1 of\nthat content.\n\nBut when commands are run from inside \"clone2\", they will naturally\ntreat \"clone2\" as the working tree. And since \"clone2/b\" does not exist\ninside there, they will say \"oops, it looks like this file has been\ndeleted\".\n\n> I'd expect that to add b inside clone2, but instead it adds an\n> inexistent clone2/clone2/b, and if I stop at this line, then the\n> status shows:\n\nSort of. It never sees the path \"clone2/clone2/b\", but the path in the\nindex coupled with the working tree being inside clone2 means that it\nwould look for such a file.\n\n> On branch master\n> Your branch is up to date with 'origin/master'.\n> \n> Changes to be committed:\n>   (use \"git reset HEAD <file>...\" to unstage)\n> \n>         new file:   clone2/b\n> \n> Changes not staged for commit:\n>   (use \"git add/rm <file>...\" to update what will be committed)\n>   (use \"git checkout -- <file>...\" to discard changes in working directory)\n> \n>         modified:   b\n>         deleted:    clone2/b\n> \n> Is this the intended behavior? It looks like that's not what the test\n> meant to do anyway...\n\nThis is the expected behavior if you did \"cd clone2 && git status\".\nLooking at the test, I don't think it quite meant to do this. It looks\nlike it predates \"git -C\", but for some reason did not want to \"cd\" in a\nsubshell.\n\nI think it would be better written as:\n\n  git -C clone2 add b &&\n  git -C clone2 commit -m new2\n\nor:\n\n  (\n\tcd clone2 &&\n\tgit add b &&\n\tgit commit -m new2\n  )\n\nAnd ditto for all of the other uses of $GIT_DIR in that script. E.g.,\nthe ones that do:\n\n  GIT_DIR=clone1/.git git checkout master\n\nare likely writing the contents of clone1's master branch to the\n_current_ directory (not the working tree in clone1).\n\n> And if I change it to (cd clone2 && git add b), then the commits look\n> reasonable, but step 6 fails.\n\nYou probably just need to update the other calls, too, so they all\nmatch.\n\n-Peff\n"},{"id":"365646","messageId":"CAGHpTBKoH9aqKKNg9k=mY2_AZymiga5x8+5jNtetVZzzqsCEPg@mail.gmail.com","threadId":"50073","inReplyTo":"20181220151830.GD27361@sigill.intra.peff.net","subject":"Re: A bug in git-add with GIT_DIR?","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2018-12-20T15:21:14Z","receivedAt":"2018-12-20T15:21:32Z","isPatch":false,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"On Thu, Dec 20, 2018 at 5:18 PM Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Dec 20, 2018 at 10:37:21AM +0200, Orgad Shaneh wrote:\n>\n> > I played around with t5403-post-checkout-hook, and noticed that its\n> > state is not exactly what I'd expect it to be.\n> >\n> > The test setup is:\n> > echo Data for commit0. >a &&\n> > echo Data for commit0. >b &&\n> > git update-index --add a &&\n> > git update-index --add b &&\n> > tree0=$(git write-tree) &&\n> > commit0=$(echo setup | git commit-tree $tree0) &&\n> > git update-ref refs/heads/master $commit0 &&\n> > git clone ./. clone1 &&\n> > git clone ./. clone2 &&\n> > GIT_DIR=clone2/.git git branch new2 &&\n> > echo Data for commit1. >clone2/b &&\n> > GIT_DIR=clone2/.git git add clone2/b &&\n> > GIT_DIR=clone2/.git git commit -m new2\n> >\n> > Now, the line before the last one executes git add clone2/b with GIT_DIR set.\n>\n> When GIT_DIR is set but not GIT_WORK_TREE, the current directory is\n> taken as the working tree.\n>\n> So that will find clone2/b (from the current directory, which is a real\n> file), and add an index entry with that path \"clone2/b\" and the sha1 of\n> that content.\n>\n> But when commands are run from inside \"clone2\", they will naturally\n> treat \"clone2\" as the working tree. And since \"clone2/b\" does not exist\n> inside there, they will say \"oops, it looks like this file has been\n> deleted\".\n>\n> > I'd expect that to add b inside clone2, but instead it adds an\n> > inexistent clone2/clone2/b, and if I stop at this line, then the\n> > status shows:\n>\n> Sort of. It never sees the path \"clone2/clone2/b\", but the path in the\n> index coupled with the working tree being inside clone2 means that it\n> would look for such a file.\n>\n> > On branch master\n> > Your branch is up to date with 'origin/master'.\n> >\n> > Changes to be committed:\n> >   (use \"git reset HEAD <file>...\" to unstage)\n> >\n> >         new file:   clone2/b\n> >\n> > Changes not staged for commit:\n> >   (use \"git add/rm <file>...\" to update what will be committed)\n> >   (use \"git checkout -- <file>...\" to discard changes in working directory)\n> >\n> >         modified:   b\n> >         deleted:    clone2/b\n> >\n> > Is this the intended behavior? It looks like that's not what the test\n> > meant to do anyway...\n>\n> This is the expected behavior if you did \"cd clone2 && git status\".\n> Looking at the test, I don't think it quite meant to do this. It looks\n> like it predates \"git -C\", but for some reason did not want to \"cd\" in a\n> subshell.\n>\n> I think it would be better written as:\n>\n>   git -C clone2 add b &&\n>   git -C clone2 commit -m new2\n>\n> or:\n>\n>   (\n>         cd clone2 &&\n>         git add b &&\n>         git commit -m new2\n>   )\n>\n> And ditto for all of the other uses of $GIT_DIR in that script. E.g.,\n> the ones that do:\n>\n>   GIT_DIR=clone1/.git git checkout master\n>\n> are likely writing the contents of clone1's master branch to the\n> _current_ directory (not the working tree in clone1).\n>\n> > And if I change it to (cd clone2 && git add b), then the commits look\n> > reasonable, but step 6 fails.\n>\n> You probably just need to update the other calls, too, so they all\n> match.\n>\n> -Peff\n\nThanks. I'll refactor the tests and post a patch later.\n\n- Orgad\n"}]}