git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
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

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