Re: [PATCH v2 1/1] fsmonitor: don't fill bitmap with entries to be removed

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Oct 10, 2019, 11:22 UTC
Message-ID: <20191010112204.GK29845@szeder.dev>
In-Reply-To: <20191010110732.GJ29845@szeder.dev>

On Thu, Oct 10, 2019 at 01:07:32PM +0200, SZEDER Gábor wrote:

Show 12 quoted lines

> On Wed, Oct 09, 2019 at 02:00:12PM -0700, William Baker via GitGitGadget wrote:
> > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> > index 81a375fa0f..87042470ab 100755
> > --- a/t/t7519-status-fsmonitor.sh
> > +++ b/t/t7519-status-fsmonitor.sh
> > @@ -354,4 +354,17 @@ test_expect_success 'discard_index() also discards fsmonitor info' '
> >  	test_cmp expect actual
> >  '
> >  
> > +# 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.

I just read through Junio's comments on the first version of this patch, in particular his remarks about this comment.

If this new test case below were run in a dedicated repository, then this comment wouldn't be necessary, and all my comments below about that not-really-initial commit would be moot, too.

Show 14 quoted lines

> > +test_expect_success 'status succeeds after staging/unstaging ' '
> > +	test_commit initial &&
> 
> This is confusing: this is the 29th test case in this script and it
> creates an "initial" commit?!
> 
> The first "setup" test case has already created an initial commit, so
> this should rather be called "second".
> 
> OTOH, none of the later commands in this test case seem to have
> anything to do with this second commit, and indeed the test case works
> even without it (i.e. 'git status' still segfaults without the fix and
> then succeeds with the fix applied), so instead of updating its
> message perhaps it could simply be removed.

Show 10 quoted lines

> 
> > +	removed=$(test_seq 1 100 | sed "s/^/z/") &&
> > +	touch $removed &&
> > +	git add $removed &&
> > +	test_config core.fsmonitor "$TEST_DIRECTORY/t7519/fsmonitor-env" &&
> > +	FSMONITOR_LIST="$removed" git restore -S $removed &&
> > +	FSMONITOR_LIST="$removed" git status
> > +'
> > +
> >  test_done