Re: [PATCH v3 16/17] sparse-checkout: write using lockfile
On 10/12/2019 6:59 PM, Elijah Newren wrote: > On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget > wrote: >> >> From: Derrick Stolee >> >> If two 'git sparse-checkout set' subcommands are launched at the >> same time, the behavior can be unexpected as they compete to write >> the sparse-checkout file and update the working directory. >> >> Take a lockfile around the writes to the sparse-checkout file. In >> addition, acquire this lock around the working directory update >> to avoid two commands updating the working directory in different >> ways. > > Wow, there's something I never would have thought to check. Did you > have folks run into this, or is this just some defensive programming? > Either way, I'm impressed. This is defensive programming thanks to Kevin Willford's careful review [1]. -Stolee [1] https://github.com/microsoft/git/pull/204#discussion_r330252848
Re: [PATCH v3 16/17] sparse-checkout: write using lockfile
On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget
wrote:
>
> From: Derrick Stolee
>
> If two 'git sparse-checkout set' subcommands are launched at the
> same time, the behavior can be unexpected as they compete to write
> the sparse-checkout file and update the working directory.
>
> Take a lockfile around the writes to the sparse-checkout file. In
> addition, acquire this lock around the working directory update
> to avoid two commands updating the working directory in different
> ways.
Wow, there's something I never would have thought to check. Did you
have folks run into this, or is this just some defensive programming?
Either way, I'm impressed.
>
> Signed-off-by: Derrick Stolee
> ---
> builtin/sparse-checkout.c | 15 ---
> t/t1091-sparse-checkout-builtin.sh | 7 +++
> 2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 542d57fac6..9b313093cd 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -308,6 +308,8 @@ static int write_patterns_and_update(struct pattern_list
> *pl)
> {
> char *sparse_filename;
> FILE *fp;
> + int fd;
> + struct lock_file lk = LOCK_INIT;
> int result;
>
> if (!core_apply_sparse_checkout) {
> @@ -317,21 +319,28 @@ static int write_patterns_and_update(struct
> pattern_list *pl)
>
> result = update_working_directory(pl);
>
> + sparse_filename = get_sparse_checkout_filename();
> + fd = hold_lock_file_for_update(&lk, sparse_filename,
> + LOCK_DIE_ON_ERROR);
> +
> + result = update_working_directory(pl);
> if (result) {
> + rollback_lock_file(&lk);
> + free(sparse_filename);
> clear_pattern_list(pl);
> update_working_directory(NULL);
> return result;
> }
>
> - sparse_filename = get_sparse_checkout_filename();
> - fp = fopen(sparse_filename, "w");
> + fp = fdopen(fd, "w");
>
> if (core_sparse_checkout_cone)
> write_cone_to_file(fp, pl);
> else
> write_patterns_to_file(fp, pl);
>
> - fclose(fp);
> + fflush(fp);
> + commit_lock_file(&lk);
>
> free(sparse_filename);
> clear_pattern_list(pl);
> diff --git a/t/t1091-sparse-checkout-builtin.sh
> b/t/t1091-sparse-checkout-builtin.sh
> index 82eb5fb2f8..f22a4afbea 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -262,4 +262,11 @@ test_expect_success 'revert to old sparse-checkout on
> bad update' '
> test_cmp dir expect
> '
>
> +test_expect_success 'fail when lock is taken' '
> + test_when_finished rm -rf repo/.git/info/sparse-checkout.lock &&
> + touch repo/.git/info/sparse-checkout.lock &&
> + test_must_fail git -C repo sparse-checkout set deep 2>err &&
> + test_i18ngrep "File exists" err
> +'
> +
> test_done
> --
> gitgitgadget
>
[PATCH v3 16/17] sparse-checkout: write using lockfile
From: Derrick Stolee
If two 'git sparse-checkout set' subcommands are launched at the
same time, the behavior can be unexpected as they compete to write
the sparse-checkout file and update the working directory.
Take a lockfile around the writes to the sparse-checkout file. In
addition, acquire this lock around the working directory update
to avoid two commands updating the working directory in different
ways.
Signed-off-by: Derrick Stolee
---
builtin/sparse-checkout.c | 15 ---
t/t1091-sparse-checkout-builtin.sh | 7 +++
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 542d57fac6..9b313093cd 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -308,6 +308,8 @@ static int write_patterns_and_update(struct pattern_list
*pl)
{
char *sparse_filename;
FILE *fp;
+ int fd;
+ struct lock_file lk = LOCK_INIT;
int result;
if (!core_apply_sparse_checkout) {
@@ -317,21 +319,28 @@ static int write_patterns_and_update(struct pattern_list
*pl)
result = update_working_directory(pl);
+ sparse_filename = get_sparse_checkout_filename();
+ fd = hold_lock_file_for_update(&lk, sparse_filename,
+ LOCK_DIE_ON_ERROR);
+
+ result = update_working_directory(pl);
if (result) {
+ rollback_lock_file(&lk);
+ free(sparse_filename);
clear_pattern_list(pl);
update_working_directory(NULL);
return result;
}
- sparse_filename = get_sparse_checkout_filename();
- fp = fopen(sparse_filename, "w");
+ fp = fdopen(fd, "w");
if (core_sparse_checkout_cone)
write_cone_to_file(fp, pl);
else
write_patterns_to_file(fp, pl);
- fclose(fp);
+ fflush(fp);
+ commit_lock_file(&lk);
free(sparse_filename);
clear_pattern_list(pl);
diff --git a/t/t1091-sparse-checkout-builtin.sh
b/t/t1091-sparse-checkout-builtin.sh
index 82eb5fb2f8..f22a4afbea 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -262,4 +262,11 @@ test_expect_success 'revert to old sparse-checkout on bad
update' '
test_cmp dir expect
'
+test_expect_success 'fail when lock is taken' '
+ test_when_finished rm -rf repo/.git/info/sparse-checkout.lock &&
+ touch repo/.git/info/sparse-checkout.lock &&
+ test_must_fail git -C repo sparse-checkout set deep 2>err &&
+ test_i18ngrep "File exists" err
+'
+
test_done
--
gitgitgadget
