From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: "Thomas Gummerer" <t.gummerer@gmail.com>,
git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
"Paul-Sebastian Ungureanu" <ungureanupaulsebastian@gmail.com>,
"SZEDER Gábor" <szeder.dev@gmail.com>,
"Joel Teichroeb" <joel@teichroeb.net>,
"Ben Peart" <Ben.Peart@microsoft.com>
Subject: Re: regression in new built-in stash + fsmonitor (was Re: [PATCH v13 11/27] stash: convert apply to builtin)
Date: Fri, 15 Mar 2019 00:39:30 +0100 [thread overview]
Message-ID: <878sxhauvh.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1903142058130.41@tvgsbejvaqbjf.bet>
On Thu, Mar 14 2019, Johannes Schindelin wrote:
> 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.
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/
>> 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
next prev parent reply other threads:[~2019-03-14 23:39 UTC|newest]
Thread overview: 106+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <https://public-inbox.org/git/cover.1542925164.git.ungureanupaulsebastian@gmail.com/>
2018-12-20 19:44 ` [PATCH v12 00/26] Convert "git stash" to C builtin Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 01/26] sha1-name.c: add `get_oidf()` which acts like `get_oid()` Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 02/26] strbuf.c: add `strbuf_join_argv()` Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 03/26] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()` Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 04/26] ident: add the ability to provide a "fallback identity" Paul-Sebastian Ungureanu
2018-12-26 21:21 ` Junio C Hamano
2018-12-27 21:24 ` Johannes Schindelin
2018-12-28 19:40 ` Junio C Hamano
2018-12-20 19:44 ` [PATCH v12 05/26] stash: improve option parsing test coverage Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 06/26] t3903: modernize style Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 07/26] stash: rename test cases to be more descriptive Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 08/26] stash: add tests for `git stash show` config Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 09/26] stash: mention options in `show` synopsis Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 10/26] stash: convert apply to builtin Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 11/26] stash: convert drop and clear " Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 12/26] stash: convert branch " Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 13/26] stash: convert pop " Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 14/26] stash: convert list " Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 15/26] stash: convert show " Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 16/26] stash: convert store " Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 17/26] stash: convert create " Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 18/26] stash: convert push " Paul-Sebastian Ungureanu
2019-02-08 11:30 ` SZEDER Gábor
2019-02-10 22:17 ` Thomas Gummerer
2019-02-11 1:13 ` SZEDER Gábor
2019-02-12 23:18 ` Thomas Gummerer
2019-02-19 0:23 ` SZEDER Gábor
2019-02-19 10:47 ` Johannes Schindelin
2019-02-19 19:59 ` Junio C Hamano
2019-02-20 21:01 ` Johannes Schindelin
2019-02-19 23:59 ` Thomas Gummerer
2019-02-20 4:37 ` Junio C Hamano
2019-02-20 21:10 ` Johannes Schindelin
2019-02-20 22:30 ` Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 00/27] Convert "git stash" to C builtin Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 01/27] sha1-name.c: add `get_oidf()` which acts like `get_oid()` Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 02/27] strbuf.c: add `strbuf_join_argv()` Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 03/27] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()` Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 04/27] ident: add the ability to provide a "fallback identity" Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 05/27] stash: improve option parsing test coverage Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 06/27] t3903: modernize style Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 07/27] t3903: add test for --intent-to-add file Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 08/27] stash: rename test cases to be more descriptive Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 09/27] stash: add tests for `git stash show` config Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 10/27] stash: mention options in `show` synopsis Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 11/27] stash: convert apply to builtin Thomas Gummerer
2019-03-14 13:19 ` regression in new built-in stash + fsmonitor (was Re: [PATCH v13 11/27] stash: convert apply to builtin) Ævar Arnfjörð Bjarmason
2019-03-14 15:20 ` Johannes Schindelin
2019-03-14 15:40 ` Ævar Arnfjörð Bjarmason
2019-03-14 22:45 ` Johannes Schindelin
2019-03-14 23:39 ` Ævar Arnfjörð Bjarmason [this message]
2019-03-15 2:23 ` Ben Peart
2019-02-25 23:16 ` [PATCH v13 12/27] stash: convert drop and clear to builtin Thomas Gummerer
2019-03-07 19:15 ` Jeff King
2019-03-09 18:30 ` Thomas Gummerer
2019-03-10 23:26 ` Jeff King
2019-03-11 1:40 ` Junio C Hamano
2019-03-11 21:40 ` Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 13/27] stash: convert branch " Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 14/27] stash: convert pop " Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 15/27] stash: convert list " Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 16/27] stash: convert show " Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 17/27] stash: convert store " Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 18/27] stash: convert create " Thomas Gummerer
2019-03-07 19:18 ` Jeff King
2019-03-08 15:30 ` Johannes Schindelin
2019-03-09 18:26 ` Thomas Gummerer
2019-03-11 1:47 ` Junio C Hamano
2019-03-11 7:30 ` Junio C Hamano
2019-03-11 21:42 ` Thomas Gummerer
2019-03-11 22:16 ` [PATCH v2] stash: pass pathspec as pointer Thomas Gummerer
2019-03-12 6:50 ` Junio C Hamano
2019-03-12 22:35 ` Johannes Schindelin
2019-03-12 23:40 ` Thomas Gummerer
2019-03-13 1:47 ` Junio C Hamano
2019-03-13 22:14 ` Johannes Schindelin
2019-03-15 22:33 ` Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 19/27] stash: convert push to builtin Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 20/27] stash: make push -q quiet Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 21/27] stash: convert save to builtin Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 22/27] stash: optimize `get_untracked_files()` and `check_changes()` Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 23/27] stash: replace all `write-tree` child processes with API calls Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 24/27] stash: convert `stash--helper.c` into `stash.c` Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 25/27] stash: add back the original, scripted `git stash` Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 26/27] stash: optionally use the scripted version again Thomas Gummerer
2019-02-25 23:16 ` [PATCH v13 27/27] tests: add a special setup where stash.useBuiltin is off Thomas Gummerer
2019-02-26 12:40 ` [PATCH v13 00/27] Convert "git stash" to C builtin Johannes Schindelin
2019-02-26 20:48 ` Thomas Gummerer
2019-02-26 21:45 ` Ævar Arnfjörð Bjarmason
2019-02-26 22:37 ` Johannes Schindelin
2019-03-03 1:24 ` Junio C Hamano
2019-03-03 1:25 ` Junio C Hamano
2019-03-03 10:03 ` Thomas Gummerer
2018-12-20 19:44 ` [PATCH v12 19/26] stash: make push -q quiet Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 20/26] stash: convert save to builtin Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 21/26] stash: optimize `get_untracked_files()` and `check_changes()` Paul-Sebastian Ungureanu
2019-01-06 22:47 ` Thomas Gummerer
2018-12-20 19:44 ` [PATCH v12 22/26] stash: replace all `write-tree` child processes with API calls Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 23/26] stash: convert `stash--helper.c` into `stash.c` Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 24/26] stash: add back the original, scripted `git stash` Paul-Sebastian Ungureanu
2018-12-20 19:44 ` [PATCH v12 25/26] stash: optionally use the scripted version again Paul-Sebastian Ungureanu
2019-01-06 22:59 ` Thomas Gummerer
2018-12-20 19:44 ` [PATCH v12 26/26] tests: add a special setup where stash.useBuiltin is off Paul-Sebastian Ungureanu
2019-01-03 23:39 ` [PATCH v12 00/26] Convert "git stash" to C builtin Junio C Hamano
2019-01-18 12:06 ` Johannes Schindelin
2019-01-18 17:49 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=878sxhauvh.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=Ben.Peart@microsoft.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=joel@teichroeb.net \
--cc=szeder.dev@gmail.com \
--cc=t.gummerer@gmail.com \
--cc=ungureanupaulsebastian@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).