From bc40a560d3c95040b55fd7be6fe5b7012d267f8f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 9 Jun 2021 09:49:50 +0200 Subject: [PATCH 1/6] fixup! fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC In FSMonitor v1, we made sure to only use a valid `since_token` when querying the FSMonitor. This condition was accidentally lost in v2, and caused segmentation faults uncovered by Scalar's Functional Tests. I had tried to fix this in https://github.com/git-for-windows/pull/3241, but the fix was incomplete, and I had to follow up with https://github.com/git-for-windows/pull/3258. However, it turns out that both of them were actually only work-arounds; I should have dug deeper to figure out _why_ the `since_token` was no longer guaranteed not to be `NULL`, and I finally did. Signed-off-by: Johannes Schindelin --- fsmonitor.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fsmonitor.c b/fsmonitor.c index 22623fd228fc61..0b40643442eb06 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -290,8 +290,9 @@ void refresh_fsmonitor(struct index_state *istate) trace_printf_key(&trace_fsmonitor, "refresh fsmonitor"); if (r->settings.use_builtin_fsmonitor > 0) { - query_success = !fsmonitor_ipc__send_query( - istate->fsmonitor_last_update, &query_result); + query_success = istate->fsmonitor_last_update && + !fsmonitor_ipc__send_query(istate->fsmonitor_last_update, + &query_result); if (query_success) { /* * The response contains a series of nul terminated From 7eb8372f9fdb6f331f1845f34f25df7aa36005e5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 9 Jun 2021 09:54:50 +0200 Subject: [PATCH 2/6] fixup! fsmonitor-ipc: create client routines for git-fsmonitor--daemon Now that we have a correct fix where we guarantee again (just like v1 of the built-in FSMonitor) that `since_token` is not `NULL`, we can revert the work-arounds introduced by these two PRs: - https://github.com/git-for-windows/pull/3241 - https://github.com/git-for-windows/pull/3258 Signed-off-by: Johannes Schindelin --- fsmonitor-ipc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c index 47afb4709f64e6..e62901a85b5dfe 100644 --- a/fsmonitor-ipc.c +++ b/fsmonitor-ipc.c @@ -44,7 +44,7 @@ int fsmonitor_ipc__send_query(const char *since_token, trace2_region_enter("fsm_client", "query", NULL); trace2_data_string("fsm_client", NULL, "query/command", - since_token ? since_token : "(null-token)"); + since_token); try_again: state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options, @@ -53,7 +53,7 @@ int fsmonitor_ipc__send_query(const char *since_token, switch (state) { case IPC_STATE__LISTENING: ret = ipc_client_send_command_to_connection( - connection, since_token, since_token ? strlen(since_token) : 0, answer); + connection, since_token, strlen(since_token), answer); ipc_client_close_connection(connection); trace2_data_intmax("fsm_client", NULL, From 5ec07b8c4a8ce7f4a9f3a8a74d87545955947e02 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 9 Jun 2021 09:59:58 +0200 Subject: [PATCH 3/6] fixup! fsmonitor--daemon: use a cookie file to sync with file system When flushing the FSMonitor data, we do not actually need to wait for a cookie file. In the worst case, we will over-report a bit. If we _do_ wait for a cookie file, in the worst case we cause a hang because the FSMonitor daemon will wait and wait even though the `.git/` directory might be gone already. Signed-off-by: Johannes Schindelin --- builtin/fsmonitor--daemon.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c index 985a82cf39e076..0f76cdb1a2ca70 100644 --- a/builtin/fsmonitor--daemon.c +++ b/builtin/fsmonitor--daemon.c @@ -672,7 +672,6 @@ static int do_handle_client(struct fsmonitor_daemon_state *state, */ do_flush = 1; do_trivial = 1; - do_cookie = 1; } else if (!skip_prefix(command, "builtin:", &p)) { /* assume V1 timestamp or garbage */ From 92392daccb31c405c529a772fc8b9ca63631e630 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 9 Jun 2021 09:59:58 +0200 Subject: [PATCH 4/6] fixup! fsmonitor--daemon: use a cookie file to sync with file system When the built-in FSMonitor receives an unexpected token, we do not actually need to wait for a cookie file. There simply is no use for waiting in this instance. It's not like the client will all of a sudden realize that it sent an incorrect token and somehow make up a correct token from thin air in a follow-up query. Signed-off-by: Johannes Schindelin --- builtin/fsmonitor--daemon.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c index 0f76cdb1a2ca70..9e0a12024a7c8c 100644 --- a/builtin/fsmonitor--daemon.c +++ b/builtin/fsmonitor--daemon.c @@ -685,7 +685,6 @@ static int do_handle_client(struct fsmonitor_daemon_state *state, "fsmonitor: unsupported V1 protocol '%s'"), command); do_trivial = 1; - do_cookie = 1; } else { /* We have "builtin:*" */ From cfa0c45308ef349b6f0a4ffa9e2ccd8d026a9f4b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 9 Jun 2021 09:59:58 +0200 Subject: [PATCH 5/6] fixup! fsmonitor--daemon: use a cookie file to sync with file system When the built-in FSMonitor receives an invalid v2 token, we do not actually need to wait for a cookie file. There simply is no use for waiting in this instance. It's not like the client will all of a sudden realize that it sent an incorrect token and somehow make up a correct token from thin air in a follow-up query. Signed-off-by: Johannes Schindelin --- builtin/fsmonitor--daemon.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c index 9e0a12024a7c8c..4afbb36fe61cae 100644 --- a/builtin/fsmonitor--daemon.c +++ b/builtin/fsmonitor--daemon.c @@ -694,7 +694,6 @@ static int do_handle_client(struct fsmonitor_daemon_state *state, "fsmonitor: invalid V2 protocol token '%s'", command); do_trivial = 1; - do_cookie = 1; } else { /* From 059c4b37955e38684a3d1236f7c831c6955e5d1a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 8 Jun 2021 09:43:36 +0200 Subject: [PATCH 6/6] ci: run Scalar's Functional Tests Scalar's Functional Test suite is pretty comprehensive, and caught more than just one bug in the built-in FSMonitor that was missed by Git's own test suite. To benefit from this test suite, automatically run it on the `vfs-*` branches. Note: for simplicity, we're building Git from scratch in all matrix jobs. Also note: for speed, we are using `git-sdk-64-minimal`, even if it lacks the `/bin/install` that we need to install Git's files; We're providing a minimal shell script shim instead. Also, we do not need to bother with the Tcl/Tk parts, therefore we're skipping them, too. Signed-off-by: Johannes Schindelin --- .github/workflows/scalar-functional-tests.yml | 216 ++++++++++++++++++ 1 file changed, 216 insertions(+) create mode 100644 .github/workflows/scalar-functional-tests.yml diff --git a/.github/workflows/scalar-functional-tests.yml b/.github/workflows/scalar-functional-tests.yml new file mode 100644 index 00000000000000..2aa0f5a183eb09 --- /dev/null +++ b/.github/workflows/scalar-functional-tests.yml @@ -0,0 +1,216 @@ +name: Scalar Functional Tests + +env: + SCALAR_REPOSITORY: dscho/scalar + SCALAR_REF: vfs-2.32.0 + DEBUG_WITH_TMATE: false + +on: + push: + branches: [ vfs-* ] + pull_request: + branches: [ vfs-* ] + +jobs: + scalar: + name: "Scalar Functional Tests" + + strategy: + fail-fast: false + matrix: + # Order by runtime (in descending order) + os: [windows-2019, macos-10.15, ubuntu-16.04, ubuntu-18.04, ubuntu-20.04] + features: [false, experimental] + exclude: + # The built-in FSMonitor is not (yet) supported on Linux + - os: ubuntu-16.04 + features: experimental + - os: ubuntu-18.04 + features: experimental + - os: ubuntu-20.04 + features: experimental + runs-on: ${{ matrix.os }} + + env: + BUILD_FRAGMENT: bin/Release/netcoreapp3.1 + + steps: + - name: Check out Git's source code + uses: actions/checkout@v2 + + - name: Setup build tools on Windows + if: runner.os == 'Windows' + uses: git-for-windows/setup-git-for-windows-sdk@v1 + + - name: Provide a minimal `install` on Windows + if: runner.os == 'Windows' + shell: bash + run: | + test -x /usr/bin/install || + tr % '\t' >/usr/bin/install <<-\EOF + #!/bin/sh + + cmd=cp + while test $# != 0 + do + %case "$1" in + %-d) cmd="mkdir -p";; + %-m) shift;; # ignore mode + %*) break;; + %esac + %shift + done + + exec $cmd "$@" + EOF + + - name: Install build dependencies for Git (Linux) + if: runner.os == 'Linux' + run: | + sudo apt-get update + sudo apt-get -q -y install libssl-dev libcurl4-openssl-dev gettext + + - name: Build and install Git + shell: bash + env: + NO_TCLTK: Yup + run: | + # We do require a VFS version + def_ver="$(sed -n 's/DEF_VER=\(.*vfs.*\)/\1/p' GIT-VERSION-GEN)" + test -n "$def_ver" + + # Ensure that `git version` reflects DEF_VER + case "$(git describe --match "v[0-9]*vfs*" HEAD)" in + ${def_ver%%.vfs.*}.vfs.*) ;; # okay, we can use this + *) git -c user.name=ci -c user.email=ci@github tag -m for-testing ${def_ver}.NNN.g$(git rev-parse --short HEAD);; + esac + + SUDO= + extra= + case "${{ runner.os }}" in + Windows) + extra=DESTDIR=/c/Progra~1/Git + cygpath -aw "/c/Program Files/Git/cmd" >>$GITHUB_PATH + ;; + Linux) + SUDO=sudo + extra=prefix=/usr + ;; + macOS) + SUDO=sudo + extra=prefix=/usr/local + ;; + esac + + $SUDO make -j5 $extra install + + - name: Ensure that we use the built Git (Windows) + if: runner.os == 'Windows' + shell: powershell + run: | + cmd /c where git + git version + if ((git version) -like "*.vfs.*") { echo Good } else { exit 1 } + + - name: Check out Scalar's source code + uses: actions/checkout@v2 + with: + fetch-depth: 0 # Indicate full history so Nerdbank.GitVersioning works. + path: scalar + repository: ${{ env.SCALAR_REPOSITORY }} + ref: ${{ env.SCALAR_REF }} + + - name: Setup .NET Core + uses: actions/setup-dotnet@v1 + with: + dotnet-version: 3.1.302 + + - name: Install dependencies + run: dotnet restore + working-directory: scalar + env: + DOTNET_NOLOGO: 1 + + - name: Build + working-directory: scalar + run: dotnet build --configuration Release --no-restore -p:UseAppHost=true # Force generation of executable on macOS. + + - name: Setup platform (Linux) + if: runner.os == 'Linux' + run: | + echo "BUILD_PLATFORM=${{ runner.os }}" >>$GITHUB_ENV + echo "TRACE2_BASENAME=Trace2.${{ github.run_id }}__${{ github.run_number }}__${{ matrix.os }}__${{ matrix.features }}" >>$GITHUB_ENV + + - name: Setup platform (Mac) + if: runner.os == 'macOS' + run: | + echo 'BUILD_PLATFORM=Mac' >>$GITHUB_ENV + echo "TRACE2_BASENAME=Trace2.${{ github.run_id }}__${{ github.run_number }}__${{ matrix.os }}__${{ matrix.features }}" >>$GITHUB_ENV + + - name: Setup platform (Windows) + if: runner.os == 'Windows' + run: | + echo "BUILD_PLATFORM=${{ runner.os }}" >>$env:GITHUB_ENV + echo 'BUILD_FILE_EXT=.exe' >>$env:GITHUB_ENV + echo "TRACE2_BASENAME=Trace2.${{ github.run_id }}__${{ github.run_number }}__${{ matrix.os }}__${{ matrix.features }}" >>$env:GITHUB_ENV + + - name: Configure feature.scalar + run: git config --global feature.scalar ${{ matrix.features }} + + - id: functional_test + name: Functional test + timeout-minutes: 60 + working-directory: scalar + shell: bash + run: | + export GIT_TRACE2_EVENT="$PWD/$TRACE2_BASENAME/Event" + export GIT_TRACE2_PERF="$PWD/$TRACE2_BASENAME/Perf" + export GIT_TRACE2_EVENT_BRIEF=true + export GIT_TRACE2_PERF_BRIEF=true + mkdir -p "$TRACE2_BASENAME" + mkdir -p "$TRACE2_BASENAME/Event" + mkdir -p "$TRACE2_BASENAME/Perf" + git version --build-options + cd ../out + PATH="$PWD/Scalar/$BUILD_FRAGMENT:$PWD/Scalar.Service/$BUILD_FRAGMENT:$PATH" + Scalar.FunctionalTests/$BUILD_FRAGMENT/Scalar.FunctionalTests$BUILD_FILE_EXT --test-scalar-on-path --test-git-on-path --timeout=300000 --full-suite + + - name: Force-stop FSMonitor daemons and Git processes (Windows) + if: runner.os == 'Windows' && matrix.features == 'experimental' && (success() || failure()) + shell: bash + run: | + set -x + wmic process get CommandLine,ExecutablePath,HandleCount,Name,ParentProcessID,ProcessID + wmic process where "CommandLine Like '%fsmonitor--daemon %run'" delete + wmic process where "ExecutablePath Like '%git.exe'" delete + + - id: trace2_zip_unix + if: runner.os != 'Windows' && ( success() || failure() ) && ( steps.functional_test.conclusion == 'success' || steps.functional_test.conclusion == 'failure' ) + name: Zip Trace2 Logs (Unix) + shell: bash + working-directory: scalar + run: zip -q -r $TRACE2_BASENAME.zip $TRACE2_BASENAME/ + + - id: trace2_zip_windows + if: runner.os == 'Windows' && ( success() || failure() ) && ( steps.functional_test.conclusion == 'success' || steps.functional_test.conclusion == 'failure' ) + name: Zip Trace2 Logs (Windows) + working-directory: scalar + run: Compress-Archive -DestinationPath ${{ env.TRACE2_BASENAME }}.zip -Path ${{ env.TRACE2_BASENAME }} + + - name: Archive Trace2 Logs + if: ( success() || failure() ) && ( steps.trace2_zip_unix.conclusion == 'success' || steps.trace2_zip_windows.conclusion == 'success' ) + uses: actions/upload-artifact@v2 + with: + name: ${{ env.TRACE2_BASENAME }}.zip + path: scalar/${{ env.TRACE2_BASENAME }}.zip + retention-days: 3 + + # The GitHub Action `action-tmate` allows developers to connect to the running agent + # using SSH (it will be a `tmux` session; on Windows agents it will be inside the MSYS2 + # environment in `C:\msys64`, therefore it can be slightly tricky to interact with + # Git for Windows, which runs a slightly incompatible MSYS2 runtime). + - name: action-tmate + if: env.DEBUG_WITH_TMATE == 'true' && failure() + uses: mxschmitt/action-tmate@v3 + with: + limit-access-to-actor: true