Re: [PATCH v2 0/1] fsmonitor: don't fill bitmap with entries to be removed
Hi Junio,
On Thu, 10 Oct 2019, Junio C Hamano wrote:
> "William Baker via GitGitGadget" writes:
>
> > This is the second iteration of changes to fix the segfault that I
> > encountered while testing fsmonitor. This iteration includes the following
> > updates for feedback I received on v1:
> >
> > * Use %u instead of %"PRIuMAX" for unsigned ints in BUG format strings
> > * Updated the new test's comment to be more descriptive
> >
> > Thanks,
> >
> > William
> >
> > William Baker (1):
> > fsmonitor: don't fill bitmap with entries to be removed
>
> Thanks. Dscho, is this still Reviewed-by: you?
Yes!
Thanks,
Dscho
P.S.: I guess we could even go and make this a `Co-Authored-by:` because
the test case is actually by me, at least originally... ;-)
>
> >
> > fsmonitor.c | 29 -
> > t/t7519-status-fsmonitor.sh | 13 +
> > t/t7519/fsmonitor-env | 24
> > 3 files changed, 61 insertions(+), 5 deletions(-)
> > create mode 100755 t/t7519/fsmonitor-env
> >
> >
> > base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
> > Published-As:
> > https://github.com/gitgitgadget/git/releases/tag/pr-372%2Fwilbaker%2Ffix_git_fsmonitor_crash-v2
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
> > pr-372/wilbaker/fix_git_fsmonitor_crash-v2
> > Pull-Request: https://github.com/gitgitgadget/git/pull/372
> >
> > Range-diff vs v1:
> >
> > 1: ce9bf4237e ! 1: 08741d986c fsmonitor: don't fill bitmap with entries
> > to be removed
> > @@ -44,8 +44,8 @@
> > + struct cache_entry *ce;
> > +
> > + if (pos >= istate->cache_nr)
> > -+ BUG("fsmonitor_dirty has more entries than the index
> > (%"PRIuMAX" >= %"PRIuMAX")",
> > -+ (uintmax_t)pos, (uintmax_t)istate->cache_nr);
> > ++ BUG("fsmonitor_dirty has more entries than the index
> > (%"PRIuMAX" >= %u)",
> > ++ (uintmax_t)pos, istate->cache_nr);
> >
> > + ce = istate->cache[pos];
> > ce->ce_flags &= ~CE_FSMONITOR_VALID;
> > @@ -56,8 +56,8 @@
> > istate->fsmonitor_dirty = fsmonitor_dirty;
> >
> > + if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > -+ BUG("fsmonitor_dirty has more entries than the index
> > (%"PRIuMAX" > %"PRIuMAX")",
> > -+ (uintmax_t)istate->fsmonitor_dirty->bit_size,
> > (uintmax_t)istate->cache_nr);
> > ++ BUG("fsmonitor_dirty has more entries than the index
> > (%"PRIuMAX" > %u)",
> > ++ (uintmax_t)istate->fsmonitor_dirty->bit_size,
> > istate->cache_nr);
> > +
> > trace_printf_key(&trace_fsmonitor, "read fsmonitor extension
> > successful");
> > return 0;
> > @@ -85,8 +85,8 @@
> > int fixup = 0;
> >
> > + if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> > -+ BUG("fsmonitor_dirty has more entries than the index
> > (%"PRIuMAX" > %"PRIuMAX")",
> > -+ (uintmax_t)istate->fsmonitor_dirty->bit_size,
> > (uintmax_t)istate->cache_nr);
> > ++ BUG("fsmonitor_dirty has more entries than the index
> > (%"PRIuMAX" > %u)",
> > ++ (uintmax_t)istate->fsmonitor_dirty->bit_size,
> > istate->cache_nr);
> > +
> > put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
> > strbuf_add(sb, &hdr_version, sizeof(uint32_t));
> > @@ -96,8 +96,8 @@
> >
> > /* Mark all previously saved entries as dirty */
> > + if (istate->fsmonitor_dirty->bit_size >
> > istate->cache_nr)
> > -+ BUG("fsmonitor_dirty has more entries
> > than the index (%"PRIuMAX" > %"PRIuMAX")",
> > -+
> > (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
> > ++ BUG("fsmonitor_dirty has more entries
> > than the index (%"PRIuMAX" > %u)",
> > ++
> > (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> > ewah_each_bit(istate->fsmonitor_dirty,
> > fsmonitor_ewah_callback, istate);
> >
> > /* Now mark the untracked cache for fsmonitor
> > usage */
> > @@ -109,8 +109,9 @@
> > test_cmp expect actual
> >'
> >
> > -+# Use test files that start with 'z' so that the entries being added
> > -+# and removed appear at the end of the index.
> > ++# This test covers staging/unstaging files that appear at the end of
> > the index.
> > ++# Test files with names beginning with 'z' are used under the
> > assumption that
> > ++# earlier tests do not add/leave index entries that sort below them.
> > +test_expect_success 'status succeeds after staging/unstaging ' '
> >
Re: [PATCH v2 0/1] fsmonitor: don't fill bitmap with entries to be removed
"William Baker via GitGitGadget" writes:
> This is the second iteration of changes to fix the segfault that I
> encountered while testing fsmonitor. This iteration includes the following
> updates for feedback I received on v1:
>
> * Use %u instead of %"PRIuMAX" for unsigned ints in BUG format strings
> * Updated the new test's comment to be more descriptive
>
> Thanks,
>
> William
>
> William Baker (1):
> fsmonitor: don't fill bitmap with entries to be removed
Thanks. Dscho, is this still Reviewed-by: you?
>
> fsmonitor.c | 29 -
> t/t7519-status-fsmonitor.sh | 13 +
> t/t7519/fsmonitor-env | 24
> 3 files changed, 61 insertions(+), 5 deletions(-)
> create mode 100755 t/t7519/fsmonitor-env
>
>
> base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
> Published-As:
> https://github.com/gitgitgadget/git/releases/tag/pr-372%2Fwilbaker%2Ffix_git_fsmonitor_crash-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
> pr-372/wilbaker/fix_git_fsmonitor_crash-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/372
>
> Range-diff vs v1:
>
> 1: ce9bf4237e ! 1: 08741d986c fsmonitor: don't fill bitmap with entries to
> be removed
> @@ -44,8 +44,8 @@
> + struct cache_entry *ce;
> +
> + if (pos >= istate->cache_nr)
> -+ BUG("fsmonitor_dirty has more entries than the index
> (%"PRIuMAX" >= %"PRIuMAX")",
> -+ (uintmax_t)pos, (uintmax_t)istate->cache_nr);
> ++ BUG("fsmonitor_dirty has more entries than the index
> (%"PRIuMAX" >= %u)",
> ++ (uintmax_t)pos, istate->cache_nr);
>
> + ce = istate->cache[pos];
> ce->ce_flags &= ~CE_FSMONITOR_VALID;
> @@ -56,8 +56,8 @@
> istate->fsmonitor_dirty = fsmonitor_dirty;
>
> + if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> -+ BUG("fsmonitor_dirty has more entries than the index
> (%"PRIuMAX" > %"PRIuMAX")",
> -+ (uintmax_t)istate->fsmonitor_dirty->bit_size,
> (uintmax_t)istate->cache_nr);
> ++ BUG("fsmonitor_dirty has more entries than the index
> (%"PRIuMAX" > %u)",
> ++ (uintmax_t)istate->fsmonitor_dirty->bit_size,
> istate->cache_nr);
> +
> trace_printf_key(&trace_fsmonitor, "read fsmonitor extension
> successful");
> return 0;
> @@ -85,8 +85,8 @@
> int fixup = 0;
>
> + if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> -+ BUG("fsmonitor_dirty has more entries than the index
> (%"PRIuMAX" > %"PRIuMAX")",
> -+ (uintmax_t)istate->fsmonitor_dirty->bit_size,
> (uintmax_t)istate->cache_nr);
> ++ BUG("fsmonitor_dirty has more entries than the index
> (%"PRIuMAX" > %u)",
> ++ (uintmax_t)istate->fsmonitor_dirty->bit_size,
> istate->cache_nr);
> +
> put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
> strbuf_add(sb, &hdr_version, sizeof(uint32_t));
> @@ -96,8 +96,8 @@
>
> /* Mark all previously saved entries as dirty */
> + if (istate->fsmonitor_dirty->bit_size >
> istate->cache_nr)
> -+ BUG("fsmonitor_dirty has more entries
> than the index (%"PRIuMAX" > %"PRIuMAX")",
> -+
> (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
> ++ BUG("fsmonitor_dirty has more entries
> than the index (%"PRIuMAX" > %u)",
> ++
> (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> ewah_each_bit(istate->fsmonitor_dirty,
> fsmonitor_ewah_callback, istate);
>
> /* Now mark the untracked cache for fsmonitor
> usage */
> @@ -109,8 +109,9 @@
> test_cmp expect actual
>'
>
> -+# Use test files that start with 'z' so that the entries being added
> -+# and removed appear at the end of the index.
> ++# This test covers staging/unstaging files that appear at the end of
> the index.
> ++# Test files with names beginning with 'z' are used under the
> assumption that
> ++# earlier tests do not add/leave index entries that sort below them.
> +test_expect_success 'status succeeds after staging/unstaging ' '
> + test_commit initial &&
> + removed=$(test_seq 1 100 | sed "s/^/z/") &&
[PATCH v2 0/1] fsmonitor: don't fill bitmap with entries to be removed
This is the second iteration of changes to fix the segfault that I
encountered while testing fsmonitor. This iteration includes the following
updates for feedback I received on v1:
* Use %u instead of %"PRIuMAX" for unsigned ints in BUG format strings
* Updated the new test's comment to be more descriptive
Thanks,
William
William Baker (1):
fsmonitor: don't fill bitmap with entries to be removed
fsmonitor.c | 29 -
t/t7519-status-fsmonitor.sh | 13 +
t/t7519/fsmonitor-env | 24
3 files changed, 61 insertions(+), 5 deletions(-)
create mode 100755 t/t7519/fsmonitor-env
base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As:
https://github.com/gitgitgadget/git/releases/tag/pr-372%2Fwilbaker%2Ffix_git_fsmonitor_crash-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-372/wilbaker/fix_git_fsmonitor_crash-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/372
Range-diff vs v1:
1: ce9bf4237e ! 1: 08741d986c fsmonitor: don't fill bitmap with entries to
be removed
@@ -44,8 +44,8 @@
+ struct cache_entry *ce;
+
+ if (pos >= istate->cache_nr)
-+ BUG("fsmonitor_dirty has more entries than the index
(%"PRIuMAX" >= %"PRIuMAX")",
-+ (uintmax_t)pos, (uintmax_t)istate->cache_nr);
++ BUG("fsmonitor_dirty has more entries than the index
(%"PRIuMAX" >= %u)",
++ (uintmax_t)pos, istate->cache_nr);
+ ce = istate->cache[pos];
ce->ce_flags &= ~CE_FSMONITOR_VALID;
@@ -56,8 +56,8 @@
istate->fsmonitor_dirty = fsmonitor_dirty;
+ if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-+ BUG("fsmonitor_dirty has more entries than the index
(%"PRIuMAX" > %"PRIuMAX")",
-+ (uintmax_t)istate->fsmonitor_dirty->bit_size,
(uintmax_t)istate->cache_nr);
++ BUG("fsmonitor_dirty has more entries than the index
(%"PRIuMAX" > %u)",
++ (uintmax_t)istate->fsmonitor_dirty->bit_size,
istate->cache_nr);
+
trace_printf_key(&trace_fsmonitor, "read fsmonitor extension
successful");
return 0;
@@ -85,8 +85,8 @@
int fixup = 0;
+ if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-+ BUG("fsmonitor_dirty has more entries than the index
(%"PRIuMAX" > %"PRIuMAX")",
-+ (uintmax_t)istate->fsmonitor_dirty->bit_size,
(uintmax_t)istate->cache_nr);
++ BUG("fsmonitor_dirty has more entries than the index
(%"PRIuMAX" > %u)",
++ (uintmax_t)istate->fsmonitor_dirty->bit_size,
istate->cache_nr);
+
put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
strbuf_add(sb, &hdr_version, sizeof(uint32_t));
@@ -96,8 +96,8 @@
/* Mark all previously saved entries as dirty */
+ if (istate->fsmonitor_dirty->bit_size >
istate->cache_nr)
-+ BUG("fsmonitor_dirty has more entries than the
index (%"PRIuMAX" > %"PRIuMAX")",
-+
(uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);
++ BUG("fsmonitor_dirty has more entries than the
index (%"PRIuMAX" > %u)",
++
(uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
ewah_each_bit(istate->fsmonitor_dirty,
fsmonitor_ewah_callback, istate);
/* Now mark the untracked cache for fsmonitor usage */
@@ -109,8 +109,9 @@
test_cmp expect actual
'
-+# Use test files that start with 'z' so that the entries being added
-+# and removed appear at the end of the index.
++# This test covers staging/unstaging files that appear at the end of the
index.
++# Test files with names beginning with 'z' are used under the assumption
that
++# earlier tests do not add/leave index entries that sort below them.
+test_expect_success 'status succeeds after staging/unstaging ' '
+ test_commit initial &&
+ removed=$(test_seq 1 100 | sed "s/^/z/") &&
--
gitgitgadget
