On Thu, Mar 14 2019, Johannes Schindelin wrote:

Hi Ævar,

On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

quoted

On Thu, Mar 14 2019, Johannes Schindelin wrote:

quoted

On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

quoted

On Tue, Feb 26 2019, Thomas Gummerer wrote:

quoted

From: Joel Teichroeb <redacted>

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.

It fixes not just this issue, but now the whole test suite passes with
GIT_TEST_FSMONITOR, i.e. this test that's been failing for ~2 years also
works now:
https://public-inbox.org/git/87k1vwn9qe.fsf@evledraar.gmail.com/

quoted

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