Ben Peart <peartben@xxxxxxxxx> writes:
> diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c
> new file mode 100644
> index 0000000000..80830d920b
> --- /dev/null
> +++ b/t/helper/test-drop-caches.c
> @@ -0,0 +1,107 @@
> +#include "git-compat-util.h"
> +#include <stdio.h>
I thought compat-util should already include stdio?
> +
> +typedef DWORD NTSTATUS;
Is this safe to have outside "#ifdef GIT_WINDOWS_NATIVE"?
> +
> +#ifdef GIT_WINDOWS_NATIVE
> +#include <tchar.h>
> +
> +#define STATUS_SUCCESS (0x00000000L)
> +#define STATUS_PRIVILEGE_NOT_HELD (0xC0000061L)
> +
> +typedef enum _SYSTEM_INFORMATION_CLASS {
> + SystemMemoryListInformation = 80, // 80, q: SYSTEM_MEMORY_LIST_INFORMATION; s: SYSTEM_MEMORY_LIST_COMMAND (requires SeProfileSingleProcessPrivilege)
I would have said "Please avoid // comment in this codebase unless
we know we only use the compilers that grok it". This particular
one may be OK, as it is inside GIT_WINDOWS_NATIVE and I assume
everybody on that platform uses recent GCC (or VStudio groks it I
guess)?
> +} SYSTEM_INFORMATION_CLASS;
> +
> +// private
> +typedef enum _SYSTEM_MEMORY_LIST_COMMAND
> +{
Style: '{' comes at the end of the previous line, with a single SP
immediately before it, unless it is the beginning of the function body.
What you did for _SYSTEM_INFORMATION_CLASS above is correct.
> + MemoryCaptureAccessedBits,
> + MemoryCaptureAndResetAccessedBits,
> + MemoryEmptyWorkingSets,
> + MemoryFlushModifiedList,
> + MemoryPurgeStandbyList,
> + MemoryPurgeLowPriorityStandbyList,
> + MemoryCommandMax
Style: avoid CamelCase.
> +} SYSTEM_MEMORY_LIST_COMMAND;
> +
> +BOOL GetPrivilege(HANDLE TokenHandle, LPCSTR lpName, int flags)
> +{
> + BOOL bResult;
> + DWORD dwBufferLength;
> + LUID luid;
> + TOKEN_PRIVILEGES tpPreviousState;
> + TOKEN_PRIVILEGES tpNewState;
> +
> + dwBufferLength = 16;
> + bResult = LookupPrivilegeValueA(0, lpName, &luid);
> + if (bResult)
> + {
Style: '{' comes at the end of the previous line, with a single SP
immediately before it, unless it is the beginning of the function body.
> + tpNewState.PrivilegeCount = 1;
> + tpNewState.Privileges[0].Luid = luid;
> + tpNewState.Privileges[0].Attributes = 0;
> + bResult = AdjustTokenPrivileges(TokenHandle, 0, &tpNewState, (DWORD)((LPBYTE)&(tpNewState.Privileges[1]) - (LPBYTE)&tpNewState), &tpPreviousState, &dwBufferLength);
> + if (bResult)
> + {
> + tpPreviousState.PrivilegeCount = 1;
> + tpPreviousState.Privileges[0].Luid = luid;
> + tpPreviousState.Privileges[0].Attributes = flags != 0 ? 2 : 0;
> + bResult = AdjustTokenPrivileges(TokenHandle, 0, &tpPreviousState, dwBufferLength, 0, 0);
> + }
> + }
> + return bResult;
> +}
> +#endif
> +
> +int cmd_main(int argc, const char **argv)
> +{
> + NTSTATUS status = 1;
> +#ifdef GIT_WINDOWS_NATIVE
> + HANDLE hProcess = GetCurrentProcess();
> + HANDLE hToken;
> + if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES, &hToken))
> + {
> + _ftprintf(stderr, _T("Can't open current process token\n"));
> + return 1;
> + }
> +
> + if (!GetPrivilege(hToken, "SeProfileSingleProcessPrivilege", 1))
> + {
> + _ftprintf(stderr, _T("Can't get SeProfileSingleProcessPrivilege\n"));
> + return 1;
> + }
> +
> + CloseHandle(hToken);
> +
> + HMODULE ntdll = LoadLibrary(_T("ntdll.dll"));
> + if (!ntdll)
> + {
> + _ftprintf(stderr, _T("Can't load ntdll.dll, wrong Windows version?\n"));
> + return 1;
> + }
> +
> + NTSTATUS(WINAPI *NtSetSystemInformation)(INT, PVOID, ULONG) = (NTSTATUS(WINAPI *)(INT, PVOID, ULONG))GetProcAddress(ntdll, "NtSetSystemInformation");
Is this "decl-after-stmt"? Avoid it.
> + if (!NtSetSystemInformation)
> + {
> + _ftprintf(stderr, _T("Can't get function addresses, wrong Windows version?\n"));
> + return 1;
> + }
> +
> + SYSTEM_MEMORY_LIST_COMMAND command = MemoryPurgeStandbyList;
> + status = NtSetSystemInformation(
> + SystemMemoryListInformation,
> + &command,
> + sizeof(SYSTEM_MEMORY_LIST_COMMAND)
> + );
> + if (status == STATUS_PRIVILEGE_NOT_HELD)
> + {
> + _ftprintf(stderr, _T("Insufficient privileges to execute the memory list command"));
> + }
> + else if (status != STATUS_SUCCESS)
> + {
> + _ftprintf(stderr, _T("Unable to execute the memory list command %lX"), status);
> + }
> +#endif
> +
> + return status;
> +}
So status is initialized to 1 and anybody without GIT_WINDOWS_NATIVE
unconditionally get exit(1)?
Given that 'status' is the return value of this function that
returns 'int', I wonder if we need NTSTATUS type here.
Having said all that, I think you are using this ONLY on windows;
perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of
the above and arrange Makefile to build test-drop-cache only on that
platform, or something?
> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
> new file mode 100755
> index 0000000000..e41905cb9b
> --- /dev/null
> +++ b/t/perf/p7519-fsmonitor.sh
> @@ -0,0 +1,161 @@
> +#!/bin/sh
> +
> +test_description="Test core.fsmonitor"
> +
> +. ./perf-lib.sh
> +
> +# This has to be run with GIT_PERF_REPEAT_COUNT=1 to generate valid results.
> +# Otherwise the caching that happens for the nth run will negate the validity
> +# of the comparisons.
> +if [ "$GIT_PERF_REPEAT_COUNT" -ne 1 ]
> +then
Style:
if test "$GIT_PERF_REPEAT_COUNT" -ne 1
then
> + ...
> +test_expect_success "setup" '
> +...
> + # Hook scaffolding
> + mkdir .git/hooks &&
> + cp ../../../templates/hooks--query-fsmonitor.sample .git/hooks/query-fsmonitor &&
Does this assume $TRASH_DIRECTORY must be in $TEST_DIRECTORY/perf/?
Shouldn't t/perf/p[0-9][0-9][0-9][0-9]-*.sh scripts be capable of
running with the --root=<ramdisk> option? Perhaps take the copy
relative to $TEST_DIRECTORY?