[v3,16/17] sparse-checkout: write using lockfile
diff mbox series

Message ID 8927494b8c418e43f5bbd6e1eb108be5a0a263fc.1570478905.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • New sparse-checkout builtin and "cone" mode
Related show

Commit Message

Utsav Shah via GitGitGadget Oct. 7, 2019, 8:08 p.m. UTC
From: Derrick Stolee <dstolee@microsoft.com>

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 <dstolee@microsoft.com>
---
 builtin/sparse-checkout.c          | 15 ++++++++++++---
 t/t1091-sparse-checkout-builtin.sh |  7 +++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

Comments

Elijah Newren Oct. 12, 2019, 10:59 p.m. UTC | #1
On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
>  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
>
Derrick Stolee Oct. 14, 2019, 8:41 p.m. UTC | #2
On 10/12/2019 6:59 PM, Elijah Newren wrote:
> On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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

Patch
diff mbox series

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