| Message ID | 8927494b8c418e43f5bbd6e1eb108be5a0a263fc.1570478905.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New sparse-checkout builtin and "cone" mode | expand |
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 >
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
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