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

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

On Wed, Jan 2, 2019 at 6:36 AM Anthony Sottile <asottile@umich.edu> wrote:

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).

On Wed, Jan 2, 2019 at 3:08 AM Duy Nguyen <pclouds@gmail.com> 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>"

    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

On Wed, Jan 2, 2019 at 11:18 PM Anthony Sottile <asottile@umich.edu> wrote:

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.

Duy Nguyen <pclouds@gmail.com> writes:

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.

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".

On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano <gitster@pobox.com> wrote:

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

Anthony Sottile <asottile@umich.edu> writes:

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 ;-).

On Thu, Jan 3, 2019 at 1:51 PM Junio C Hamano <gitster@pobox.com> wrote:

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

On 1/3/2019 5:05 PM, Anthony Sottile wrote:

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.

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

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(+)

On Fri, Jan 18, 2019 at 01:55:57PM -0500, Ben Peart wrote:

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.

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).

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

The above three lines could be replaced by a single

  test_commit -C src a

command.

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.

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(-)

Ben Peart <peartben@gmail.com> writes:

Yup, that's a trivial fix ;-)

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.

On Fri, Jan 18, 2019 at 01:55:58PM -0500, Ben Peart wrote:

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

Ben Peart <peartben@gmail.com> writes:

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

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):

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(+)

On Mon, Jan 21, 2019 at 02:50:07PM -0500, Ben Peart wrote:

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.

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(-)

Hi,

On Mon, 21 Jan 2019, Ben Peart wrote:

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

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

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.

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

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...

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

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?

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.

On 1/22/2019 1:54 PM, Junio C Hamano wrote:

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.

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.

Ben Peart <peartben@gmail.com> writes:

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.

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.

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):

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(+)

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(-)

Ben Peart <peartben@gmail.com> writes:

Perfect.  Thanks.