Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files

Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files

[Anthony Sottile]

Here's a simple regression test -- haven't had time to bisect this

```
#!/usr/bin/env bash
set -euxo pipefail

rm -rf src dest

git --version

git init src
echo hi > src/a
git -C src add .
git -C src commit -m "initial commit"
rev="$(git -C src rev-parse HEAD)"

git clone --no-checkout src dest
git -C dest checkout "$rev" -b branch
test -f dest/a

: 'SUCCESS!'
```

With git 2.17.1

```
+ set -euo pipefail
+ rm -rf src dest
+ git --version
git version 2.17.1
+ git init src
Initialized empty Git repository in /tmp/t/src/.git/
+ echo hi
+ git -C src add .
+ git -C src commit -m 'initial commit'
[master (root-commit) 61ae2ae] initial commit
 1 file changed, 1 insertion(+)
 create mode 100644 a
++ git -C src rev-parse HEAD
+ rev=61ae2ae9c9a96de7b1688b095f20a6adf9c20db1
+ git clone --no-checkout src dest
Cloning into 'dest'...
done.
+ git -C dest checkout 61ae2ae9c9a96de7b1688b095f20a6adf9c20db1 -b branch
Switched to a new branch 'branch'
+ test -f dest/a
+ : 'SUCCESS!'
```

With git 2.20.GIT (b21ebb671bb7dea8d342225f0d66c41f4e54d5ca)

```
+ set -euo pipefail
+ rm -rf src dest
+ git --version
git version 2.20.GIT
+ git init src
Initialized empty Git repository in /tmp/t/src/.git/
+ echo hi
+ git -C src add .
+ git -C src commit -m 'initial commit'
[master (root-commit) df4d6dc] initial commit
 1 file changed, 1 insertion(+)
 create mode 100644 a
++ git -C src rev-parse HEAD
+ rev=df4d6dcf02b15bfe2b3f0e4a8aa25ac165b1368c
+ git clone --no-checkout src dest
Cloning into 'dest'...
done.
+ git -C dest checkout df4d6dcf02b15bfe2b3f0e4a8aa25ac165b1368c -b branch
D    a
Switched to a new branch 'branch'
+ test -f dest/a
```

A workaround for my use case is to not use `--no-checkout`, though
this is potentially slow

Anthony

Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files

[Duy Nguyen]

On Wed, Jan 2, 2019 at 6:36 AM Anthony Sottile <asottile@umich.edu> wrote:
>
> Here's a simple regression test -- haven't had time to bisect this

I can't reproduce with either 2.20.0, 2.20.1 or 'master'. It would be
great if you could bisect this.

There are no suspicious commits from 2.27.1 touching
builtin/checkout.c. Though there are some more changes in
unpack-trees.c that might cause this (big wild guess).
-- 
Duy

Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files

[Anthony Sottile]

On Wed, Jan 2, 2019 at 3:08 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Wed, Jan 2, 2019 at 6:36 AM Anthony Sottile <asottile@umich.edu> wrote:
> >
> > Here's a simple regression test -- haven't had time to bisect this
>
> I can't reproduce with either 2.20.0, 2.20.1 or 'master'. It would be
> great if you could bisect this.
>
> There are no suspicious commits from 2.27.1 touching
> builtin/checkout.c. Though there are some more changes in
> unpack-trees.c that might cause this (big wild guess).
> --
> Duy


heated a small room but here's the results of the bisect!

fa655d8411cc2d7ffcf898e53a1493c737d7de68 is the first bad commit
commit fa655d8411cc2d7ffcf898e53a1493c737d7de68
Author: Ben Peart <Ben.Peart@microsoft.com>
Date:   Thu Aug 16 18:27:11 2018 +0000

    checkout: optimize "git checkout -b <new_branch>"

    Skip merging the commit, updating the index and working directory if and
    only if we are creating a new branch via "git checkout -b <new_branch>."
    Any other checkout options will still go through the former code path.

    If sparse_checkout is on, require the user to manually opt in to this
    optimzed behavior by setting the config setting checkout.optimizeNewBranch
    to true as we will no longer update the skip-worktree bit in the index, nor
    add/remove files in the working directory to reflect the current sparse
    checkout settings.

    For comparison, running "git checkout -b <new_branch>" on a large
repo takes:

    14.6 seconds - without this patch
    0.3 seconds - with this patch

    Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

:040000 040000 817bfb8ef961545a554005d42967b5ab7cfdb041
e57e576d0d4fb7f25c12a5dcc7651ef6698e961b M    Documentation
:040000 040000 c089f91f4532caa2a17e4f10a1a7ed3aa5d2023c
7cf16a0aa288f898a880ffefe82ee7506b83bef4 M    builtin
:040000 040000 adfdb05964a692e03ee07d2e43841f6304d996bd
8681416093802b9051599ebea8f63f5a45968e6f M    t
bisect run success


Here's the script and invocations:

```
#!/usr/bin/env bash
set -euxo pipefail

rm -rf "$PWD/prefix"
make prefix="$PWD/prefix" -j8 install
export PATH="$PWD/prefix/bin:$PATH"

rm -rf src dest

git --version

git init src
echo hi > src/a
git -C src add .
git -C src commit -m "initial commit"
rev="$(git -C src rev-parse HEAD)"

git clone --no-checkout src dest
git -C dest checkout "$rev" -b branch
test -f dest/a

: 'SUCCESS!'
```

```
git bisect begin
git bisect bad HEAD
git bisect good v2.17.1
git bisect run ./bisect.sh
```

Anthony

Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files

[Duy Nguyen]

On Wed, Jan 2, 2019 at 11:18 PM Anthony Sottile <asottile@umich.edu> wrote:
> heated a small room but here's the results of the bisect!
>
> fa655d8411cc2d7ffcf898e53a1493c737d7de68 is the first bad commit
> commit fa655d8411cc2d7ffcf898e53a1493c737d7de68
> Author: Ben Peart <Ben.Peart@microsoft.com>
> Date:   Thu Aug 16 18:27:11 2018 +0000
>
>     checkout: optimize "git checkout -b <new_branch>"

I did test this commit before and could not reproduce. But one thing I
did not notice is I set sparsecheckout on by default. which always
forces this optimization off Turning off sparse checkout, I could
indeed reproduce.

I plan to revert this commit anyway when the new command "git
switch-branch" comes. The optimization will be unconditionally in the
new command without this hack and users are encouraged to use that one
instead of "git checkout".

Meanwhile, let's see if Ben wants to fix this or revert it.

>
>     Skip merging the commit, updating the index and working directory if and
>     only if we are creating a new branch via "git checkout -b <new_branch>."
>     Any other checkout options will still go through the former code path.
>
>     If sparse_checkout is on, require the user to manually opt in to this
>     optimzed behavior by setting the config setting checkout.optimizeNewBranch
>     to true as we will no longer update the skip-worktree bit in the index, nor
>     add/remove files in the working directory to reflect the current sparse
>     checkout settings.
>
>     For comparison, running "git checkout -b <new_branch>" on a large
> repo takes:
>
>     14.6 seconds - without this patch
>     0.3 seconds - with this patch
>
>     Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> :040000 040000 817bfb8ef961545a554005d42967b5ab7cfdb041
> e57e576d0d4fb7f25c12a5dcc7651ef6698e961b M    Documentation
> :040000 040000 c089f91f4532caa2a17e4f10a1a7ed3aa5d2023c
> 7cf16a0aa288f898a880ffefe82ee7506b83bef4 M    builtin
> :040000 040000 adfdb05964a692e03ee07d2e43841f6304d996bd
> 8681416093802b9051599ebea8f63f5a45968e6f M    t
> bisect run success
>
>
> Here's the script and invocations:
>
> ```
> #!/usr/bin/env bash
> set -euxo pipefail
>
> rm -rf "$PWD/prefix"
> make prefix="$PWD/prefix" -j8 install
> export PATH="$PWD/prefix/bin:$PATH"
>
> rm -rf src dest
>
> git --version
>
> git init src
> echo hi > src/a
> git -C src add .
> git -C src commit -m "initial commit"
> rev="$(git -C src rev-parse HEAD)"
>
> git clone --no-checkout src dest
> git -C dest checkout "$rev" -b branch
> test -f dest/a
>
> : 'SUCCESS!'
> ```
>
> ```
> git bisect begin
> git bisect bad HEAD
> git bisect good v2.17.1
> git bisect run ./bisect.sh
> ```
>
> Anthony



-- 
Duy

Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> I plan to revert this commit anyway when the new command "git
> switch-branch" comes. The optimization will be unconditionally in the
> new command without this hack and users are encouraged to use that one
> instead of "git checkout".

I tend to think that the behaviour is perfectly in line with what
Ben wanted to have, which is to make "checkout -b new [HEAD]" not to
touch anything in the index or the working tree at all.

It further is possible to argue that what is strange in the whole
episode is what "clone --no-checkout" does.  In such a repository,
if you say "git status", you'd notice that it is reported that all
paths have been deleted.

Now, if you instead do

	git clone $src dst
	cd dst
	git rm file
	git checkout -b new

i.e. starting from a clone with normal working tree, manually
removing a path or two, and then create a new branch starting from
that state while carrying all the local changes, you *do* want to
see that 'file' to stay missing.  After all, "do not lose the local
changes; carry them forward" is what switching branches is about.

And from that point of view, we could consider that

	git clone --no-checkout $src $dst

is equivalent to

	git clone $src $dst && git -C $dst rm -r .

Having said all that.

> Meanwhile, let's see if Ben wants to fix this or revert it.

A "fix" to Ben's optimization for this particular case should be
fairly straight-forward.  I think we have a special case in the
checkout codepath for an initial checkout and disable "carry forward
the fact that the user wanted all the paths removed", so it would be
the matter of adding yet another condition (is_cache_unborn(), which
is used to set topts.initial_checkout) to the large collection of
conditions in skip_merge_working_tree().

Back when the "optimization" was discussed, all reviewers said that
it would become maintenance nightmare to ensure that the set of
conditions accurately tracks the case where the optimization is
safe.  Now they are entitled to say "we told you so".

Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files

[Anthony Sottile]

On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano <gitster@pobox.com> wrote:
> A "fix" to Ben's optimization for this particular case should be
> fairly straight-forward.  I think we have a special case in the
> checkout codepath for an initial checkout and disable "carry forward
> the fact that the user wanted all the paths removed", so it would be
> the matter of adding yet another condition (is_cache_unborn(), which
> is used to set topts.initial_checkout) to the large collection of
> conditions in skip_merge_working_tree().
>

I think it might be simpler than that even -- the optimization treats
the following as equivalent when the current checked out revision is
deadbeef (even if the index / worktree differ), when before they were
not:

- git checkout -b newbranch
- git checkout deadbeef -b newbranch

If a revision is specified on the commandline it should be checked out.

Anthony

Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files

[Junio C Hamano]

Anthony Sottile <asottile@umich.edu> writes:

> On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>> A "fix" to Ben's optimization for this particular case should be
>> fairly straight-forward.  I think we have a special case in the
>> checkout codepath for an initial checkout and disable "carry forward
>> the fact that the user wanted all the paths removed", so it would be
>> the matter of adding yet another condition (is_cache_unborn(), which
>> is used to set topts.initial_checkout) to the large collection of
>> conditions in skip_merge_working_tree().
>
> I think it might be simpler than that even -- the optimization treats
> the following as equivalent when the current checked out revision is
> deadbeef (even if the index / worktree differ), when before they were
> not:
>
> - git checkout -b newbranch
> - git checkout deadbeef -b newbranch
>
> If a revision is specified on the commandline it should be checked out.

If it were to be a "fix", the exact same command line as people used
to be able to use, i.e. "git checkout -b newbranch", should be made
to do what it used to do.

Forcing users to use a different command to workaround the bug is
not a usable "fix".  If we want a working workaround, you can tell
your users to use 

    git reset --hard HEAD && git checkout -b newbranch

and that would already work without any code change ;-).

Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files

[Anthony Sottile]

On Thu, Jan 3, 2019 at 1:51 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Anthony Sottile <asottile@umich.edu> writes:
>
> > On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> A "fix" to Ben's optimization for this particular case should be
> >> fairly straight-forward.  I think we have a special case in the
> >> checkout codepath for an initial checkout and disable "carry forward
> >> the fact that the user wanted all the paths removed", so it would be
> >> the matter of adding yet another condition (is_cache_unborn(), which
> >> is used to set topts.initial_checkout) to the large collection of
> >> conditions in skip_merge_working_tree().
> >
> > I think it might be simpler than that even -- the optimization treats
> > the following as equivalent when the current checked out revision is
> > deadbeef (even if the index / worktree differ), when before they were
> > not:
> >
> > - git checkout -b newbranch
> > - git checkout deadbeef -b newbranch
> >
> > If a revision is specified on the commandline it should be checked out.
>
> If it were to be a "fix", the exact same command line as people used
> to be able to use, i.e. "git checkout -b newbranch", should be made
> to do what it used to do.
>
> Forcing users to use a different command to workaround the bug is
> not a usable "fix".  If we want a working workaround, you can tell
> your users to use
>
>     git reset --hard HEAD && git checkout -b newbranch
>
> and that would already work without any code change ;-).
>
>

oh wow, I didn't realize `git checkout -b newbranch` also used to
reset the `--no-checkout` state, yeah you're right the optimization is
way more problematic than I had considered.

I'm working around by not using `--no-checkout` personally

Anthony

Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files

[Ben Peart]

On 1/3/2019 5:05 PM, Anthony Sottile wrote:
> On Thu, Jan 3, 2019 at 1:51 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Anthony Sottile <asottile@umich.edu> writes:
>>
>>> On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>>>> A "fix" to Ben's optimization for this particular case should be
>>>> fairly straight-forward.  I think we have a special case in the
>>>> checkout codepath for an initial checkout and disable "carry forward
>>>> the fact that the user wanted all the paths removed", so it would be
>>>> the matter of adding yet another condition (is_cache_unborn(), which
>>>> is used to set topts.initial_checkout) to the large collection of
>>>> conditions in skip_merge_working_tree().
>>>
>>> I think it might be simpler than that even -- the optimization treats
>>> the following as equivalent when the current checked out revision is
>>> deadbeef (even if the index / worktree differ), when before they were
>>> not:
>>>
>>> - git checkout -b newbranch
>>> - git checkout deadbeef -b newbranch
>>>
>>> If a revision is specified on the commandline it should be checked out.
>>
>> If it were to be a "fix", the exact same command line as people used
>> to be able to use, i.e. "git checkout -b newbranch", should be made
>> to do what it used to do.
>>
>> Forcing users to use a different command to workaround the bug is
>> not a usable "fix".  If we want a working workaround, you can tell
>> your users to use
>>
>>      git reset --hard HEAD && git checkout -b newbranch
>>
>> and that would already work without any code change ;-).
>>
>>

Just noticed this thread.  I agree that the behavior of `git clone 
--no-checkout` is a little odd in that it shows everything as deleted 
but the goal of the `checkout -b` optimization was to not change 
behavior (unless the user opt-ed in to the changed behavior via 
checkout.optimizeNewBranch).  I'll work on a patch to detect this case 
and ensure the default behavior doesn't change.

> 
> oh wow, I didn't realize `git checkout -b newbranch` also used to
> reset the `--no-checkout` state, yeah you're right the optimization is
> way more problematic than I had considered.
> 
> I'm working around by not using `--no-checkout` personally
> 
> Anthony
> 

[PATCH v1 0/2] Fix regression in checkout -b

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Anthony Sottile <asottile@umich.edu> determined that commit fa655d8411
"checkout: optimize "git checkout -b <new_branch>" introduced
an unintentional change in behavior for 'checkout -b' after doing a
'clone --no-checkout'.  Create a test to demonstrate the regression then
fix the bug and update the test to demonstrate the fix.

Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/3930f7fa32
Checkout: git fetch https://github.com/benpeart/git initial-checkout-v1 && git checkout 3930f7fa32

Ben Peart (2):
  checkout: add test to demonstrate regression with checkout -b on
    initial commit
  checkout: fix regression in checkout -b on intitial checkout

 builtin/checkout.c         |  6 ++++++
 t/t2018-checkout-branch.sh | 11 +++++++++++
 2 files changed, 17 insertions(+)


base-commit: 77556354bb7ac50450e3b28999e3576969869068
-- 
2.19.1.gvfs.1.16.g9d1374d

[PATCH v1 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Commit fa655d8411 checkout: optimize "git checkout -b <new_branch>" introduced
an unintentional change in behavior for 'checkout -b' after doing a
'clone --no-checkout'.  Add a test to demonstrate the changed behavior to be
used in a later patch to verify the fix.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 t/t2018-checkout-branch.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 2131fb2a56..35999b3adb 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -198,4 +198,15 @@ test_expect_success 'checkout -B to the current branch works' '
 	test_dirty_mergeable
 '
 
+test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '
+	git init src &&
+	echo hi > src/a &&
+	git -C src add . &&
+	git -C src commit -m "initial commit" &&
+	rev="$(git -C src rev-parse HEAD)" &&
+	git clone --no-checkout src dest &&
+	git -C dest checkout "$rev" -b branch &&
+	test_must_fail test -f dest/a
+'
+
 test_done
-- 
2.19.1.gvfs.1.16.g9d1374d

Re: [PATCH v1 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit

[SZEDER Gábor]

On Fri, Jan 18, 2019 at 01:55:57PM -0500, Ben Peart wrote:
> From: Ben Peart <benpeart@microsoft.com>
> 
> Commit fa655d8411 checkout: optimize "git checkout -b <new_branch>" introduced

Style nit: fa655d8411 (checkout: optimize "git checkout -b
<new_branch>", 2018-08-16)

Furthermore, please wrap the commit message at a width of around 70 or
so chars.

> an unintentional change in behavior for 'checkout -b' after doing a
> 'clone --no-checkout'.  Add a test to demonstrate the changed behavior to be
> used in a later patch to verify the fix.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
>  t/t2018-checkout-branch.sh | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> index 2131fb2a56..35999b3adb 100755
> --- a/t/t2018-checkout-branch.sh
> +++ b/t/t2018-checkout-branch.sh
> @@ -198,4 +198,15 @@ test_expect_success 'checkout -B to the current branch works' '
>  	test_dirty_mergeable
>  '
>  
> +test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '

As this test is supposed to demonstrate a regression, this should be
test_expect_failure (to be flipped to test_expect_success in the next
patch fixing the regression).

> +	git init src &&
> +	echo hi > src/a &&

Style nit: no space between redirection and filename, but...

> +	git -C src add . &&
> +	git -C src commit -m "initial commit" &&

The above three lines could be replaced by a single

  test_commit -C src a

command.

> +	rev="$(git -C src rev-parse HEAD)" &&
> +	git clone --no-checkout src dest &&
> +	git -C dest checkout "$rev" -b branch &&
> +	test_must_fail test -f dest/a

And here the test should expect the file to be there even when
demonstrating the regression.

Please use the 'test_path_is_file' helper instead of 'test -f', as it
gives a useful error on failure.

> +'
> +
>  test_done
> -- 
> 2.19.1.gvfs.1.16.g9d1374d
> 

[PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

When doing a 'checkout -b' do a full checkout including updating the working
tree when doing the initial checkout.  This fixes the regression in behavior
caused by fa655d8411 checkout: optimize "git checkout -b <new_branch>"

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 builtin/checkout.c         | 6 ++++++
 t/t2018-checkout-branch.sh | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6fadf412e8..af6b5c8336 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -517,6 +517,12 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
 	if (core_apply_sparse_checkout && !checkout_optimize_new_branch)
 		return 0;
 
+	/*
+	 * We must do the merge if this is the initial checkout
+	 */
+	if (is_cache_unborn())
+		return 0;
+
 	/*
 	 * We must do the merge if we are actually moving to a new commit.
 	 */
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 35999b3adb..c438889b0c 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -206,7 +206,7 @@ test_expect_success 'checkout -b after clone --no-checkout does a checkout of HE
 	rev="$(git -C src rev-parse HEAD)" &&
 	git clone --no-checkout src dest &&
 	git -C dest checkout "$rev" -b branch &&
-	test_must_fail test -f dest/a
+	test -f dest/a
 '
 
 test_done
-- 
2.19.1.gvfs.1.16.g9d1374d

Re: [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> From: Ben Peart <benpeart@microsoft.com>
>
> When doing a 'checkout -b' do a full checkout including updating the working
> tree when doing the initial checkout.  This fixes the regression in behavior
> caused by fa655d8411 checkout: optimize "git checkout -b <new_branch>"
>
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
>  builtin/checkout.c         | 6 ++++++
>  t/t2018-checkout-branch.sh | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 6fadf412e8..af6b5c8336 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -517,6 +517,12 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
>  	if (core_apply_sparse_checkout && !checkout_optimize_new_branch)
>  		return 0;
>  
> +	/*
> +	 * We must do the merge if this is the initial checkout
> +	 */
> +	if (is_cache_unborn())
> +		return 0;
> +

Yup, that's a trivial fix ;-)

>  	/*
>  	 * We must do the merge if we are actually moving to a new commit.
>  	 */
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> index 35999b3adb..c438889b0c 100755
> --- a/t/t2018-checkout-branch.sh
> +++ b/t/t2018-checkout-branch.sh
> @@ -206,7 +206,7 @@ test_expect_success 'checkout -b after clone --no-checkout does a checkout of HE
>  	rev="$(git -C src rev-parse HEAD)" &&
>  	git clone --no-checkout src dest &&
>  	git -C dest checkout "$rev" -b branch &&
> -	test_must_fail test -f dest/a
> +	test -f dest/a
>  '

This is flipping the wrong thing.  Rather, introduce the whole test
as test_expect_failure that wants to make sure dest/a exists, and
with this 2/2 patch flip test_expect_failure into test_expect_success.

Thanks.

Re: [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout

[SZEDER Gábor]

On Fri, Jan 18, 2019 at 01:55:58PM -0500, Ben Peart wrote:
> From: Ben Peart <benpeart@microsoft.com>
> 
> When doing a 'checkout -b' do a full checkout including updating the working
> tree when doing the initial checkout.  This fixes the regression in behavior
> caused by fa655d8411 checkout: optimize "git checkout -b <new_branch>"
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
>  builtin/checkout.c         | 6 ++++++
>  t/t2018-checkout-branch.sh | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 6fadf412e8..af6b5c8336 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -517,6 +517,12 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
>  	if (core_apply_sparse_checkout && !checkout_optimize_new_branch)
>  		return 0;
>  
> +	/*
> +	 * We must do the merge if this is the initial checkout
> +	 */
> +	if (is_cache_unborn())
> +		return 0;
> +
>  	/*
>  	 * We must do the merge if we are actually moving to a new commit.
>  	 */

This patch breaks 'checkout -b checkout.optimizeNewBranch interaction'
in 't1090-sparse-checkout-scope.sh':

  + cp .git/info/sparse-checkout .git/info/sparse-checkout.bak
  + test_when_finished
                  mv -f .git/info/sparse-checkout.bak .git/info/sparse-checkout
                  git checkout master
  
  + test 0 = 0
  + test_cleanup={
                  mv -f .git/info/sparse-checkout.bak .git/info/sparse-checkout
                  git checkout master
  
                  } && (exit "$eval_ret"); eval_ret=$?; :
  + echo /b
  + git ls-files -t b
  + test S b = S b
  + git -c checkout.optimizeNewBranch=true checkout -b fast
  Switched to a new branch 'fast'
  + git ls-files -t b
  + test H b = S b
  error: last command exited with $?=1
  not ok 4 - checkout -b checkout.optimizeNewBranch interaction

Re: [PATCH v1 0/2] Fix regression in checkout -b

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

>   checkout: add test to demonstrate regression with checkout -b on
>     initial commit
>   checkout: fix regression in checkout -b on intitial checkout
>
>  builtin/checkout.c         |  6 ++++++
>  t/t2018-checkout-branch.sh | 11 +++++++++++
>  2 files changed, 17 insertions(+)
>
>
> base-commit: 77556354bb7ac50450e3b28999e3576969869068

After applying these two patches on this exact commit, I tried to
run the usual test; t1090 seems to break.

[PATCH v2 0/2] Fix regression in checkout -b

[Ben Peart]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 2703 bytes --]

From: Ben Peart <benpeart@microsoft.com>

The optimized `checkout -b` doesn’t typically create/update the index and
working directory.  Add a new test to detect the case when the call to
`checkout -b` is the first call after doing a `clone --no-checkout` and no
index exists.  In this specific case, well now make the call to
merge_working_tree() which will create the index and update the working
directory.

Also simplify and update the test cases based on the feedback provided by
Szeder and Junio.  A shout out to Johannes who diagnosed, fixed and patched
a bug that was preventing me from running the git test suite in the master
branch.

Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/55dd8602f5
Checkout: git fetch https://github.com/benpeart/git initial-checkout-v2 && git checkout 55dd8602f5


### Interdiff (v1..v2):

diff --git a/builtin/checkout.c b/builtin/checkout.c
index af6b5c8336..9c6e94319e 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -517,12 +517,6 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
 	if (core_apply_sparse_checkout && !checkout_optimize_new_branch)
 		return 0;
 
-	/*
-	 * We must do the merge if this is the initial checkout
-	 */
-	if (is_cache_unborn())
-		return 0;
-
 	/*
 	 * We must do the merge if we are actually moving to a new commit.
 	 */
@@ -598,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
 	 * Remaining variables are not checkout options but used to track state
 	 */
 
+	 /*
+	  * Do the merge if this is the initial checkout
+	  *
+	  */
+	if (!file_exists(get_index_file()))
+		return 0;
+
 	return 1;
 }
 
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index c438889b0c..c5014ad9a6 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -200,13 +200,11 @@ test_expect_success 'checkout -B to the current branch works' '
 
 test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '
 	git init src &&
-	echo hi > src/a &&
-	git -C src add . &&
-	git -C src commit -m "initial commit" &&
+	test_commit -C src a &&
 	rev="$(git -C src rev-parse HEAD)" &&
 	git clone --no-checkout src dest &&
 	git -C dest checkout "$rev" -b branch &&
-	test -f dest/a
+	test_path_is_file dest/a.t
 '
 
 test_done


### Patches

Ben Peart (2):
  checkout: add test to demonstrate regression with checkout -b on
    initial commit
  checkout: fix regression in checkout -b on intitial checkout

 builtin/checkout.c         | 7 +++++++
 t/t2018-checkout-branch.sh | 9 +++++++++
 2 files changed, 16 insertions(+)


base-commit: 77556354bb7ac50450e3b28999e3576969869068
-- 
2.19.1.gvfs.1.16.g9d1374d

[PATCH v2 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Commit fa655d8411 (checkout: optimize "git checkout -b <new_branch>", 2018-08-16)
introduced an unintentional change in behavior for 'checkout -b' after doing
'clone --no-checkout'.  Add a test to demonstrate the changed behavior to be
used in a later patch to verify the fix.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 t/t2018-checkout-branch.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 2131fb2a56..6da2d4e68f 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -198,4 +198,13 @@ test_expect_success 'checkout -B to the current branch works' '
 	test_dirty_mergeable
 '
 
+test_expect_failure 'checkout -b after clone --no-checkout does a checkout of HEAD' '
+	git init src &&
+	test_commit -C src a &&
+	rev="$(git -C src rev-parse HEAD)" &&
+	git clone --no-checkout src dest &&
+	git -C dest checkout "$rev" -b branch &&
+	test_path_is_file dest/a.t
+'
+
 test_done
-- 
2.19.1.gvfs.1.16.g9d1374d

Re: [PATCH v2 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit

[SZEDER Gábor]

On Mon, Jan 21, 2019 at 02:50:07PM -0500, Ben Peart wrote:
> From: Ben Peart <benpeart@microsoft.com>
> 
> Commit fa655d8411 (checkout: optimize "git checkout -b <new_branch>", 2018-08-16)
> introduced an unintentional change in behavior for 'checkout -b' after doing
> 'clone --no-checkout'.  Add a test to demonstrate the changed behavior to be
> used in a later patch to verify the fix.

Please wrap the commit message at a width of around 70 or so
characters.  The commit messages of both patches contain lines that
are wider than a standard 80 char wide terminal.

[PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

When doing a 'checkout -b' do a full checkout including updating the working
tree when doing the initial checkout.  This fixes the regression in behavior
caused by fa655d8411 (checkout: optimize "git checkout -b <new_branch>", 2018-08-16)

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 builtin/checkout.c         | 7 +++++++
 t/t2018-checkout-branch.sh | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6fadf412e8..9c6e94319e 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -592,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
 	 * Remaining variables are not checkout options but used to track state
 	 */
 
+	 /*
+	  * Do the merge if this is the initial checkout
+	  *
+	  */
+	if (!file_exists(get_index_file()))
+		return 0;
+
 	return 1;
 }
 
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 6da2d4e68f..c5014ad9a6 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -198,7 +198,7 @@ test_expect_success 'checkout -B to the current branch works' '
 	test_dirty_mergeable
 '
 
-test_expect_failure 'checkout -b after clone --no-checkout does a checkout of HEAD' '
+test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '
 	git init src &&
 	test_commit -C src a &&
 	rev="$(git -C src rev-parse HEAD)" &&
-- 
2.19.1.gvfs.1.16.g9d1374d

Re: [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout

[Johannes Schindelin]

Hi,

On Mon, 21 Jan 2019, Ben Peart wrote:

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 6fadf412e8..9c6e94319e 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -592,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
>  	 * Remaining variables are not checkout options but used to track state
>  	 */
>  
> +	 /*
> +	  * Do the merge if this is the initial checkout
> +	  *
> +	  */
> +	if (!file_exists(get_index_file()))

We had a little internal discussion whether to use `access()` or
`file_exists()`, and I was pretty strongly in favor of `file_exists()`
because it says what we care about.

And when I looked, I found quite a few callers to `access()` that look
like they simply want to test whether a file exists.

I also looked at the implementation of `file_exists()` and found that it
uses `lstat()`. Peff, you introduced this (using `stat()`) in c91f0d92efb3
(git-commit.sh: convert run_status to a C builtin, 2006-09-08), could you
enlighten me why you chose `stat()` over `access()` (the latter seems more
light-weight to me)? Also, Junio, you changed it to use `lstat()` in
a50f9fc5feb0 (file_exists(): dangling symlinks do exist, 2007-11-18), do
you think we can/should use `access()` instead?

Thanks,
Dscho

> +		return 0;
> +
>  	return 1;
>  }
>  
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> index 6da2d4e68f..c5014ad9a6 100755
> --- a/t/t2018-checkout-branch.sh
> +++ b/t/t2018-checkout-branch.sh
> @@ -198,7 +198,7 @@ test_expect_success 'checkout -B to the current branch works' '
>  	test_dirty_mergeable
>  '
>  
> -test_expect_failure 'checkout -b after clone --no-checkout does a checkout of HEAD' '
> +test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '
>  	git init src &&
>  	test_commit -C src a &&
>  	rev="$(git -C src rev-parse HEAD)" &&
> -- 
> 2.19.1.gvfs.1.16.g9d1374d
> 
> 

Re: [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I also looked at the implementation of `file_exists()` and found that it
> uses `lstat()`. Peff, you introduced this (using `stat()`) in c91f0d92efb3
> (git-commit.sh: convert run_status to a C builtin, 2006-09-08), could you
> enlighten me why you chose `stat()` over `access()` (the latter seems more
> light-weight to me)? Also, Junio, you changed it to use `lstat()` in
> a50f9fc5feb0 (file_exists(): dangling symlinks do exist, 2007-11-18), do
> you think we can/should use `access()` instead?

Given that the whole point of a50f9fc5 ("file_exists(): dangling
symlinks do exist", 2007-11-18) is to make sure that we say "it
exists" for a symbolic link that does not point anywhere, and that
access(2) dereferences a symbolic link, changing it to use access(2)
would change the meaning of the file_exists() function.  So I do not
think we _should_ use access(2).

I however do not know if we _can_ use access(2).  It takes auditing
the current callers and see if they truly care about knowing that a
dangling symbolic link exists to determine if it is safe to do so.
If none of them does, and if we do not have an immediate plan to add
a new one that does, then we obviously can use it, but even in that
case we'd need a comment in *.h to warn about it.

Re: [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout

[Jeff King]

On Tue, Jan 22, 2019 at 03:35:21PM +0100, Johannes Schindelin wrote:

> I also looked at the implementation of `file_exists()` and found that it
> uses `lstat()`. Peff, you introduced this (using `stat()`) in c91f0d92efb3
> (git-commit.sh: convert run_status to a C builtin, 2006-09-08), could you
> enlighten me why you chose `stat()` over `access()` (the latter seems more
> light-weight to me)?

That's quite a while ago, but I'm pretty sure I was just following
existing practice. It would be fine to switch from stat() to access().
But...

> Also, Junio, you changed it to use `lstat()` in
> a50f9fc5feb0 (file_exists(): dangling symlinks do exist, 2007-11-18), do
> you think we can/should use `access()` instead?

I think access() will always dereference. So it would not detect a
dangling symlink. Whether that matters or not is going to depend on each
caller.

I doubt it would matter much either way in this case. And I don't think
this is performance critical (it should be once per checkout, not once
per file).

If there are callers that care (and I assume there are due to the
existence of a50f9fcfeb0), and if we do care about performance on
platforms where stat() is slower, it might be reasonable to have a
platform-specific implementation of file_exists().

Likewise, anybody converting from access() should consider whether each
site cares about dangling symlinks (though in general, I'd expect most
of them to simply not have thought of it, and be happy to start
considering that as "exists").

-Peff

Re: [PATCH v2 0/2] Fix regression in checkout -b

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index af6b5c8336..9c6e94319e 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -517,12 +517,6 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
>  	if (core_apply_sparse_checkout && !checkout_optimize_new_branch)
>  		return 0;
>  
> -	/*
> -	 * We must do the merge if this is the initial checkout
> -	 */
> -	if (is_cache_unborn())
> -		return 0;
> -
>  	/*
>  	 * We must do the merge if we are actually moving to a new commit.
>  	 */
> @@ -598,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
>  	 * Remaining variables are not checkout options but used to track state
>  	 */
>  
> +	 /*
> +	  * Do the merge if this is the initial checkout
> +	  *
> +	  */
> +	if (!file_exists(get_index_file()))
> +		return 0;
> +
>  	return 1;
>  }

This is curious.  The location the new special case is added is
different, and the way the new special case is detected is also
different, between v1 and v2.  Are both of them significant?  IOW,
if we moved the check down but kept using is_cache_unborn(), would
it break?  Or if we did not move the check but switched to check the
index file on the filesystem instead of calling is_cache_unborn(),
would it break?

There are three existing callers of is_{cache,index}_unborn(), all
of which want to use it to decide if we are in this funny "unborn"
state.  If this fixes the issue we saw in v1 of these two patches,
does that mean these three existing callers also are buggy in the
same way and we are better off rewriting is_index_unborn() to see if
the index file is on the disk?

I am *not* suggesting to make such a drastic change to the existing
system.  I am wondering why they are working fine but only this new
code has to avoid the existing is_index_unborn() logic and go
directly to the filesystem.  Especially as this new exception added
to "skip-merge-working-tree" is to allow the special case code in
merge-working-tree that depends on is_cache_unborn() to trigger.

Thanks for working on this.

Re: [PATCH v2 0/2] Fix regression in checkout -b

[Ben Peart]

On 1/22/2019 1:54 PM, Junio C Hamano wrote:
> Ben Peart <peartben@gmail.com> writes:
> 
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index af6b5c8336..9c6e94319e 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -517,12 +517,6 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
>>   	if (core_apply_sparse_checkout && !checkout_optimize_new_branch)
>>   		return 0;
>>   
>> -	/*
>> -	 * We must do the merge if this is the initial checkout
>> -	 */
>> -	if (is_cache_unborn())
>> -		return 0;
>> -
>>   	/*
>>   	 * We must do the merge if we are actually moving to a new commit.
>>   	 */
>> @@ -598,6 +592,13 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
>>   	 * Remaining variables are not checkout options but used to track state
>>   	 */
>>   
>> +	 /*
>> +	  * Do the merge if this is the initial checkout
>> +	  *
>> +	  */
>> +	if (!file_exists(get_index_file()))
>> +		return 0;
>> +
>>   	return 1;
>>   }
> 
> This is curious.  The location the new special case is added is
> different, and the way the new special case is detected is also
> different, between v1 and v2.  Are both of them significant?  IOW,
> if we moved the check down but kept using is_cache_unborn(), would
> it break?  Or if we did not move the check but switched to check the
> index file on the filesystem instead of calling is_cache_unborn(),
> would it break?
> 

I had to change the check to not use is_cache_unborn() because at this 
point, the index has not been loaded so cache_nr and timestamp.sec are 
always zero (thus defeating the entire optimization).  Since part of the 
optimization was to avoid loading the index when it isn't necessary, the 
only replacement I could think of was to check for the existence of the 
index file as if it is missing entirely, it is clearly unborn.  This 
solved the behavior change for the --no-checkout sequence reported.

The only reason I moved it lower in the function was a micro perf 
optimization.  Since file_exists() does file I/O, I thought I'd do all 
the in memory/flag checks first in case they drop out early and we can 
avoid the unnecessary file I/O.  As long as it is tested before the 
'return 1;' call, it is logically correct.

> There are three existing callers of is_{cache,index}_unborn(), all
> of which want to use it to decide if we are in this funny "unborn"
> state.  If this fixes the issue we saw in v1 of these two patches,
> does that mean these three existing callers also are buggy in the
> same way and we are better off rewriting is_index_unborn() to see if
> the index file is on the disk?
> 

It is just the fact that I needed to check for an unborn index _before_ 
it was loaded that makes me unable to use is_{cache,index}_unborn() 
here.  The other callers should still be fine.  I could add a comment in 
the code to clarify this if you think it will cause confusion later.

> I am *not* suggesting to make such a drastic change to the existing
> system.  I am wondering why they are working fine but only this new
> code has to avoid the existing is_index_unborn() logic and go
> directly to the filesystem.  Especially as this new exception added
> to "skip-merge-working-tree" is to allow the special case code in
> merge-working-tree that depends on is_cache_unborn() to trigger.
> 
> Thanks for working on this.
> 

Re: [PATCH v2 0/2] Fix regression in checkout -b

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

>> This is curious.  The location the new special case is added is
>> different, and the way the new special case is detected is also
>> different, between v1 and v2.  Are both of them significant?  IOW,
>> if we moved the check down but kept using is_cache_unborn(), would
>> it break?  Or if we did not move the check but switched to check the
>> index file on the filesystem instead of calling is_cache_unborn(),
>> would it break?
>>
>
> I had to change the check to not use is_cache_unborn() because at this
> point, the index has not been loaded so cache_nr and timestamp.sec are
> always zero (thus defeating the entire optimization).  Since part of
> the optimization was to avoid loading the index when it isn't
> necessary, the only replacement I could think of was to check for the
> existence of the index file as if it is missing entirely, it is
> clearly unborn.  This solved the behavior change for the --no-checkout
> sequence reported.

Ahh, chicken-and-egg.  Please do add an in-code comment on why this
is not "is_cache_unborn()" but must be "file_exists()" immediately
before that if() statement.

> The only reason I moved it lower in the function was a micro perf
> optimization.  Since file_exists() does file I/O, I thought I'd do all
> the in memory/flag checks first in case they drop out early and we can
> avoid the unnecessary file I/O.  As long as it is tested before the
> 'return 1;' call, it is logically correct.

I see, and it does make sense.  This explanation only matters to
those who read and compare v1 and v2 and much less to those who read
and consume only v2, so it probably would have made a difference if
it were described in the cover letter, but a passing mention in the
commit log message may be enough, if we wre to leave a record of the
decision somewhere, perhaps like "As the new test involves an
filesystem access, do it later in the sequence to give chance to
other cheaper tests to leave early" or something at the end.

Thanks.

[PATCH v3 0/2] Fix regression in checkout -b

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Minor update to comment from V2.  Also wrapped commit messages to be <80
chars wide.

Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/fef76edbdc
Checkout: git fetch https://github.com/benpeart/git initial-checkout-v3 && git checkout fef76edbdc


### Interdiff (v2..v3):

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9c6e94319e..9f8f3466f6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -593,8 +593,9 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
 	 */
 
 	 /*
-	  * Do the merge if this is the initial checkout
-	  *
+	  * Do the merge if this is the initial checkout. We cannot use
+	  * is_cache_unborn() here because the index hasn't been loaded yet
+	  * so cache_nr and timestamp.sec are always zero.
 	  */
 	if (!file_exists(get_index_file()))
 		return 0;


### Patches

Ben Peart (2):
  checkout: add test demonstrating regression with checkout -b on
    initial commit
  checkout: fix regression in checkout -b on intitial checkout

 builtin/checkout.c         | 8 ++++++++
 t/t2018-checkout-branch.sh | 9 +++++++++
 2 files changed, 17 insertions(+)


base-commit: 77556354bb7ac50450e3b28999e3576969869068
-- 
2.19.1.gvfs.1.16.g9d1374d

[PATCH v3 1/2] checkout: add test demonstrating regression with checkout -b on initial commit

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

Commit fa655d8411 (checkout: optimize "git checkout -b <new_branch>",
2018-08-16) introduced an unintentional change in behavior for 'checkout -b'
after doing 'clone --no-checkout'.  Add a test to demonstrate the changed
behavior to be used in a later patch to verify the fix.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 t/t2018-checkout-branch.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 2131fb2a56..6da2d4e68f 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -198,4 +198,13 @@ test_expect_success 'checkout -B to the current branch works' '
 	test_dirty_mergeable
 '
 
+test_expect_failure 'checkout -b after clone --no-checkout does a checkout of HEAD' '
+	git init src &&
+	test_commit -C src a &&
+	rev="$(git -C src rev-parse HEAD)" &&
+	git clone --no-checkout src dest &&
+	git -C dest checkout "$rev" -b branch &&
+	test_path_is_file dest/a.t
+'
+
 test_done
-- 
2.19.1.gvfs.1.16.g9d1374d

[PATCH v3 2/2] checkout: fix regression in checkout -b on intitial checkout

[Ben Peart]

From: Ben Peart <benpeart@microsoft.com>

When doing a 'checkout -b' do a full checkout including updating the working
tree when doing the initial checkout. As the new test involves an filesystem
access, do it later in the sequence to give chance to other cheaper tests to
leave early. This fixes the regression in behavior caused by fa655d8411
(checkout: optimize "git checkout -b <new_branch>", 2018-08-16).

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 builtin/checkout.c         | 8 ++++++++
 t/t2018-checkout-branch.sh | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6fadf412e8..9f8f3466f6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -592,6 +592,14 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
 	 * Remaining variables are not checkout options but used to track state
 	 */
 
+	 /*
+	  * Do the merge if this is the initial checkout. We cannot use
+	  * is_cache_unborn() here because the index hasn't been loaded yet
+	  * so cache_nr and timestamp.sec are always zero.
+	  */
+	if (!file_exists(get_index_file()))
+		return 0;
+
 	return 1;
 }
 
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 6da2d4e68f..c5014ad9a6 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -198,7 +198,7 @@ test_expect_success 'checkout -B to the current branch works' '
 	test_dirty_mergeable
 '
 
-test_expect_failure 'checkout -b after clone --no-checkout does a checkout of HEAD' '
+test_expect_success 'checkout -b after clone --no-checkout does a checkout of HEAD' '
 	git init src &&
 	test_commit -C src a &&
 	rev="$(git -C src rev-parse HEAD)" &&
-- 
2.19.1.gvfs.1.16.g9d1374d

Re: [PATCH v3 0/2] Fix regression in checkout -b

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> From: Ben Peart <benpeart@microsoft.com>
>
> Minor update to comment from V2.  Also wrapped commit messages to be <80
> chars wide.

Perfect.  Thanks.