* [PATCH v1 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit
2019-01-18 18:55 ` [PATCH v1 0/2] Fix regression in checkout -b Ben Peart
@ 2019-01-18 18:55 ` Ben Peart
2019-01-18 19:23 ` SZEDER Gábor
2019-01-18 18:55 ` [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout Ben Peart
` (3 subsequent siblings)
4 siblings, 1 reply; 30+ messages in thread
From: Ben Peart @ 2019-01-18 18:55 UTC (permalink / raw)
To: git; +Cc: benpeart, asottile, pclouds, gitster
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v1 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit
2019-01-18 18:55 ` [PATCH v1 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit Ben Peart
@ 2019-01-18 19:23 ` SZEDER Gábor
0 siblings, 0 replies; 30+ messages in thread
From: SZEDER Gábor @ 2019-01-18 19:23 UTC (permalink / raw)
To: Ben Peart; +Cc: git, benpeart, asottile, pclouds, gitster
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
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout
2019-01-18 18:55 ` [PATCH v1 0/2] Fix regression in checkout -b Ben Peart
2019-01-18 18:55 ` [PATCH v1 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit Ben Peart
@ 2019-01-18 18:55 ` Ben Peart
2019-01-18 20:00 ` Junio C Hamano
2019-01-19 0:52 ` SZEDER Gábor
2019-01-19 1:26 ` [PATCH v1 0/2] Fix regression in checkout -b Junio C Hamano
` (2 subsequent siblings)
4 siblings, 2 replies; 30+ messages in thread
From: Ben Peart @ 2019-01-18 18:55 UTC (permalink / raw)
To: git; +Cc: benpeart, asottile, pclouds, gitster
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout
2019-01-18 18:55 ` [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout Ben Peart
@ 2019-01-18 20:00 ` Junio C Hamano
2019-01-19 0:52 ` SZEDER Gábor
1 sibling, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2019-01-18 20:00 UTC (permalink / raw)
To: Ben Peart; +Cc: git, benpeart, asottile, pclouds
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout
2019-01-18 18:55 ` [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout Ben Peart
2019-01-18 20:00 ` Junio C Hamano
@ 2019-01-19 0:52 ` SZEDER Gábor
1 sibling, 0 replies; 30+ messages in thread
From: SZEDER Gábor @ 2019-01-19 0:52 UTC (permalink / raw)
To: Ben Peart; +Cc: git, benpeart, asottile, pclouds, gitster
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v1 0/2] Fix regression in checkout -b
2019-01-18 18:55 ` [PATCH v1 0/2] Fix regression in checkout -b Ben Peart
2019-01-18 18:55 ` [PATCH v1 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit Ben Peart
2019-01-18 18:55 ` [PATCH v1 2/2] checkout: fix regression in checkout -b on intitial checkout Ben Peart
@ 2019-01-19 1:26 ` Junio C Hamano
2019-01-21 19:50 ` [PATCH v2 " Ben Peart
2019-01-23 20:01 ` [PATCH v3 " Ben Peart
4 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2019-01-19 1:26 UTC (permalink / raw)
To: Ben Peart; +Cc: git, benpeart, asottile, pclouds
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 0/2] Fix regression in checkout -b
2019-01-18 18:55 ` [PATCH v1 0/2] Fix regression in checkout -b Ben Peart
` (2 preceding siblings ...)
2019-01-19 1:26 ` [PATCH v1 0/2] Fix regression in checkout -b Junio C Hamano
@ 2019-01-21 19:50 ` Ben Peart
2019-01-21 19:50 ` [PATCH v2 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit Ben Peart
` (2 more replies)
2019-01-23 20:01 ` [PATCH v3 " Ben Peart
4 siblings, 3 replies; 30+ messages in thread
From: Ben Peart @ 2019-01-21 19:50 UTC (permalink / raw)
To: git; +Cc: asottile, benpeart, gitster, pclouds, peartben
[-- 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` doesnt 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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit
2019-01-21 19:50 ` [PATCH v2 " Ben Peart
@ 2019-01-21 19:50 ` Ben Peart
2019-01-23 17:57 ` SZEDER Gábor
2019-01-21 19:50 ` [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout Ben Peart
2019-01-22 18:54 ` [PATCH v2 0/2] Fix regression in checkout -b Junio C Hamano
2 siblings, 1 reply; 30+ messages in thread
From: Ben Peart @ 2019-01-21 19:50 UTC (permalink / raw)
To: git; +Cc: asottile, benpeart, gitster, pclouds, peartben
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit
2019-01-21 19:50 ` [PATCH v2 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit Ben Peart
@ 2019-01-23 17:57 ` SZEDER Gábor
0 siblings, 0 replies; 30+ messages in thread
From: SZEDER Gábor @ 2019-01-23 17:57 UTC (permalink / raw)
To: Ben Peart; +Cc: git, asottile, benpeart, gitster, pclouds
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout
2019-01-21 19:50 ` [PATCH v2 " Ben Peart
2019-01-21 19:50 ` [PATCH v2 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit Ben Peart
@ 2019-01-21 19:50 ` Ben Peart
2019-01-22 14:35 ` Johannes Schindelin
2019-01-22 18:54 ` [PATCH v2 0/2] Fix regression in checkout -b Junio C Hamano
2 siblings, 1 reply; 30+ messages in thread
From: Ben Peart @ 2019-01-21 19:50 UTC (permalink / raw)
To: git; +Cc: asottile, benpeart, gitster, pclouds, peartben
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout
2019-01-21 19:50 ` [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout Ben Peart
@ 2019-01-22 14:35 ` Johannes Schindelin
2019-01-22 18:42 ` Junio C Hamano
2019-01-22 18:49 ` Jeff King
0 siblings, 2 replies; 30+ messages in thread
From: Johannes Schindelin @ 2019-01-22 14:35 UTC (permalink / raw)
To: Ben Peart; +Cc: git, asottile, benpeart, gitster, pclouds
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
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout
2019-01-22 14:35 ` Johannes Schindelin
@ 2019-01-22 18:42 ` Junio C Hamano
2019-01-22 18:49 ` Jeff King
1 sibling, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2019-01-22 18:42 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Ben Peart, git, asottile, benpeart, pclouds
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout
2019-01-22 14:35 ` Johannes Schindelin
2019-01-22 18:42 ` Junio C Hamano
@ 2019-01-22 18:49 ` Jeff King
1 sibling, 0 replies; 30+ messages in thread
From: Jeff King @ 2019-01-22 18:49 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Ben Peart, git, asottile, benpeart, gitster, pclouds
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 0/2] Fix regression in checkout -b
2019-01-21 19:50 ` [PATCH v2 " Ben Peart
2019-01-21 19:50 ` [PATCH v2 1/2] checkout: add test to demonstrate regression with checkout -b on initial commit Ben Peart
2019-01-21 19:50 ` [PATCH v2 2/2] checkout: fix regression in checkout -b on intitial checkout Ben Peart
@ 2019-01-22 18:54 ` Junio C Hamano
2019-01-22 19:31 ` Ben Peart
2 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2019-01-22 18:54 UTC (permalink / raw)
To: Ben Peart; +Cc: git, asottile, benpeart, pclouds
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 0/2] Fix regression in checkout -b
2019-01-22 18:54 ` [PATCH v2 0/2] Fix regression in checkout -b Junio C Hamano
@ 2019-01-22 19:31 ` Ben Peart
2019-01-23 19:14 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Ben Peart @ 2019-01-22 19:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, asottile, benpeart, pclouds
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.
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 0/2] Fix regression in checkout -b
2019-01-22 19:31 ` Ben Peart
@ 2019-01-23 19:14 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2019-01-23 19:14 UTC (permalink / raw)
To: Ben Peart; +Cc: git, asottile, benpeart, pclouds
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3 0/2] Fix regression in checkout -b
2019-01-18 18:55 ` [PATCH v1 0/2] Fix regression in checkout -b Ben Peart
` (3 preceding siblings ...)
2019-01-21 19:50 ` [PATCH v2 " Ben Peart
@ 2019-01-23 20:01 ` Ben Peart
2019-01-23 20:02 ` [PATCH v3 1/2] checkout: add test demonstrating regression with checkout -b on initial commit Ben Peart
` (2 more replies)
4 siblings, 3 replies; 30+ messages in thread
From: Ben Peart @ 2019-01-23 20:01 UTC (permalink / raw)
To: git; +Cc: asottile, benpeart, gitster, pclouds, peartben
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 1/2] checkout: add test demonstrating regression with checkout -b on initial commit
2019-01-23 20:01 ` [PATCH v3 " Ben Peart
@ 2019-01-23 20:02 ` Ben Peart
2019-01-23 20:02 ` [PATCH v3 2/2] checkout: fix regression in checkout -b on intitial checkout Ben Peart
2019-01-23 21:23 ` [PATCH v3 0/2] Fix regression in checkout -b Junio C Hamano
2 siblings, 0 replies; 30+ messages in thread
From: Ben Peart @ 2019-01-23 20:02 UTC (permalink / raw)
To: git; +Cc: asottile, benpeart, gitster, pclouds, peartben
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 2/2] checkout: fix regression in checkout -b on intitial checkout
2019-01-23 20:01 ` [PATCH v3 " Ben Peart
2019-01-23 20:02 ` [PATCH v3 1/2] checkout: add test demonstrating regression with checkout -b on initial commit Ben Peart
@ 2019-01-23 20:02 ` Ben Peart
2019-01-23 21:23 ` [PATCH v3 0/2] Fix regression in checkout -b Junio C Hamano
2 siblings, 0 replies; 30+ messages in thread
From: Ben Peart @ 2019-01-23 20:02 UTC (permalink / raw)
To: git; +Cc: asottile, benpeart, gitster, pclouds, peartben
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3 0/2] Fix regression in checkout -b
2019-01-23 20:01 ` [PATCH v3 " Ben Peart
2019-01-23 20:02 ` [PATCH v3 1/2] checkout: add test demonstrating regression with checkout -b on initial commit Ben Peart
2019-01-23 20:02 ` [PATCH v3 2/2] checkout: fix regression in checkout -b on intitial checkout Ben Peart
@ 2019-01-23 21:23 ` Junio C Hamano
2 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2019-01-23 21:23 UTC (permalink / raw)
To: Ben Peart; +Cc: git, asottile, benpeart, pclouds
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.
^ permalink raw reply [flat|nested] 30+ messages in thread