{"thread":{"id":"50141","subject":"Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files","startedAt":"2019-01-01T23:18:05Z","lastAt":"2019-01-23T21:23:08Z","messageCount":30,"participants":["Anthony Sottile","Duy Nguyen","Junio C Hamano","Ben Peart","SZEDER Gábor","Johannes Schindelin","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"366022","messageId":"CA+dzEB=DH0irkFaRzkKERSjdZ=EJ+mG3Ri2Xeobx9Yu_eDd+jg@mail.gmail.com","threadId":"50141","inReplyTo":null,"subject":"Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2019-01-01T23:17:49Z","receivedAt":"2019-01-01T23:18:05Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"Here's a simple regression test -- haven't had time to bisect this\n\n```\n#!/usr/bin/env bash\nset -euxo pipefail\n\nrm -rf src dest\n\ngit --version\n\ngit init src\necho hi > src/a\ngit -C src add .\ngit -C src commit -m \"initial commit\"\nrev=\"$(git -C src rev-parse HEAD)\"\n\ngit clone --no-checkout src dest\ngit -C dest checkout \"$rev\" -b branch\ntest -f dest/a\n\n: 'SUCCESS!'\n```\n\nWith git 2.17.1\n\n```\n+ set -euo pipefail\n+ rm -rf src dest\n+ git --version\ngit version 2.17.1\n+ git init src\nInitialized empty Git repository in /tmp/t/src/.git/\n+ echo hi\n+ git -C src add .\n+ git -C src commit -m 'initial commit'\n[master (root-commit) 61ae2ae] initial commit\n 1 file changed, 1 insertion(+)\n create mode 100644 a\n++ git -C src rev-parse HEAD\n+ rev=61ae2ae9c9a96de7b1688b095f20a6adf9c20db1\n+ git clone --no-checkout src dest\nCloning into 'dest'...\ndone.\n+ git -C dest checkout 61ae2ae9c9a96de7b1688b095f20a6adf9c20db1 -b branch\nSwitched to a new branch 'branch'\n+ test -f dest/a\n+ : 'SUCCESS!'\n```\n\nWith git 2.20.GIT (b21ebb671bb7dea8d342225f0d66c41f4e54d5ca)\n\n```\n+ set -euo pipefail\n+ rm -rf src dest\n+ git --version\ngit version 2.20.GIT\n+ git init src\nInitialized empty Git repository in /tmp/t/src/.git/\n+ echo hi\n+ git -C src add .\n+ git -C src commit -m 'initial commit'\n[master (root-commit) df4d6dc] initial commit\n 1 file changed, 1 insertion(+)\n create mode 100644 a\n++ git -C src rev-parse HEAD\n+ rev=df4d6dcf02b15bfe2b3f0e4a8aa25ac165b1368c\n+ git clone --no-checkout src dest\nCloning into 'dest'...\ndone.\n+ git -C dest checkout df4d6dcf02b15bfe2b3f0e4a8aa25ac165b1368c -b branch\nD a\nSwitched to a new branch 'branch'\n+ test -f dest/a\n```\n\nA workaround for my use case is to not use `--no-checkout`, though\nthis is potentially slow\n\nAnthony\n"},{"id":"366032","messageId":"CACsJy8B=-V7XY+=5pwwSzg8B6Goa55DPPU3ErgjOEsSJVni18Q@mail.gmail.com","threadId":"50141","inReplyTo":"CA+dzEB=DH0irkFaRzkKERSjdZ=EJ+mG3Ri2Xeobx9Yu_eDd+jg@mail.gmail.com","subject":"Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-02T11:08:15Z","receivedAt":"2019-01-02T11:08:52Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 2, 2019 at 6:36 AM Anthony Sottile wrote:\n>\n> Here's a simple regression test -- haven't had time to bisect this\n\nI can't reproduce with either 2.20.0, 2.20.1 or 'master'. It would be\ngreat if you could bisect this.\n\nThere are no suspicious commits from 2.27.1 touching\nbuiltin/checkout.c. Though there are some more changes in\nunpack-trees.c that might cause this (big wild guess).\n-- \nDuy\n"},{"id":"366043","messageId":"CA+dzEB=TPxng4YBC4Vfh=ZcctAzRQ+drJ3y2sXwP=JXf+UweSA@mail.gmail.com","threadId":"50141","inReplyTo":"CACsJy8B=-V7XY+=5pwwSzg8B6Goa55DPPU3ErgjOEsSJVni18Q@mail.gmail.com","subject":"Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2019-01-02T16:18:02Z","receivedAt":"2019-01-02T16:18:17Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"On Wed, Jan 2, 2019 at 3:08 AM Duy Nguyen wrote:\n>\n> On Wed, Jan 2, 2019 at 6:36 AM Anthony Sottile wrote:\n> >\n> > Here's a simple regression test -- haven't had time to bisect this\n>\n> I can't reproduce with either 2.20.0, 2.20.1 or 'master'. It would be\n> great if you could bisect this.\n>\n> There are no suspicious commits from 2.27.1 touching\n> builtin/checkout.c. Though there are some more changes in\n> unpack-trees.c that might cause this (big wild guess).\n> --\n> Duy\n\n\nheated a small room but here's the results of the bisect!\n\nfa655d8411cc2d7ffcf898e53a1493c737d7de68 is the first bad commit\ncommit fa655d8411cc2d7ffcf898e53a1493c737d7de68\nAuthor: Ben Peart \nDate: Thu Aug 16 18:27:11 2018 +0000\n\n checkout: optimize \"git checkout -b \"\n\n Skip merging the commit, updating the index and working directory if and\n only if we are creating a new branch via \"git checkout -b .\"\n Any other checkout options will still go through the former code path.\n\n If sparse_checkout is on, require the user to manually opt in to this\n optimzed behavior by setting the config setting checkout.optimizeNewBranch\n to true as we will no longer update the skip-worktree bit in the index, nor\n add/remove files in the working directory to reflect the current sparse\n checkout settings.\n\n For comparison, running \"git checkout -b \" on a large\nrepo takes:\n\n 14.6 seconds - without this patch\n 0.3 seconds - with this patch\n\n Signed-off-by: Ben Peart \n Signed-off-by: Junio C Hamano \n\n:040000 040000 817bfb8ef961545a554005d42967b5ab7cfdb041\ne57e576d0d4fb7f25c12a5dcc7651ef6698e961b M Documentation\n:040000 040000 c089f91f4532caa2a17e4f10a1a7ed3aa5d2023c\n7cf16a0aa288f898a880ffefe82ee7506b83bef4 M builtin\n:040000 040000 adfdb05964a692e03ee07d2e43841f6304d996bd\n8681416093802b9051599ebea8f63f5a45968e6f M t\nbisect run success\n\n\nHere's the script and invocations:\n\n```\n#!/usr/bin/env bash\nset -euxo pipefail\n\nrm -rf \"$PWD/prefix\"\nmake prefix=\"$PWD/prefix\" -j8 install\nexport PATH=\"$PWD/prefix/bin:$PATH\"\n\nrm -rf src dest\n\ngit --version\n\ngit init src\necho hi > src/a\ngit -C src add .\ngit -C src commit -m \"initial commit\"\nrev=\"$(git -C src rev-parse HEAD)\"\n\ngit clone --no-checkout src dest\ngit -C dest checkout \"$rev\" -b branch\ntest -f dest/a\n\n: 'SUCCESS!'\n```\n\n```\ngit bisect begin\ngit bisect bad HEAD\ngit bisect good v2.17.1\ngit bisect run ./bisect.sh\n```\n\nAnthony\n"},{"id":"366082","messageId":"CACsJy8C=O=ZDvD0ReSJOyAsNDEb5Yz-iFvs7oV5zAXaFf-dw5g@mail.gmail.com","threadId":"50141","inReplyTo":"CA+dzEB=TPxng4YBC4Vfh=ZcctAzRQ+drJ3y2sXwP=JXf+UweSA@mail.gmail.com","subject":"Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-03T10:04:47Z","receivedAt":"2019-01-03T10:05:16Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 2, 2019 at 11:18 PM Anthony Sottile wrote:\n> heated a small room but here's the results of the bisect!\n>\n> fa655d8411cc2d7ffcf898e53a1493c737d7de68 is the first bad commit\n> commit fa655d8411cc2d7ffcf898e53a1493c737d7de68\n> Author: Ben Peart \n> Date: Thu Aug 16 18:27:11 2018 +0000\n>\n> checkout: optimize \"git checkout -b \"\n\nI did test this commit before and could not reproduce. But one thing I\ndid not notice is I set sparsecheckout on by default. which always\nforces this optimization off Turning off sparse checkout, I could\nindeed reproduce.\n\nI plan to revert this commit anyway when the new command \"git\nswitch-branch\" comes. The optimization will be unconditionally in the\nnew command without this hack and users are encouraged to use that one\ninstead of \"git checkout\".\n\nMeanwhile, let's see if Ben wants to fix this or revert it.\n\n>\n> Skip merging the commit, updating the index and working directory if and\n> only if we are creating a new branch via \"git checkout -b .\"\n> Any other checkout options will still go through the former code path.\n>\n> If sparse_checkout is on, require the user to manually opt in to this\n> optimzed behavior by setting the config setting checkout.optimizeNewBranch\n> to true as we will no longer update the skip-worktree bit in the index, nor\n> add/remove files in the working directory to reflect the current sparse\n> checkout settings.\n>\n> For comparison, running \"git checkout -b \" on a large\n> repo takes:\n>\n> 14.6 seconds - without this patch\n> 0.3 seconds - with this patch\n>\n> Signed-off-by: Ben Peart \n> Signed-off-by: Junio C Hamano \n>\n> :040000 040000 817bfb8ef961545a554005d42967b5ab7cfdb041\n> e57e576d0d4fb7f25c12a5dcc7651ef6698e961b M Documentation\n> :040000 040000 c089f91f4532caa2a17e4f10a1a7ed3aa5d2023c\n> 7cf16a0aa288f898a880ffefe82ee7506b83bef4 M builtin\n> :040000 040000 adfdb05964a692e03ee07d2e43841f6304d996bd\n> 8681416093802b9051599ebea8f63f5a45968e6f M t\n> bisect run success\n>\n>\n> Here's the script and invocations:\n>\n> ```\n> #!/usr/bin/env bash\n> set -euxo pipefail\n>\n> rm -rf \"$PWD/prefix\"\n> make prefix=\"$PWD/prefix\" -j8 install\n> export PATH=\"$PWD/prefix/bin:$PATH\"\n>\n> rm -rf src dest\n>\n> git --version\n>\n> git init src\n> echo hi > src/a\n> git -C src add .\n> git -C src commit -m \"initial commit\"\n> rev=\"$(git -C src rev-parse HEAD)\"\n>\n> git clone --no-checkout src dest\n> git -C dest checkout \"$rev\" -b branch\n> test -f dest/a\n>\n> : 'SUCCESS!'\n> ```\n>\n> ```\n> git bisect begin\n> git bisect bad HEAD\n> git bisect good v2.17.1\n> git bisect run ./bisect.sh\n> ```\n>\n> Anthony\n\n\n\n-- \nDuy\n"},{"id":"366101","messageId":"xmqqef9th4iy.fsf@gitster-ct.c.googlers.com","threadId":"50141","inReplyTo":"CACsJy8C=O=ZDvD0ReSJOyAsNDEb5Yz-iFvs7oV5zAXaFf-dw5g@mail.gmail.com","subject":"Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-03T20:25:57Z","receivedAt":"2019-01-03T20:26:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> I plan to revert this commit anyway when the new command \"git\n> switch-branch\" comes. The optimization will be unconditionally in the\n> new command without this hack and users are encouraged to use that one\n> instead of \"git checkout\".\n\nI tend to think that the behaviour is perfectly in line with what\nBen wanted to have, which is to make \"checkout -b new [HEAD]\" not to\ntouch anything in the index or the working tree at all.\n\nIt further is possible to argue that what is strange in the whole\nepisode is what \"clone --no-checkout\" does. In such a repository,\nif you say \"git status\", you'd notice that it is reported that all\npaths have been deleted.\n\nNow, if you instead do\n\n\tgit clone $src dst\n\tcd dst\n\tgit rm file\n\tgit checkout -b new\n\ni.e. starting from a clone with normal working tree, manually\nremoving a path or two, and then create a new branch starting from\nthat state while carrying all the local changes, you *do* want to\nsee that 'file' to stay missing. After all, \"do not lose the local\nchanges; carry them forward\" is what switching branches is about.\n\nAnd from that point of view, we could consider that\n\n\tgit clone --no-checkout $src $dst\n\nis equivalent to\n\n\tgit clone $src $dst && git -C $dst rm -r .\n\nHaving said all that.\n\n> Meanwhile, let's see if Ben wants to fix this or revert it.\n\nA \"fix\" to Ben's optimization for this particular case should be\nfairly straight-forward. I think we have a special case in the\ncheckout codepath for an initial checkout and disable \"carry forward\nthe fact that the user wanted all the paths removed\", so it would be\nthe matter of adding yet another condition (is_cache_unborn(), which\nis used to set topts.initial_checkout) to the large collection of\nconditions in skip_merge_working_tree().\n\nBack when the \"optimization\" was discussed, all reviewers said that\nit would become maintenance nightmare to ensure that the set of\nconditions accurately tracks the case where the optimization is\nsafe. Now they are entitled to say \"we told you so\".\n"},{"id":"366103","messageId":"CA+dzEB=+ROLVjp36SQjucouc8YUWTvYBrN4QyS5fsStMPtbw_w@mail.gmail.com","threadId":"50141","inReplyTo":"xmqqef9th4iy.fsf@gitster-ct.c.googlers.com","subject":"Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2019-01-03T20:35:24Z","receivedAt":"2019-01-03T20:35:38Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano wrote:\n> A \"fix\" to Ben's optimization for this particular case should be\n> fairly straight-forward. I think we have a special case in the\n> checkout codepath for an initial checkout and disable \"carry forward\n> the fact that the user wanted all the paths removed\", so it would be\n> the matter of adding yet another condition (is_cache_unborn(), which\n> is used to set topts.initial_checkout) to the large collection of\n> conditions in skip_merge_working_tree().\n>\n\nI think it might be simpler than that even -- the optimization treats\nthe following as equivalent when the current checked out revision is\ndeadbeef (even if the index / worktree differ), when before they were\nnot:\n\n- git checkout -b newbranch\n- git checkout deadbeef -b newbranch\n\nIf a revision is specified on the commandline it should be checked out.\n\nAnthony\n"},{"id":"366115","messageId":"xmqqwonlfm0g.fsf@gitster-ct.c.googlers.com","threadId":"50141","inReplyTo":"CA+dzEB=+ROLVjp36SQjucouc8YUWTvYBrN4QyS5fsStMPtbw_w@mail.gmail.com","subject":"Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-03T21:51:11Z","receivedAt":"2019-01-03T21:51:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anthony Sottile writes:\n\n> On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano wrote:\n>> A \"fix\" to Ben's optimization for this particular case should be\n>> fairly straight-forward. I think we have a special case in the\n>> checkout codepath for an initial checkout and disable \"carry forward\n>> the fact that the user wanted all the paths removed\", so it would be\n>> the matter of adding yet another condition (is_cache_unborn(), which\n>> is used to set topts.initial_checkout) to the large collection of\n>> conditions in skip_merge_working_tree().\n>\n> I think it might be simpler than that even -- the optimization treats\n> the following as equivalent when the current checked out revision is\n> deadbeef (even if the index / worktree differ), when before they were\n> not:\n>\n> - git checkout -b newbranch\n> - git checkout deadbeef -b newbranch\n>\n> If a revision is specified on the commandline it should be checked out.\n\nIf it were to be a \"fix\", the exact same command line as people used\nto be able to use, i.e. \"git checkout -b newbranch\", should be made\nto do what it used to do.\n\nForcing users to use a different command to workaround the bug is\nnot a usable \"fix\". If we want a working workaround, you can tell\nyour users to use \n\n git reset --hard HEAD && git checkout -b newbranch\n\nand that would already work without any code change ;-).\n\n\n"},{"id":"366117","messageId":"CA+dzEB=oeL2oByqiH4FeCHc29yGL2TwhmO1DKmRTDx8Xdhh=NQ@mail.gmail.com","threadId":"50141","inReplyTo":"xmqqwonlfm0g.fsf@gitster-ct.c.googlers.com","subject":"Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2019-01-03T22:05:59Z","receivedAt":"2019-01-03T22:06:15Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"On Thu, Jan 3, 2019 at 1:51 PM Junio C Hamano wrote:\n>\n> Anthony Sottile writes:\n>\n> > On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano wrote:\n> >> A \"fix\" to Ben's optimization for this particular case should be\n> >> fairly straight-forward. I think we have a special case in the\n> >> checkout codepath for an initial checkout and disable \"carry forward\n> >> the fact that the user wanted all the paths removed\", so it would be\n> >> the matter of adding yet another condition (is_cache_unborn(), which\n> >> is used to set topts.initial_checkout) to the large collection of\n> >> conditions in skip_merge_working_tree().\n> >\n> > I think it might be simpler than that even -- the optimization treats\n> > the following as equivalent when the current checked out revision is\n> > deadbeef (even if the index / worktree differ), when before they were\n> > not:\n> >\n> > - git checkout -b newbranch\n> > - git checkout deadbeef -b newbranch\n> >\n> > If a revision is specified on the commandline it should be checked out.\n>\n> If it were to be a \"fix\", the exact same command line as people used\n> to be able to use, i.e. \"git checkout -b newbranch\", should be made\n> to do what it used to do.\n>\n> Forcing users to use a different command to workaround the bug is\n> not a usable \"fix\". If we want a working workaround, you can tell\n> your users to use\n>\n> git reset --hard HEAD && git checkout -b newbranch\n>\n> and that would already work without any code change ;-).\n>\n>\n\noh wow, I didn't realize `git checkout -b newbranch` also used to\nreset the `--no-checkout` state, yeah you're right the optimization is\nway more problematic than I had considered.\n\nI'm working around by not using `--no-checkout` personally\n\nAnthony\n"},{"id":"366853","messageId":"e9284bc1-0db2-6f90-6e8e-3a7682c03dd0@gmail.com","threadId":"50141","inReplyTo":"CA+dzEB=oeL2oByqiH4FeCHc29yGL2TwhmO1DKmRTDx8Xdhh=NQ@mail.gmail.com","subject":"Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-16T14:39:16Z","receivedAt":"2019-01-16T14:39:22Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 1/3/2019 5:05 PM, Anthony Sottile wrote:\n> On Thu, Jan 3, 2019 at 1:51 PM Junio C Hamano wrote:\n>>\n>> Anthony Sottile writes:\n>>\n>>> On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano wrote:\n>>>> A \"fix\" to Ben's optimization for this particular case should be\n>>>> fairly straight-forward. I think we have a special case in the\n>>>> checkout codepath for an initial checkout and disable \"carry forward\n>>>> the fact that the user wanted all the paths removed\", so it would be\n>>>> the matter of adding yet another condition (is_cache_unborn(), which\n>>>> is used to set topts.initial_checkout) to the large collection of\n>>>> conditions in skip_merge_working_tree().\n>>>\n>>> I think it might be simpler than that even -- the optimization treats\n>>> the following as equivalent when the current checked out revision is\n>>> deadbeef (even if the index / worktree differ), when before they were\n>>> not:\n>>>\n>>> - git checkout -b newbranch\n>>> - git checkout deadbeef -b newbranch\n>>>\n>>> If a revision is specified on the commandline it should be checked out.\n>>\n>> If it were to be a \"fix\", the exact same command line as people used\n>> to be able to use, i.e. \"git checkout -b newbranch\", should be made\n>> to do what it used to do.\n>>\n>> Forcing users to use a different command to workaround the bug is\n>> not a usable \"fix\". If we want a working workaround, you can tell\n>> your users to use\n>>\n>> git reset --hard HEAD && git checkout -b newbranch\n>>\n>> and that would already work without any code change ;-).\n>>\n>>\n\nJust noticed this thread. I agree that the behavior of `git clone \n--no-checkout` is a little odd in that it shows everything as deleted \nbut the goal of the `checkout -b` optimization was to not change \nbehavior (unless the user opt-ed in to the changed behavior via \ncheckout.optimizeNewBranch). I'll work on a patch to detect this case \nand ensure the default behavior doesn't change.\n\n> \n> oh wow, I didn't realize `git checkout -b newbranch` also used to\n> reset the `--no-checkout` state, yeah you're right the optimization is\n> way more problematic than I had considered.\n> \n> I'm working around by not using `--no-checkout` personally\n> \n> Anthony\n> \n"},{"id":"367128","messageId":"20190118185558.17688-1-peartben@gmail.com","threadId":"50141","inReplyTo":"CA+dzEB=DH0irkFaRzkKERSjdZ=EJ+mG3Ri2Xeobx9Yu_eDd+jg@mail.gmail.com","subject":"[PATCH v1 0/2] Fix regression in checkout -b","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-18T18:55:56Z","receivedAt":"2019-01-18T18:56:09Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nAnthony Sottile determined that commit fa655d8411\n\"checkout: optimize \"git checkout -b \" introduced\nan unintentional change in behavior for 'checkout -b' after doing a\n'clone --no-checkout'. Create a test to demonstrate the regression then\nfix the bug and update the test to demonstrate the fix.\n\nBase Ref: master\nWeb-Diff: https://github.com/benpeart/git/commit/3930f7fa32\nCheckout: git fetch https://github.com/benpeart/git initial-checkout-v1 && git checkout 3930f7fa32\n\nBen Peart (2):\n checkout: add test to demonstrate regression with checkout -b on\n initial commit\n checkout: fix regression in checkout -b on intitial checkout\n\n builtin/checkout.c | 6 ++++++\n t/t2018-checkout-branch.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+)\n\n\nbase-commit: 77556354bb7ac50450e3b28999e3576969869068\n-- \n2.19.1.gvfs.1.16.g9d1374d\n\n\n"},{"id":"367129","messageId":"20190118185558.17688-2-peartben@gmail.com","threadId":"50141","inReplyTo":"20190118185558.17688-1-peartben@gmail.com","subject":"[PATCH v1 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-18T18:55:57Z","receivedAt":"2019-01-18T18:56:11Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nCommit fa655d8411 checkout: optimize \"git checkout -b \" introduced\nan unintentional change in behavior for 'checkout -b' after doing a\n'clone --no-checkout'. Add a test to demonstrate the changed behavior to be\nused in a later patch to verify the fix.\n\nSigned-off-by: Ben Peart \n---\n t/t2018-checkout-branch.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 2131fb2a56..35999b3adb 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -198,4 +198,15 @@ test_expect_success 'checkout -B to the current branch works' '\n \ttest_dirty_mergeable\n '\n \n+test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n+\tgit init src &&\n+\techo hi > src/a &&\n+\tgit -C src add . &&\n+\tgit -C src commit -m \"initial commit\" &&\n+\trev=\"$(git -C src rev-parse HEAD)\" &&\n+\tgit clone --no-checkout src dest &&\n+\tgit -C dest checkout \"$rev\" -b branch &&\n+\ttest_must_fail test -f dest/a\n+'\n+\n test_done\n-- \n2.19.1.gvfs.1.16.g9d1374d\n\n"},{"id":"367130","messageId":"20190118185558.17688-3-peartben@gmail.com","threadId":"50141","inReplyTo":"20190118185558.17688-1-peartben@gmail.com","subject":"[PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-18T18:55:58Z","receivedAt":"2019-01-18T18:56:13Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nWhen doing a 'checkout -b' do a full checkout including updating the working\ntree when doing the initial checkout. This fixes the regression in behavior\ncaused by fa655d8411 checkout: optimize \"git checkout -b \"\n\nSigned-off-by: Ben Peart \n---\n builtin/checkout.c | 6 ++++++\n t/t2018-checkout-branch.sh | 2 +-\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 6fadf412e8..af6b5c8336 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -517,6 +517,12 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n \tif (core_apply_sparse_checkout && !checkout_optimize_new_branch)\n \t\treturn 0;\n \n+\t/*\n+\t * We must do the merge if this is the initial checkout\n+\t */\n+\tif (is_cache_unborn())\n+\t\treturn 0;\n+\n \t/*\n \t * We must do the merge if we are actually moving to a new commit.\n \t */\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 35999b3adb..c438889b0c 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -206,7 +206,7 @@ test_expect_success 'checkout -b after clone --no-checkout does a checkout of HE\n \trev=\"$(git -C src rev-parse HEAD)\" &&\n \tgit clone --no-checkout src dest &&\n \tgit -C dest checkout \"$rev\" -b branch &&\n-\ttest_must_fail test -f dest/a\n+\ttest -f dest/a\n '\n \n test_done\n-- \n2.19.1.gvfs.1.16.g9d1374d\n\n"},{"id":"367133","messageId":"20190118192331.GN840@szeder.dev","threadId":"50141","inReplyTo":"20190118185558.17688-2-peartben@gmail.com","subject":"Re: [PATCH v1 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-18T19:23:31Z","receivedAt":"2019-01-18T19:23:38Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 18, 2019 at 01:55:57PM -0500, Ben Peart wrote:\n> From: Ben Peart \n> \n> Commit fa655d8411 checkout: optimize \"git checkout -b \" introduced\n\nStyle nit: fa655d8411 (checkout: optimize \"git checkout -b\n\", 2018-08-16)\n\nFurthermore, please wrap the commit message at a width of around 70 or\nso chars.\n\n> an unintentional change in behavior for 'checkout -b' after doing a\n> 'clone --no-checkout'. Add a test to demonstrate the changed behavior to be\n> used in a later patch to verify the fix.\n> \n> Signed-off-by: Ben Peart \n> ---\n> t/t2018-checkout-branch.sh | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n> \n> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> index 2131fb2a56..35999b3adb 100755\n> --- a/t/t2018-checkout-branch.sh\n> +++ b/t/t2018-checkout-branch.sh\n> @@ -198,4 +198,15 @@ test_expect_success 'checkout -B to the current branch works' '\n> \ttest_dirty_mergeable\n> '\n> \n> +test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n\nAs this test is supposed to demonstrate a regression, this should be\ntest_expect_failure (to be flipped to test_expect_success in the next\npatch fixing the regression).\n\n> +\tgit init src &&\n> +\techo hi > src/a &&\n\nStyle nit: no space between redirection and filename, but...\n\n> +\tgit -C src add . &&\n> +\tgit -C src commit -m \"initial commit\" &&\n\nThe above three lines could be replaced by a single\n\n test_commit -C src a\n\ncommand.\n\n> +\trev=\"$(git -C src rev-parse HEAD)\" &&\n> +\tgit clone --no-checkout src dest &&\n> +\tgit -C dest checkout \"$rev\" -b branch &&\n> +\ttest_must_fail test -f dest/a\n\nAnd here the test should expect the file to be there even when\ndemonstrating the regression.\n\nPlease use the 'test_path_is_file' helper instead of 'test -f', as it\ngives a useful error on failure.\n\n> +'\n> +\n> test_done\n> -- \n> 2.19.1.gvfs.1.16.g9d1374d\n> \n"},{"id":"367136","messageId":"xmqqef99ka71.fsf@gitster-ct.c.googlers.com","threadId":"50141","inReplyTo":"20190118185558.17688-3-peartben@gmail.com","subject":"Re: [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-18T20:00:34Z","receivedAt":"2019-01-18T20:00:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> From: Ben Peart \n>\n> When doing a 'checkout -b' do a full checkout including updating the working\n> tree when doing the initial checkout. This fixes the regression in behavior\n> caused by fa655d8411 checkout: optimize \"git checkout -b \"\n>\n> Signed-off-by: Ben Peart \n> ---\n> builtin/checkout.c | 6 ++++++\n> t/t2018-checkout-branch.sh | 2 +-\n> 2 files changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 6fadf412e8..af6b5c8336 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -517,6 +517,12 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n> \tif (core_apply_sparse_checkout && !checkout_optimize_new_branch)\n> \t\treturn 0;\n> \n> +\t/*\n> +\t * We must do the merge if this is the initial checkout\n> +\t */\n> +\tif (is_cache_unborn())\n> +\t\treturn 0;\n> +\n\nYup, that's a trivial fix ;-)\n\n> \t/*\n> \t * We must do the merge if we are actually moving to a new commit.\n> \t */\n> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> index 35999b3adb..c438889b0c 100755\n> --- a/t/t2018-checkout-branch.sh\n> +++ b/t/t2018-checkout-branch.sh\n> @@ -206,7 +206,7 @@ test_expect_success 'checkout -b after clone --no-checkout does a checkout of HE\n> \trev=\"$(git -C src rev-parse HEAD)\" &&\n> \tgit clone --no-checkout src dest &&\n> \tgit -C dest checkout \"$rev\" -b branch &&\n> -\ttest_must_fail test -f dest/a\n> +\ttest -f dest/a\n> '\n\nThis is flipping the wrong thing. Rather, introduce the whole test\nas test_expect_failure that wants to make sure dest/a exists, and\nwith this 2/2 patch flip test_expect_failure into test_expect_success.\n\nThanks.\n"},{"id":"367162","messageId":"20190119005202.GO840@szeder.dev","threadId":"50141","inReplyTo":"20190118185558.17688-3-peartben@gmail.com","subject":"Re: [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-19T00:52:02Z","receivedAt":"2019-01-19T00:52:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 18, 2019 at 01:55:58PM -0500, Ben Peart wrote:\n> From: Ben Peart \n> \n> When doing a 'checkout -b' do a full checkout including updating the working\n> tree when doing the initial checkout. This fixes the regression in behavior\n> caused by fa655d8411 checkout: optimize \"git checkout -b \"\n> \n> Signed-off-by: Ben Peart \n> ---\n> builtin/checkout.c | 6 ++++++\n> t/t2018-checkout-branch.sh | 2 +-\n> 2 files changed, 7 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 6fadf412e8..af6b5c8336 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -517,6 +517,12 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n> \tif (core_apply_sparse_checkout && !checkout_optimize_new_branch)\n> \t\treturn 0;\n> \n> +\t/*\n> +\t * We must do the merge if this is the initial checkout\n> +\t */\n> +\tif (is_cache_unborn())\n> +\t\treturn 0;\n> +\n> \t/*\n> \t * We must do the merge if we are actually moving to a new commit.\n> \t */\n\nThis patch breaks 'checkout -b checkout.optimizeNewBranch interaction'\nin 't1090-sparse-checkout-scope.sh':\n\n + cp .git/info/sparse-checkout .git/info/sparse-checkout.bak\n + test_when_finished\n mv -f .git/info/sparse-checkout.bak .git/info/sparse-checkout\n git checkout master\n \n + test 0 = 0\n + test_cleanup={\n mv -f .git/info/sparse-checkout.bak .git/info/sparse-checkout\n git checkout master\n \n } && (exit \"$eval_ret\"); eval_ret=$?; :\n + echo /b\n + git ls-files -t b\n + test S b = S b\n + git -c checkout.optimizeNewBranch=true checkout -b fast\n Switched to a new branch 'fast'\n + git ls-files -t b\n + test H b = S b\n error: last command exited with $?=1\n not ok 4 - checkout -b checkout.optimizeNewBranch interaction\n\n\n"},{"id":"367167","messageId":"xmqq1s59igj2.fsf@gitster-ct.c.googlers.com","threadId":"50141","inReplyTo":"20190118185558.17688-1-peartben@gmail.com","subject":"Re: [PATCH v1 0/2] Fix regression in checkout -b","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-19T01:26:41Z","receivedAt":"2019-01-19T01:26:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> checkout: add test to demonstrate regression with checkout -b on\n> initial commit\n> checkout: fix regression in checkout -b on intitial checkout\n>\n> builtin/checkout.c | 6 ++++++\n> t/t2018-checkout-branch.sh | 11 +++++++++++\n> 2 files changed, 17 insertions(+)\n>\n>\n> base-commit: 77556354bb7ac50450e3b28999e3576969869068\n\nAfter applying these two patches on this exact commit, I tried to\nrun the usual test; t1090 seems to break.\n"},{"id":"367252","messageId":"20190121195008.8700-1-peartben@gmail.com","threadId":"50141","inReplyTo":"20190118185558.17688-1-peartben@gmail.com","subject":"[PATCH v2 0/2] Fix regression in checkout -b","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-21T19:50:06Z","receivedAt":"2019-01-21T19:50:21Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nThe optimized `checkout -b` doesn�t typically create/update the index and\nworking directory. Add a new test to detect the case when the call to\n`checkout -b` is the first call after doing a `clone --no-checkout` and no\nindex exists. In this specific case, well now make the call to\nmerge_working_tree() which will create the index and update the working\ndirectory.\n\nAlso simplify and update the test cases based on the feedback provided by\nSzeder and Junio. A shout out to Johannes who diagnosed, fixed and patched\na bug that was preventing me from running the git test suite in the master\nbranch.\n\nBase Ref: master\nWeb-Diff: https://github.com/benpeart/git/commit/55dd8602f5\nCheckout: git fetch https://github.com/benpeart/git initial-checkout-v2 && git checkout 55dd8602f5\n\n\n### Interdiff (v1..v2):\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex af6b5c8336..9c6e94319e 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -517,12 +517,6 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n \tif (core_apply_sparse_checkout && !checkout_optimize_new_branch)\n \t\treturn 0;\n \n-\t/*\n-\t * We must do the merge if this is the initial checkout\n-\t */\n-\tif (is_cache_unborn())\n-\t\treturn 0;\n-\n \t/*\n \t * We must do the merge if we are actually moving to a new commit.\n \t */\n@@ -598,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n \t * Remaining variables are not checkout options but used to track state\n \t */\n \n+\t /*\n+\t * Do the merge if this is the initial checkout\n+\t *\n+\t */\n+\tif (!file_exists(get_index_file()))\n+\t\treturn 0;\n+\n \treturn 1;\n }\n \ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex c438889b0c..c5014ad9a6 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -200,13 +200,11 @@ test_expect_success 'checkout -B to the current branch works' '\n \n test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n \tgit init src &&\n-\techo hi > src/a &&\n-\tgit -C src add . &&\n-\tgit -C src commit -m \"initial commit\" &&\n+\ttest_commit -C src a &&\n \trev=\"$(git -C src rev-parse HEAD)\" &&\n \tgit clone --no-checkout src dest &&\n \tgit -C dest checkout \"$rev\" -b branch &&\n-\ttest -f dest/a\n+\ttest_path_is_file dest/a.t\n '\n \n test_done\n\n\n### Patches\n\nBen Peart (2):\n checkout: add test to demonstrate regression with checkout -b on\n initial commit\n checkout: fix regression in checkout -b on intitial checkout\n\n builtin/checkout.c | 7 +++++++\n t/t2018-checkout-branch.sh | 9 +++++++++\n 2 files changed, 16 insertions(+)\n\n\nbase-commit: 77556354bb7ac50450e3b28999e3576969869068\n-- \n2.19.1.gvfs.1.16.g9d1374d\n\n\n"},{"id":"367253","messageId":"20190121195008.8700-2-peartben@gmail.com","threadId":"50141","inReplyTo":"20190121195008.8700-1-peartben@gmail.com","subject":"[PATCH v2 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-21T19:50:07Z","receivedAt":"2019-01-21T19:50:23Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nCommit fa655d8411 (checkout: optimize \"git checkout -b \", 2018-08-16)\nintroduced an unintentional change in behavior for 'checkout -b' after doing\n'clone --no-checkout'. Add a test to demonstrate the changed behavior to be\nused in a later patch to verify the fix.\n\nSigned-off-by: Ben Peart \n---\n t/t2018-checkout-branch.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 2131fb2a56..6da2d4e68f 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -198,4 +198,13 @@ test_expect_success 'checkout -B to the current branch works' '\n \ttest_dirty_mergeable\n '\n \n+test_expect_failure 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n+\tgit init src &&\n+\ttest_commit -C src a &&\n+\trev=\"$(git -C src rev-parse HEAD)\" &&\n+\tgit clone --no-checkout src dest &&\n+\tgit -C dest checkout \"$rev\" -b branch &&\n+\ttest_path_is_file dest/a.t\n+'\n+\n test_done\n-- \n2.19.1.gvfs.1.16.g9d1374d\n\n"},{"id":"367254","messageId":"20190121195008.8700-3-peartben@gmail.com","threadId":"50141","inReplyTo":"20190121195008.8700-1-peartben@gmail.com","subject":"[PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-21T19:50:08Z","receivedAt":"2019-01-21T19:50:24Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nWhen doing a 'checkout -b' do a full checkout including updating the working\ntree when doing the initial checkout. This fixes the regression in behavior\ncaused by fa655d8411 (checkout: optimize \"git checkout -b \", 2018-08-16)\n\nSigned-off-by: Ben Peart \n---\n builtin/checkout.c | 7 +++++++\n t/t2018-checkout-branch.sh | 2 +-\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 6fadf412e8..9c6e94319e 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -592,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n \t * Remaining variables are not checkout options but used to track state\n \t */\n \n+\t /*\n+\t * Do the merge if this is the initial checkout\n+\t *\n+\t */\n+\tif (!file_exists(get_index_file()))\n+\t\treturn 0;\n+\n \treturn 1;\n }\n \ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 6da2d4e68f..c5014ad9a6 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -198,7 +198,7 @@ test_expect_success 'checkout -B to the current branch works' '\n \ttest_dirty_mergeable\n '\n \n-test_expect_failure 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n+test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n \tgit init src &&\n \ttest_commit -C src a &&\n \trev=\"$(git -C src rev-parse HEAD)\" &&\n-- \n2.19.1.gvfs.1.16.g9d1374d\n\n"},{"id":"367303","messageId":"nycvar.QRO.7.76.6.1901221529210.41@tvgsbejvaqbjf.bet","threadId":"50141","inReplyTo":"20190121195008.8700-3-peartben@gmail.com","subject":"Re: [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-22T14:35:21Z","receivedAt":"2019-01-22T14:35:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 21 Jan 2019, Ben Peart wrote:\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 6fadf412e8..9c6e94319e 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -592,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n> \t * Remaining variables are not checkout options but used to track state\n> \t */\n> \n> +\t /*\n> +\t * Do the merge if this is the initial checkout\n> +\t *\n> +\t */\n> +\tif (!file_exists(get_index_file()))\n\nWe had a little internal discussion whether to use `access()` or\n`file_exists()`, and I was pretty strongly in favor of `file_exists()`\nbecause it says what we care about.\n\nAnd when I looked, I found quite a few callers to `access()` that look\nlike they simply want to test whether a file exists.\n\nI also looked at the implementation of `file_exists()` and found that it\nuses `lstat()`. Peff, you introduced this (using `stat()`) in c91f0d92efb3\n(git-commit.sh: convert run_status to a C builtin, 2006-09-08), could you\nenlighten me why you chose `stat()` over `access()` (the latter seems more\nlight-weight to me)? Also, Junio, you changed it to use `lstat()` in\na50f9fc5feb0 (file_exists(): dangling symlinks do exist, 2007-11-18), do\nyou think we can/should use `access()` instead?\n\nThanks,\nDscho\n\n> +\t\treturn 0;\n> +\n> \treturn 1;\n> }\n> \n> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> index 6da2d4e68f..c5014ad9a6 100755\n> --- a/t/t2018-checkout-branch.sh\n> +++ b/t/t2018-checkout-branch.sh\n> @@ -198,7 +198,7 @@ test_expect_success 'checkout -B to the current branch works' '\n> \ttest_dirty_mergeable\n> '\n> \n> -test_expect_failure 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n> +test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n> \tgit init src &&\n> \ttest_commit -C src a &&\n> \trev=\"$(git -C src rev-parse HEAD)\" &&\n> -- \n> 2.19.1.gvfs.1.16.g9d1374d\n> \n> \n"},{"id":"367339","messageId":"xmqq8szch6u4.fsf@gitster-ct.c.googlers.com","threadId":"50141","inReplyTo":"nycvar.QRO.7.76.6.1901221529210.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T18:42:43Z","receivedAt":"2019-01-22T18:42:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I also looked at the implementation of `file_exists()` and found that it\n> uses `lstat()`. Peff, you introduced this (using `stat()`) in c91f0d92efb3\n> (git-commit.sh: convert run_status to a C builtin, 2006-09-08), could you\n> enlighten me why you chose `stat()` over `access()` (the latter seems more\n> light-weight to me)? Also, Junio, you changed it to use `lstat()` in\n> a50f9fc5feb0 (file_exists(): dangling symlinks do exist, 2007-11-18), do\n> you think we can/should use `access()` instead?\n\nGiven that the whole point of a50f9fc5 (\"file_exists(): dangling\nsymlinks do exist\", 2007-11-18) is to make sure that we say \"it\nexists\" for a symbolic link that does not point anywhere, and that\naccess(2) dereferences a symbolic link, changing it to use access(2)\nwould change the meaning of the file_exists() function. So I do not\nthink we _should_ use access(2).\n\nI however do not know if we _can_ use access(2). It takes auditing\nthe current callers and see if they truly care about knowing that a\ndangling symbolic link exists to determine if it is safe to do so.\nIf none of them does, and if we do not have an immediate plan to add\na new one that does, then we obviously can use it, but even in that\ncase we'd need a comment in *.h to warn about it.\n\n"},{"id":"367340","messageId":"20190122184959.GF4399@sigill.intra.peff.net","threadId":"50141","inReplyTo":"nycvar.QRO.7.76.6.1901221529210.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-22T18:49:59Z","receivedAt":"2019-01-22T18:50:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 22, 2019 at 03:35:21PM +0100, Johannes Schindelin wrote:\n\n> I also looked at the implementation of `file_exists()` and found that it\n> uses `lstat()`. Peff, you introduced this (using `stat()`) in c91f0d92efb3\n> (git-commit.sh: convert run_status to a C builtin, 2006-09-08), could you\n> enlighten me why you chose `stat()` over `access()` (the latter seems more\n> light-weight to me)?\n\nThat's quite a while ago, but I'm pretty sure I was just following\nexisting practice. It would be fine to switch from stat() to access().\nBut...\n\n> Also, Junio, you changed it to use `lstat()` in\n> a50f9fc5feb0 (file_exists(): dangling symlinks do exist, 2007-11-18), do\n> you think we can/should use `access()` instead?\n\nI think access() will always dereference. So it would not detect a\ndangling symlink. Whether that matters or not is going to depend on each\ncaller.\n\nI doubt it would matter much either way in this case. And I don't think\nthis is performance critical (it should be once per checkout, not once\nper file).\n\nIf there are callers that care (and I assume there are due to the\nexistence of a50f9fcfeb0), and if we do care about performance on\nplatforms where stat() is slower, it might be reasonable to have a\nplatform-specific implementation of file_exists().\n\nLikewise, anybody converting from access() should consider whether each\nsite cares about dangling symlinks (though in general, I'd expect most\nof them to simply not have thought of it, and be happy to start\nconsidering that as \"exists\").\n\n-Peff\n"},{"id":"367341","messageId":"xmqq4la0h6am.fsf@gitster-ct.c.googlers.com","threadId":"50141","inReplyTo":"20190121195008.8700-1-peartben@gmail.com","subject":"Re: [PATCH v2 0/2] Fix regression in checkout -b","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T18:54:25Z","receivedAt":"2019-01-22T18:54:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index af6b5c8336..9c6e94319e 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -517,12 +517,6 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n> \tif (core_apply_sparse_checkout && !checkout_optimize_new_branch)\n> \t\treturn 0;\n> \n> -\t/*\n> -\t * We must do the merge if this is the initial checkout\n> -\t */\n> -\tif (is_cache_unborn())\n> -\t\treturn 0;\n> -\n> \t/*\n> \t * We must do the merge if we are actually moving to a new commit.\n> \t */\n> @@ -598,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n> \t * Remaining variables are not checkout options but used to track state\n> \t */\n> \n> +\t /*\n> +\t * Do the merge if this is the initial checkout\n> +\t *\n> +\t */\n> +\tif (!file_exists(get_index_file()))\n> +\t\treturn 0;\n> +\n> \treturn 1;\n> }\n\nThis is curious. The location the new special case is added is\ndifferent, and the way the new special case is detected is also\ndifferent, between v1 and v2. Are both of them significant? IOW,\nif we moved the check down but kept using is_cache_unborn(), would\nit break? Or if we did not move the check but switched to check the\nindex file on the filesystem instead of calling is_cache_unborn(),\nwould it break?\n\nThere are three existing callers of is_{cache,index}_unborn(), all\nof which want to use it to decide if we are in this funny \"unborn\"\nstate. If this fixes the issue we saw in v1 of these two patches,\ndoes that mean these three existing callers also are buggy in the\nsame way and we are better off rewriting is_index_unborn() to see if\nthe index file is on the disk?\n\nI am *not* suggesting to make such a drastic change to the existing\nsystem. I am wondering why they are working fine but only this new\ncode has to avoid the existing is_index_unborn() logic and go\ndirectly to the filesystem. Especially as this new exception added\nto \"skip-merge-working-tree\" is to allow the special case code in\nmerge-working-tree that depends on is_cache_unborn() to trigger.\n\nThanks for working on this.\n"},{"id":"367343","messageId":"2a3ac803-133c-98fb-45e9-43f6e4a018d1@gmail.com","threadId":"50141","inReplyTo":"xmqq4la0h6am.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] Fix regression in checkout -b","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-22T19:31:16Z","receivedAt":"2019-01-22T19:31:21Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 1/22/2019 1:54 PM, Junio C Hamano wrote:\n> Ben Peart writes:\n> \n>> diff --git a/builtin/checkout.c b/builtin/checkout.c\n>> index af6b5c8336..9c6e94319e 100644\n>> --- a/builtin/checkout.c\n>> +++ b/builtin/checkout.c\n>> @@ -517,12 +517,6 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n>> \tif (core_apply_sparse_checkout && !checkout_optimize_new_branch)\n>> \t\treturn 0;\n>> \n>> -\t/*\n>> -\t * We must do the merge if this is the initial checkout\n>> -\t */\n>> -\tif (is_cache_unborn())\n>> -\t\treturn 0;\n>> -\n>> \t/*\n>> \t * We must do the merge if we are actually moving to a new commit.\n>> \t */\n>> @@ -598,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n>> \t * Remaining variables are not checkout options but used to track state\n>> \t */\n>> \n>> +\t /*\n>> +\t * Do the merge if this is the initial checkout\n>> +\t *\n>> +\t */\n>> +\tif (!file_exists(get_index_file()))\n>> +\t\treturn 0;\n>> +\n>> \treturn 1;\n>> }\n> \n> This is curious. The location the new special case is added is\n> different, and the way the new special case is detected is also\n> different, between v1 and v2. Are both of them significant? IOW,\n> if we moved the check down but kept using is_cache_unborn(), would\n> it break? Or if we did not move the check but switched to check the\n> index file on the filesystem instead of calling is_cache_unborn(),\n> would it break?\n> \n\nI had to change the check to not use is_cache_unborn() because at this \npoint, the index has not been loaded so cache_nr and timestamp.sec are \nalways zero (thus defeating the entire optimization). Since part of the \noptimization was to avoid loading the index when it isn't necessary, the \nonly replacement I could think of was to check for the existence of the \nindex file as if it is missing entirely, it is clearly unborn. This \nsolved the behavior change for the --no-checkout sequence reported.\n\nThe only reason I moved it lower in the function was a micro perf \noptimization. Since file_exists() does file I/O, I thought I'd do all \nthe in memory/flag checks first in case they drop out early and we can \navoid the unnecessary file I/O. As long as it is tested before the \n'return 1;' call, it is logically correct.\n\n> There are three existing callers of is_{cache,index}_unborn(), all\n> of which want to use it to decide if we are in this funny \"unborn\"\n> state. If this fixes the issue we saw in v1 of these two patches,\n> does that mean these three existing callers also are buggy in the\n> same way and we are better off rewriting is_index_unborn() to see if\n> the index file is on the disk?\n> \n\nIt is just the fact that I needed to check for an unborn index _before_ \nit was loaded that makes me unable to use is_{cache,index}_unborn() \nhere. The other callers should still be fine. I could add a comment in \nthe code to clarify this if you think it will cause confusion later.\n\n> I am *not* suggesting to make such a drastic change to the existing\n> system. I am wondering why they are working fine but only this new\n> code has to avoid the existing is_index_unborn() logic and go\n> directly to the filesystem. Especially as this new exception added\n> to \"skip-merge-working-tree\" is to allow the special case code in\n> merge-working-tree that depends on is_cache_unborn() to trigger.\n> \n> Thanks for working on this.\n> \n"},{"id":"367448","messageId":"20190123175756.GA6702@szeder.dev","threadId":"50141","inReplyTo":"20190121195008.8700-2-peartben@gmail.com","subject":"Re: [PATCH v2 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-23T17:57:56Z","receivedAt":"2019-01-23T17:58:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jan 21, 2019 at 02:50:07PM -0500, Ben Peart wrote:\n> From: Ben Peart \n> \n> Commit fa655d8411 (checkout: optimize \"git checkout -b \", 2018-08-16)\n> introduced an unintentional change in behavior for 'checkout -b' after doing\n> 'clone --no-checkout'. Add a test to demonstrate the changed behavior to be\n> used in a later patch to verify the fix.\n\nPlease wrap the commit message at a width of around 70 or so\ncharacters. The commit messages of both patches contain lines that\nare wider than a standard 80 char wide terminal.\n\n\n"},{"id":"367451","messageId":"xmqqsgxjb2zq.fsf@gitster-ct.c.googlers.com","threadId":"50141","inReplyTo":"2a3ac803-133c-98fb-45e9-43f6e4a018d1@gmail.com","subject":"Re: [PATCH v2 0/2] Fix regression in checkout -b","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-23T19:14:33Z","receivedAt":"2019-01-23T19:14:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n>> This is curious. The location the new special case is added is\n>> different, and the way the new special case is detected is also\n>> different, between v1 and v2. Are both of them significant? IOW,\n>> if we moved the check down but kept using is_cache_unborn(), would\n>> it break? Or if we did not move the check but switched to check the\n>> index file on the filesystem instead of calling is_cache_unborn(),\n>> would it break?\n>>\n>\n> I had to change the check to not use is_cache_unborn() because at this\n> point, the index has not been loaded so cache_nr and timestamp.sec are\n> always zero (thus defeating the entire optimization). Since part of\n> the optimization was to avoid loading the index when it isn't\n> necessary, the only replacement I could think of was to check for the\n> existence of the index file as if it is missing entirely, it is\n> clearly unborn. This solved the behavior change for the --no-checkout\n> sequence reported.\n\nAhh, chicken-and-egg. Please do add an in-code comment on why this\nis not \"is_cache_unborn()\" but must be \"file_exists()\" immediately\nbefore that if() statement.\n\n> The only reason I moved it lower in the function was a micro perf\n> optimization. Since file_exists() does file I/O, I thought I'd do all\n> the in memory/flag checks first in case they drop out early and we can\n> avoid the unnecessary file I/O. As long as it is tested before the\n> 'return 1;' call, it is logically correct.\n\nI see, and it does make sense. This explanation only matters to\nthose who read and compare v1 and v2 and much less to those who read\nand consume only v2, so it probably would have made a difference if\nit were described in the cover letter, but a passing mention in the\ncommit log message may be enough, if we wre to leave a record of the\ndecision somewhere, perhaps like \"As the new test involves an\nfilesystem access, do it later in the sequence to give chance to\nother cheaper tests to leave early\" or something at the end.\n\nThanks.\n"},{"id":"367456","messageId":"20190123200201.7396-2-peartben@gmail.com","threadId":"50141","inReplyTo":"20190123200201.7396-1-peartben@gmail.com","subject":"[PATCH v3 1/2] checkout: add test demonstrating regression with checkout -b on initial commit","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-23T20:02:00Z","receivedAt":"2019-01-23T20:02:14Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nCommit fa655d8411 (checkout: optimize \"git checkout -b \",\n2018-08-16) introduced an unintentional change in behavior for 'checkout -b'\nafter doing 'clone --no-checkout'. Add a test to demonstrate the changed\nbehavior to be used in a later patch to verify the fix.\n\nSigned-off-by: Ben Peart \n---\n t/t2018-checkout-branch.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 2131fb2a56..6da2d4e68f 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -198,4 +198,13 @@ test_expect_success 'checkout -B to the current branch works' '\n \ttest_dirty_mergeable\n '\n \n+test_expect_failure 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n+\tgit init src &&\n+\ttest_commit -C src a &&\n+\trev=\"$(git -C src rev-parse HEAD)\" &&\n+\tgit clone --no-checkout src dest &&\n+\tgit -C dest checkout \"$rev\" -b branch &&\n+\ttest_path_is_file dest/a.t\n+'\n+\n test_done\n-- \n2.19.1.gvfs.1.16.g9d1374d\n\n"},{"id":"367457","messageId":"20190123200201.7396-1-peartben@gmail.com","threadId":"50141","inReplyTo":"20190118185558.17688-1-peartben@gmail.com","subject":"[PATCH v3 0/2] Fix regression in checkout -b","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-23T20:01:59Z","receivedAt":"2019-01-23T20:02:14Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nMinor update to comment from V2. Also wrapped commit messages to be <80\nchars wide.\n\nBase Ref: master\nWeb-Diff: https://github.com/benpeart/git/commit/fef76edbdc\nCheckout: git fetch https://github.com/benpeart/git initial-checkout-v3 && git checkout fef76edbdc\n\n\n### Interdiff (v2..v3):\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9c6e94319e..9f8f3466f6 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -593,8 +593,9 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n \t */\n \n \t /*\n-\t * Do the merge if this is the initial checkout\n-\t *\n+\t * Do the merge if this is the initial checkout. We cannot use\n+\t * is_cache_unborn() here because the index hasn't been loaded yet\n+\t * so cache_nr and timestamp.sec are always zero.\n \t */\n \tif (!file_exists(get_index_file()))\n \t\treturn 0;\n\n\n### Patches\n\nBen Peart (2):\n checkout: add test demonstrating regression with checkout -b on\n initial commit\n checkout: fix regression in checkout -b on intitial checkout\n\n builtin/checkout.c | 8 ++++++++\n t/t2018-checkout-branch.sh | 9 +++++++++\n 2 files changed, 17 insertions(+)\n\n\nbase-commit: 77556354bb7ac50450e3b28999e3576969869068\n-- \n2.19.1.gvfs.1.16.g9d1374d\n\n\n"},{"id":"367458","messageId":"20190123200201.7396-3-peartben@gmail.com","threadId":"50141","inReplyTo":"20190123200201.7396-1-peartben@gmail.com","subject":"[PATCH v3 2/2] checkout: fix regression in checkout -b on intitial checkout","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2019-01-23T20:02:01Z","receivedAt":"2019-01-23T20:02:16Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nWhen doing a 'checkout -b' do a full checkout including updating the working\ntree when doing the initial checkout. As the new test involves an filesystem\naccess, do it later in the sequence to give chance to other cheaper tests to\nleave early. This fixes the regression in behavior caused by fa655d8411\n(checkout: optimize \"git checkout -b \", 2018-08-16).\n\nSigned-off-by: Ben Peart \n---\n builtin/checkout.c | 8 ++++++++\n t/t2018-checkout-branch.sh | 2 +-\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 6fadf412e8..9f8f3466f6 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -592,6 +592,14 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,\n \t * Remaining variables are not checkout options but used to track state\n \t */\n \n+\t /*\n+\t * Do the merge if this is the initial checkout. We cannot use\n+\t * is_cache_unborn() here because the index hasn't been loaded yet\n+\t * so cache_nr and timestamp.sec are always zero.\n+\t */\n+\tif (!file_exists(get_index_file()))\n+\t\treturn 0;\n+\n \treturn 1;\n }\n \ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 6da2d4e68f..c5014ad9a6 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -198,7 +198,7 @@ test_expect_success 'checkout -B to the current branch works' '\n \ttest_dirty_mergeable\n '\n \n-test_expect_failure 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n+test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '\n \tgit init src &&\n \ttest_commit -C src a &&\n \trev=\"$(git -C src rev-parse HEAD)\" &&\n-- \n2.19.1.gvfs.1.16.g9d1374d\n\n"},{"id":"367483","messageId":"xmqqr2d39ih3.fsf@gitster-ct.c.googlers.com","threadId":"50141","inReplyTo":"20190123200201.7396-1-peartben@gmail.com","subject":"Re: [PATCH v3 0/2] Fix regression in checkout -b","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-23T21:23:04Z","receivedAt":"2019-01-23T21:23:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> From: Ben Peart \n>\n> Minor update to comment from V2. Also wrapped commit messages to be <80\n> chars wide.\n\nPerfect. Thanks.\n"}]}