Re: [PATCH 2/2] fsmonitor: Store fsmonitor bitmap before splitting index

From: Ben Peart <peartben@gmail.com>
Date: Nov 13, 2017, 15:28 UTC
Message-ID: <9b6679ea-b7e4-b45a-32bb-448cd2e891df@gmail.com>
In-Reply-To: <4ff73be656d5bbf9e2cada6bdec61843da9d1516.1510257457.git.alexmv@dropbox.com>

On 11/9/2017 2:58 PM, Alex Vandiver wrote:

Show 16 quoted lines

> ba1b9caca6 resolved the problem of the fsmonitor data being applied to
> the non-base index when reading; however, a similar problem exists
> when writing the index.  Specifically, writing of the fsmonitor
> extension happens only after the work to split the index has been
> applied -- as such, the information in the index is only for the
> non-"base" index, and thus the extension information contains only
> partial data.
> 
> When saving, compute the ewah bitmap before the index is split, and
> store it in the fsmonitor_dirty field, mirroring the behavior that
> occurred during reading.  fsmonitor_dirty is kept from being leaked by
> being freed when the extension data is written -- which always happens
> precisely once, no matter the split index configuration.
> 
> Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
> ---

The patch looks like a reasonable fix to make fsmonitor work correctly with split index. I also did manual testing to verify it was working as expected.

Thanks for adding this additional test case to ensure we don't have any regressions with the interactions between fsmonitor and split-index. While the test does correctly fail before the patch and pass after the patch, I had a question about the test-dump-fsmonitor lines.

Why do you redirect stdout to stderr and then and perform an "echo" afterwards? I don't understand what benefit that provides. I removed this logic and the test still passes so am confused as to what its purpose is.

Show 16 quoted lines

>   
> +# test that splitting the index dosn't interfere
> +test_expect_success 'splitting the index results in the same state' '
> +	write_integration_script &&
> +	dirty_repo &&
> +	git update-index --fsmonitor  &&
> +	git ls-files -f >expect &&
> +	test-dump-fsmonitor >&2 && echo &&
> +	git update-index --fsmonitor --split-index &&
> +	test-dump-fsmonitor >&2 && echo &&
> +	git ls-files -f >actual &&
> +	test_cmp expect actual
> +'
> +
>   test_done
>