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 > >> > > >> > 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