Re: regression in new built-in stash + fsmonitor (was Re: [PATCH v13 11/27] stash: convert apply to builtin)

> Hi Ævar,
>
> On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
>
>> On Thu, Mar 14 2019, Johannes Schindelin wrote:
>>
>> > On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> On Tue, Feb 26 2019, Thomas Gummerer wrote:
>> >>
>> >> > From: Joel Teichroeb <joel@teichroeb.net>
>> >> >
>> >> > Add a builtin helper for performing stash commands. Converting
>> >> > all at once proved hard to review, so starting with just apply
>> >> > lets conversion get started without the other commands being
>> >> > finished.
>> >> >
>> >> > The helper is being implemented as a drop in replacement for
>> >> > stash so that when it is complete it can simply be renamed and
>> >> > the shell script deleted.
>> >> >
>> >> > Delete the contents of the apply_stash shell function and replace
>> >> > it with a call to stash--helper apply until pop is also
>> >> > converted.
>> >>
>> >> This
>> >>
>> >>     GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all ./t3420-rebase-autostash.sh
>> >>
>> >> Now fails, which bisects to 8a0fc8d19d ("stash: convert apply to
>> >> builtin", 2019-02-25).
>> >>
>> >> Tested on both a CentOS 6 & modern Debian testing machine:
>> >>
>> >>     + git rebase -i --autostash HEAD^
>> >>     Created autostash: 5cd734b
>> >>     HEAD is now at 0c4d2f1 third commit
>> >>     hint: Waiting for your editor to close the file...
>> >>     error: There was a problem with the editor '"$FAKE_EDITOR"'.
>> >>     Applied autostash.
>> >>     + exit_code=1
>> >>     + test 1 -eq 0
>> >>     + test_match_signal 13 1
>> >>     + test 1 = 141
>> >>     + test 1 = 269
>> >>     + return 1
>> >>     + test 1 -gt 129
>> >>     + test 1 -eq 127
>> >>     + test 1 -eq 126
>> >>     + return 0
>> >>     + rm -f abort-editor.sh
>> >>     + echo conflicting-content
>> >>     + test_cmp expected file0
>> >>     + diff -u expected file0
>> >>     --- expected    2019-03-14 13:19:08.212215263 +0000
>> >>     +++ file0       2019-03-14 13:19:08.196215250 +0000
>> >>     @@ -1 +1 @@
>> >>     -conflicting-content
>> >>     +uncommitted-content
>> >>     error: last command exited with $?=1
>> >>     not ok 36 - autostash is saved on editor failure with conflict
>> >>
>> >> Are you able to reproduce this? And if so I suggest running the test
>> >> suite with some of the other GIT_TEST_* modes documented in
>> >> t/README. Maybe it'll turn up something else...
>> >
>> > Yep, totally can reproduce it :-(
>
> Well, isn't this exciting: we found not a bug in the built-in stash (even
> if Junio probably expected yet another one), but an fsmonitor one! Even
> better, I think this might be the bug that Alex Vandiver was chasing and
> that he talked about at the Contributors' Summit last year in Barcelona.
>
> The symptom is that cache entries are sometimes considered up to date,
> when they really are not.
>
> And the reason is that the fsmonitor has this honking global flag
> `has_run_once` (it is not really global, it is `static` to
> `refresh_fsmonitor()`, but that's the same for all practical purposes, as
> it is *not* specific to one `struct index_state`), which was kind of okay
> as long as `the_index` was used implicitly by everything.
>
> Except it was not okay when `discard_index()` (or `discard_cache()`) was
> called: in that case, the flag was not re-set. And re-set it needs to be,
> in that case, otherwise the fsmonitor is not asked which entries need to
> be updated.
>
> I saw this pretty early on in my investigation and marked it up for a
> follow-up task, wasting hours of investigation by not believing that this
> could be the culprit of the bug you described. I did not believe it
> because `git stash apply` is *spawned*, so there is not even an index that
> needs to be discarded (I thought; more on that one later).
>
> It is quite curious that this is the only occasion that our test suite
> covers that particular part of the fsmonitor...
>
> I do not really want to rely on implementation details of the rebase to verify
> that the fsmonitor is queried again (and, crucially, resets the
> FSMONITOR_VALID flag of the file(s) indicated as out of date) after the index
> is discarded and re-read.
>
> I guess the best bet is to extend `t/helper/test-read-cache.c` to optionally
> output information about a specific cache entry, then refresh it, and then run
> that test helper with fsmonitor-all (which should mark anything as modified,
> all the time). That should verify that the fix works.
>
> I did exactly that, and pushed the result to https://github.com/dscho/git
> as `fix-fsmonitor` branch.
>
> Could I ask you to test that one (it is based off of Git for Windows'
> `master`, but that should compile cleanly for you)?
>
> For now, I am a bit spent, so I'll leave the rest for tomorrow.

>> In the meantime I did a build with "next" (so stash-in-C) using the
>> standard test mode and these:
>>
>>     (cd t && GIT_TEST_GETTEXT_POISON=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all GIT_SKIP_TESTS="t3404.8 t3420.36" /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_SPLIT_INDEX=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_FULL_IN_PACK_ARRAY=true GIT_TEST_OE_SIZE=10 /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_COMMIT_GRAPH=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_MULTI_PACK_INDEX=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_STASH_USE_BUILTIN=false /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_CHECK_COLLISIONS=false /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>
>> Only this specific test failed.
>
> Well, good!
>
> Thank you for getting the ball rolling!
> Dscho