| Message ID | 20190121195008.8700-3-peartben@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] checkout: add test to demonstrate regression with checkout -b on initial commit | expand |
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 > >
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.
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
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)" &&