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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 5666 bytes --]

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