trace2: add macOS and Windows process ancestry tracing

In 353d3d77 (trace2: collect Windows-specific process information) Windows-specific process ancestry information was added as a data_json event to TRACE2. Furthermore in 2f732bf1 (tr2: log parent process name) similar functionality was added for Linux-based systems, using procfs.

Let's teach Git on macOS to also gather process ancestry information, and emit it as a cmd_ancestry TRACE2 event.

Furthermore, let's refactor the Windows implementation to align with the Linux and macOS versions - by emitting the ancestry information as a cmd_ancestry event. We keep the older, custom data_json event type on Windows for compatibility for consumers of the TRACE2 data that use the older event.

Thanks, Matthew

Matthew John Cheetham (4):
trace2: add macOS process ancestry tracing
build: include procinfo.c impl for macOS
trace2: refactor Windows process ancestry trace2 event
trace2: emit cmd_ancestry data for Windows

 compat/darwin/procinfo.c                 | 99 ++++++++++++++++++++++++
 compat/win32/trace2_win32_process_info.c | 58 ++++++++------
 config.mak.uname                         |  2 +
 contrib/buildsystems/CMakeLists.txt      |  2 +
 meson.build                              |  2 +
 5 files changed, 138 insertions(+), 25 deletions(-)
 create mode 100644 compat/darwin/procinfo.c

base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2040%2Fmjcheetham%2Ftrace2-macos-ancestry-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2040/mjcheetham/trace2-macos-ancestry-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2040

gitgitgadget

From: Matthew John Cheetham <[email protected]>

In 353d3d77 (trace2: collect Windows-specific process information) Windows-specific process ancestry information was added as a data_json event to TRACE2. Furthermore in 2f732bf1 (tr2: log parent process name) similar functionality was added for Linux-based systems, using procfs.

Teach Git to also log process ancestry on macOS using the sysctl with KERN_PROC to get process information (PPID and process name). Like the Linux implementation, we use the cmd_ancestry TRACE2 event rather than using a data_json event and creating another custom data point.

---
 compat/darwin/procinfo.c | 99 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 compat/darwin/procinfo.c

new file mode 100644
index 0000000000..e0d76d81ec
@@ -0,0 +1,99 @@
+#define USE_THE_REPOSITORY_VARIABLE
+
+#include "git-compat-util.h"
+#include "strbuf.h"
+#include "strvec.h"
+#include "trace2.h"
+#include <sys/sysctl.h>
+
+/*
+ * An arbitrarily chosen value to limit the depth of the ancestor chain.
+ */
+#define NR_PIDS_LIMIT 10
+
+/*
+ * Get the process name and parent PID for a given PID using sysctl().
+ * Returns 0 on success, -1 on failure.
+ */
+static int get_proc_info(pid_t pid, struct strbuf *name, pid_t *ppid)
+{
+	int mib[4];
+	struct kinfo_proc proc;
+	size_t size = sizeof(proc);
+
+	mib[0] = CTL_KERN;
+	mib[1] = KERN_PROC;
+	mib[2] = KERN_PROC_PID;
+	mib[3] = pid;
+
+	if (sysctl(mib, 4, &proc, &size, NULL, 0) < 0)
+		return -1;
+
+	if (size == 0)
+		return -1;
+
+	strbuf_addstr(name, proc.kp_proc.p_comm);
+	*ppid = proc.kp_eproc.e_ppid;
+
+	return 0;
+}
+
+/*
+ * Recursively push process names onto the ancestry array.
+ * We guard against cycles by limiting the depth to NR_PIDS_LIMIT.
+ */
+static void push_ancestry_name(struct strvec *names, pid_t pid, int depth)
+{
+	struct strbuf name = STRBUF_INIT;
+	pid_t ppid;
+
+	if (depth >= NR_PIDS_LIMIT)
+		return;
+
+	if (pid <= 0)
+		return;
+
+	if (get_proc_info(pid, &name, &ppid) < 0)
+		goto cleanup;
+
+	strvec_push(names, name.buf);
+
+	/*
+	 * Recurse to the parent process. Stop if ppid is 0 or 1
+	 * (init/launchd) or if we've reached ourselves (cycle).
+	 */
+	if (ppid > 1 && ppid != pid)
+		push_ancestry_name(names, ppid, depth + 1);
+
+cleanup:
+	strbuf_release(&name);
+}
+
+void trace2_collect_process_info(enum trace2_process_info_reason reason)
+{
+	struct strvec names = STRVEC_INIT;
+
+	if (!trace2_is_enabled())
+		return;
+
+	switch (reason) {
+	case TRACE2_PROCESS_INFO_STARTUP:
+		push_ancestry_name(&names, getppid(), 0);
+		if (names.nr)
+			trace2_cmd_ancestry(names.v);
+
+		strvec_clear(&names);
+		break;
+
+	case TRACE2_PROCESS_INFO_EXIT:
+		/*
+		 * The Windows version of this calls its
+		 * get_peak_memory_info() here. We may want to insert
+		 * similar process-end statistics here in the future.
+		 */
+		break;
+
+	default:
+		BUG("trace2_collect_process_info: unknown reason '%d'", reason);
+	}
+}

gitgitgadget

If we are creating a new file, then it would be best if we avoid this macro, which is intended for older code to still work until it can be fixed.

But also it seems that you don't use the_repository anywhere, so this can be deleted without consequence!

You are correct - I neglected to remove this macro when preparing to submit the series. I can remove on the next iteration.

I had based this on the compat/linux/procinfo.c implementation which also uses recursion to walk the parent processes (and also defines an upper limit to the number of processes to walk).

If I were to transform this to a loop, would we not also be wanting to update linux/procinfo.c too?

Thanks,
Matthew

If you're matching the structure in the Linux version, then keep it.

If it's worth fixing, then it would be worth fixing in both places at a later time.

Thanks,
-Stolee

From: Matthew John Cheetham <[email protected]>

Include an implementation of trace2_collect_process_info for macOS.

---
 config.mak.uname                    | 2 ++
 contrib/buildsystems/CMakeLists.txt | 2 ++
 meson.build                         | 2 ++
 3 files changed, 6 insertions(+)

index 1691c6ae6e..baa5018461 100644
@@ -148,6 +148,8 @@ ifeq ($(uname_S),Darwin)
 	HAVE_NS_GET_EXECUTABLE_PATH = YesPlease
 	CSPRNG_METHOD = arc4random
 	USE_ENHANCED_BASIC_REGULAR_EXPRESSIONS = YesPlease
+	HAVE_PLATFORM_PROCINFO = YesPlease
+	COMPAT_OBJS += compat/darwin/procinfo.o
 
 	# Workaround for `gettext` being keg-only and not even being linked via
 	# `brew link --force gettext`, should be obsolete as of

index edb0fc04ad..d489f0cada 100644
@@ -274,6 +274,8 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
 elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux")
 	add_compile_definitions(PROCFS_EXECUTABLE_PATH="/proc/self/exe" HAVE_DEV_TTY )
 	list(APPEND compat_SOURCES unix-socket.c unix-stream-server.c compat/linux/procinfo.c)
+elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
+	list(APPEND compat_SOURCES compat/darwin/procinfo.c)
 endif()
 
 if(CMAKE_SYSTEM_NAME STREQUAL "Windows")

index 1f95a06edb..32d470e4f7 100644
@@ -1292,6 +1292,8 @@ if host_machine.system() == 'linux'
   libgit_sources += 'compat/linux/procinfo.c'
 elif host_machine.system() == 'windows'
   libgit_sources += 'compat/win32/trace2_win32_process_info.c'
+elif host_machine.system() == 'darwin'
+  libgit_sources += 'compat/darwin/procinfo.c'
 else
   libgit_sources += 'compat/stub/procinfo.c'
 endif

gitgitgadget

So many build systems!

Each logic you included seems to be correct and matches the patterns from the Linux case.

Thanks,
-Stolee

From: Matthew John Cheetham <[email protected]>

In 353d3d77 (trace2: collect Windows-specific process information) we added process ancestry information for Windows to TRACE2 via a data_json event. It was only later in 2f732bf1 (tr2: log parent process name) that the specific cmd_ancestry event was added to TRACE2.

In a future commit we will emit the ancestry information with the newer cmd_ancestry TRACE2 event. Right now, we rework this implementation of trace2_collect_process_info to separate the calculation of ancestors from building and emiting the JSON array via a data_json event.

---
 compat/win32/trace2_win32_process_info.c | 50 ++++++++++++------------
 1 file changed, 25 insertions(+), 25 deletions(-)

index f147da706a..aceea05430 100644
@@ -3,6 +3,7 @@
 #include "../../git-compat-util.h"
 #include "../../json-writer.h"
 #include "../../repository.h"
+#include "../../strvec.h"
 #include "../../trace2.h"
 #include "lazyload.h"
 #include <psapi.h>

@@ -32,12 +33,7 @@ static int find_pid(DWORD pid, HANDLE hSnapshot, PROCESSENTRY32 *pe32)
 }
 
 /*
- * Accumulate JSON array of our parent processes:
- *     [
- *         exe-name-parent,
- *         exe-name-grand-parent,
- *         ...
- *     ]
+ * Accumulate array of our parent process names.
  *
  * Note: we only report the filename of the process executable; the
  *       only way to get its full pathname is to use OpenProcess()

@@ -73,7 +69,7 @@ static int find_pid(DWORD pid, HANDLE hSnapshot, PROCESSENTRY32 *pe32)
  * simple and avoid the alloc/realloc overhead.  It is OK if we
  * truncate the search and return a partial answer.
  */
-static void get_processes(struct json_writer *jw, HANDLE hSnapshot)
+static void get_processes(struct strvec *names, HANDLE hSnapshot)
 {
 	PROCESSENTRY32 pe32;
 	DWORD pid;

@@ -82,19 +78,19 @@ static void get_processes(struct json_writer *jw, HANDLE hSnapshot)
 
 	pid = GetCurrentProcessId();
 	while (find_pid(pid, hSnapshot, &pe32)) {
-		/* Only report parents. Omit self from the JSON output. */
+		/* Only report parents. Omit self from the output. */
 		if (nr_pids)
-			jw_array_string(jw, pe32.szExeFile);
+			strvec_push(names, pe32.szExeFile);
 
 		/* Check for cycle in snapshot. (Yes, it happened.) */
 		for (k = 0; k < nr_pids; k++)
 			if (pid == pid_list[k]) {
-				jw_array_string(jw, "(cycle)");
+				strvec_push(names, "(cycle)");
 				return;
 			}
 
 		if (nr_pids == NR_PIDS_LIMIT) {
-			jw_array_string(jw, "(truncated)");
+			strvec_push(names, "(truncated)");
 			return;
 		}
 

@@ -105,24 +101,14 @@ static void get_processes(struct json_writer *jw, HANDLE hSnapshot)
 }
 
 /*
- * Emit JSON data for the current and parent processes.  Individual
- * trace2 targets can decide how to actually print it.
+ * Collect the list of parent process names.
  */
-static void get_ancestry(void)
+static void get_ancestry(struct strvec *names)
 {
 	HANDLE hSnapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
 
 	if (hSnapshot != INVALID_HANDLE_VALUE) {
-		struct json_writer jw = JSON_WRITER_INIT;
-
-		jw_array_begin(&jw, 0);
-		get_processes(&jw, hSnapshot);
-		jw_end(&jw);
-
-		trace2_data_json("process", the_repository, "windows/ancestry",
-				 &jw);
-
-		jw_release(&jw);
+		get_processes(names, hSnapshot);
 		CloseHandle(hSnapshot);
 	}
 }

@@ -176,13 +162,27 @@ static void get_peak_memory_info(void)
 
 void trace2_collect_process_info(enum trace2_process_info_reason reason)
 {
+	struct strvec names = STRVEC_INIT;
+
 	if (!trace2_is_enabled())
 		return;
 
 	switch (reason) {
 	case TRACE2_PROCESS_INFO_STARTUP:
 		get_is_being_debugged();
-		get_ancestry();
+		get_ancestry(&names);
+		if (names.nr) {
+			struct json_writer jw = JSON_WRITER_INIT;
+			jw_array_begin(&jw, 0);
+			for (size_t i = 0; i < names.nr; i++)
+				jw_array_string(&jw, names.v[i]);
+			jw_end(&jw);
+			trace2_data_json("process", the_repository,
+					 "windows/ancestry", &jw);
+			jw_release(&jw);
+		}
+
+		strvec_clear(&names);
 		return;
 
 	case TRACE2_PROCESS_INFO_EXIT:

gitgitgadget

...

Are we able to delete this after your change?

From: Matthew John Cheetham <[email protected]>

Since 2f732bf1 (tr2: log parent process name) it is now possible to emit a specific process ancestry event in TRACE2. We should emit the Windows process ancestry data with the correct event type.

To not break existing consumers of the data_json "windows/ancestry" event, we continue to emit the ancestry data as a JSON event.

---
 compat/win32/trace2_win32_process_info.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index aceea05430..6a6a396078 100644
@@ -172,6 +172,11 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason)
 		get_is_being_debugged();
 		get_ancestry(&names);
 		if (names.nr) {
+			/*
+			  Emit the ancestry data as a data_json event to
+			  maintain compatibility for consumers of the older
+			  "windows/ancestry" event.
+			 */
 			struct json_writer jw = JSON_WRITER_INIT;
 			jw_array_begin(&jw, 0);
 			for (size_t i = 0; i < names.nr; i++)

@@ -180,6 +185,9 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason)
 			trace2_data_json("process", the_repository,
 					 "windows/ancestry", &jw);
 			jw_release(&jw);
+
+			/* Emit the ancestry data with the new event. */
+			trace2_cmd_ancestry(names.v);
 		}
 
 		strvec_clear(&names);

gitgitgadget

The usual way to refer to commits is to use git show -s --pretty=reference. (Or maybe with --abbrev=8 as well which seems to be the case here.) That also adds the date. See SubmittingPatches.

This is the important compatibility statement. When this merges, we should make a claim that in two major versions we will drop this compatibility.

Thanks,
-Stolee

This is done in patches 1-2. I notice that there are no tests validating that this works.

I see that in t/t0210-trace2-normal.sh there is a scrub_normal() helper that removes the cmd_ancestry event due to compatibility reasons.

I'd be interested to see if we could enable these tests to demonstrate your changes here. Of course, Windows, Linux, and macOS are not the _only_ platforms we support. They are the only ones we check in CI, though.

I appreciate this compatibility approach. I mention in my patch-by-patch review that we should eventually drop the old mechanism, say in two major versions.

The code looks good to me, just a question about the testing and some very minor nitpicks around recursion and strvecs.

Thanks,
-Stolee

Thanks for a prompt review (and thanks for the series author for writing the patches, of course).

Will mark as "expecting a hopefully minor and final reroll".

In 353d3d77f4 (trace2: collect Windows-specific process information, 2019-02-22) Windows-specific process ancestry information was added as a data_json event to TRACE2. Furthermore in 2f732bf15e (tr2: log parent process name, 2021-07-21) similar functionality was added for Linux-based systems, using procfs.

Let's teach Git on macOS to also gather process ancestry information, and emit it as a cmd_ancestry TRACE2 event.

Furthermore, let's refactor the Windows implementation to align with the Linux and macOS versions - by emitting the ancestry information as a cmd_ancestry event. We keep the older, custom data_json event type on Windows for compatibility for consumers of the TRACE2 data that use the older event.

Finally, we add tests of the cmd_ancestry events in the new t0213 test script. Extend the trace2 test helper to allow us to execute commands with a known process in the ancestry ("test-tool"); we use this to allow tests to filter out the uncontrolled environment (how the test script was run, and on what system).

Thanks, Matthew

Updates in v2
=============

* On macOS do not filter out PPID 1 or 0, to match what the Linux implementation does.

Stopping before PID 1 and 0 means we do not emit the launchd init process on macOS. The Linux implementation does not do this, nor does the Windows implementation.

* Add t0213-trace2-ancestry tests and extend the trace2 test helper.

The tests use the "400ancestry" test helper to spawn child processes with controlled trace2 environments. Verify that the process ancestry is being correctly captured on platforms that support cmd_ancestry.

* Drop USE_THE_REPOSITORY_VARIABLE macro as it was not required.

* Updated commit messages to use more standard format to refer to existing commits.

Matthew John Cheetham (6):
trace2: add macOS process ancestry tracing
build: include procinfo.c impl for macOS
trace2: refactor Windows process ancestry trace2 event
trace2: emit cmd_ancestry data for Windows
test-tool: extend trace2 helper with 400ancestry
t0213: add trace2 cmd_ancestry tests

 compat/darwin/procinfo.c                 |  97 ++++++++++++
 compat/win32/trace2_win32_process_info.c |  58 ++++----
 config.mak.uname                         |   2 +
 contrib/buildsystems/CMakeLists.txt      |   2 +
 meson.build                              |   2 +
 t/helper/test-trace2.c                   |  59 ++++++++
 t/meson.build                            |   1 +
 t/t0210-trace2-normal.sh                 |   5 +-
 t/t0213-trace2-ancestry.sh               | 180 +++++++++++++++++++++++
 9 files changed, 379 insertions(+), 27 deletions(-)
 create mode 100644 compat/darwin/procinfo.c
 create mode 100755 t/t0213-trace2-ancestry.sh

base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2040%2Fmjcheetham%2Ftrace2-macos-ancestry-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2040/mjcheetham/trace2-macos-ancestry-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2040

Range-diff vs v1:

1: d99a30a1a7 ! 1: 233f6cdd33 trace2: add macOS process ancestry tracing
@@ Metadata
## Commit message ##
trace2: add macOS process ancestry tracing

- In 353d3d77 (trace2: collect Windows-specific process information)
- Windows-specific process ancestry information was added as a data_json
- event to TRACE2. Furthermore in 2f732bf1 (tr2: log parent process name)
- similar functionality was added for Linux-based systems, using procfs.
+ In 353d3d77f4 (trace2: collect Windows-specific process information,
+ 2019-02-22) Windows-specific process ancestry information was added as
+ a data_json event to TRACE2. Furthermore in 2f732bf15e (tr2: log
+ parent process name, 2021-07-21) similar functionality was added for
+ Linux-based systems, using procfs.

Teach Git to also log process ancestry on macOS using the sysctl with KERN_PROC to get process information (PPID and process name).
@@ Commit message

## compat/darwin/procinfo.c (new) ##
@@
-+#define USE_THE_REPOSITORY_VARIABLE
-+
+#include "git-compat-util.h"
+#include "strbuf.h"
+#include "strvec.h"
@@ compat/darwin/procinfo.c (new)
+ strvec_push(names, name.buf);
+
+ /*
-+ * Recurse to the parent process. Stop if ppid is 0 or 1
-+ * (init/launchd) or if we've reached ourselves (cycle).
++ * Recurse to the parent process. Stop if ppid not valid
++ * or if we've reached ourselves (cycle).
+ */
-+ if (ppid > 1 && ppid != pid)
++ if (ppid && ppid != pid)
+ push_ancestry_name(names, ppid, depth + 1);
+
+cleanup:

 2:  c786a038f3 = 2:  546fcc3446 build: include procinfo.c impl for macOS
 3:  7ccd0a9a6d ! 3:  2b02f62f0d trace2: refactor Windows process ancestry trace2 event

@@ Metadata
## Commit message ##
trace2: refactor Windows process ancestry trace2 event

- In 353d3d77 (trace2: collect Windows-specific process information) we
- added process ancestry information for Windows to TRACE2 via a data_json
- event. It was only later in 2f732bf1 (tr2: log parent process name) that
- the specific cmd_ancestry event was added to TRACE2.
+ In 353d3d77f4 (trace2: collect Windows-specific process information,
+ 2019-02-22) we added process ancestry information for Windows to TRACE2
+ via a data_json event. It was only later in 2f732bf15e (tr2: log parent
+ process name, 2021-07-21) that the specific cmd_ancestry event was
+ added to TRACE2.

In a future commit we will emit the ancestry information with the newer cmd_ancestry TRACE2 event. Right now, we rework this implementation of
4: a06344dc75 ! 4: 6b9054115e trace2: emit cmd_ancestry data for Windows
@@ Metadata
## Commit message ##
trace2: emit cmd_ancestry data for Windows

- Since 2f732bf1 (tr2: log parent process name) it is now possible to emit
- a specific process ancestry event in TRACE2. We should emit the Windows
- process ancestry data with the correct event type.
+ Since 2f732bf15e (tr2: log parent process name, 2021-07-21) it is now
+ now possible to emit a specific process ancestry event in TRACE2. We
+ should emit the Windows process ancestry data with the correct event
+ type.

To not break existing consumers of the data_json "windows/ancestry" event, we continue to emit the ancestry data as a JSON event.

 -:  ---------- > 5:  b9a94291a6 test-tool: extend trace2 helper with 400ancestry
 -:  ---------- > 6:  6a5232540e t0213: add trace2 cmd_ancestry tests

gitgitgadget

From: Matthew John Cheetham <[email protected]>

In 353d3d77f4 (trace2: collect Windows-specific process information, 2019-02-22) Windows-specific process ancestry information was added as a data_json event to TRACE2. Furthermore in 2f732bf15e (tr2: log parent process name, 2021-07-21) similar functionality was added for Linux-based systems, using procfs.

Teach Git to also log process ancestry on macOS using the sysctl with KERN_PROC to get process information (PPID and process name). Like the Linux implementation, we use the cmd_ancestry TRACE2 event rather than using a data_json event and creating another custom data point.

---
 compat/darwin/procinfo.c | 97 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)
 create mode 100644 compat/darwin/procinfo.c

new file mode 100644
index 0000000000..c8954f02d7
@@ -0,0 +1,97 @@
+#include "git-compat-util.h"
+#include "strbuf.h"
+#include "strvec.h"
+#include "trace2.h"
+#include <sys/sysctl.h>
+
+/*
+ * An arbitrarily chosen value to limit the depth of the ancestor chain.
+ */
+#define NR_PIDS_LIMIT 10
+
+/*
+ * Get the process name and parent PID for a given PID using sysctl().
+ * Returns 0 on success, -1 on failure.
+ */
+static int get_proc_info(pid_t pid, struct strbuf *name, pid_t *ppid)
+{
+	int mib[4];
+	struct kinfo_proc proc;
+	size_t size = sizeof(proc);
+
+	mib[0] = CTL_KERN;
+	mib[1] = KERN_PROC;
+	mib[2] = KERN_PROC_PID;
+	mib[3] = pid;
+
+	if (sysctl(mib, 4, &proc, &size, NULL, 0) < 0)
+		return -1;
+
+	if (size == 0)
+		return -1;
+
+	strbuf_addstr(name, proc.kp_proc.p_comm);
+	*ppid = proc.kp_eproc.e_ppid;
+
+	return 0;
+}
+
+/*
+ * Recursively push process names onto the ancestry array.
+ * We guard against cycles by limiting the depth to NR_PIDS_LIMIT.
+ */
+static void push_ancestry_name(struct strvec *names, pid_t pid, int depth)
+{
+	struct strbuf name = STRBUF_INIT;
+	pid_t ppid;
+
+	if (depth >= NR_PIDS_LIMIT)
+		return;
+
+	if (pid <= 0)
+		return;
+
+	if (get_proc_info(pid, &name, &ppid) < 0)
+		goto cleanup;
+
+	strvec_push(names, name.buf);
+
+	/*
+	 * Recurse to the parent process. Stop if ppid not valid
+	 * or if we've reached ourselves (cycle).
+	 */
+	if (ppid && ppid != pid)
+		push_ancestry_name(names, ppid, depth + 1);
+
+cleanup:
+	strbuf_release(&name);
+}
+
+void trace2_collect_process_info(enum trace2_process_info_reason reason)
+{
+	struct strvec names = STRVEC_INIT;
+
+	if (!trace2_is_enabled())
+		return;
+
+	switch (reason) {
+	case TRACE2_PROCESS_INFO_STARTUP:
+		push_ancestry_name(&names, getppid(), 0);
+		if (names.nr)
+			trace2_cmd_ancestry(names.v);
+
+		strvec_clear(&names);
+		break;
+
+	case TRACE2_PROCESS_INFO_EXIT:
+		/*
+		 * The Windows version of this calls its
+		 * get_peak_memory_info() here. We may want to insert
+		 * similar process-end statistics here in the future.
+		 */
+		break;
+
+	default:
+		BUG("trace2_collect_process_info: unknown reason '%d'", reason);
+	}
+}

gitgitgadget

From: Matthew John Cheetham <[email protected]>

Include an implementation of trace2_collect_process_info for macOS.

---
 config.mak.uname                    | 2 ++
 contrib/buildsystems/CMakeLists.txt | 2 ++
 meson.build                         | 2 ++
 3 files changed, 6 insertions(+)

index 1691c6ae6e..baa5018461 100644
@@ -148,6 +148,8 @@ ifeq ($(uname_S),Darwin)
 	HAVE_NS_GET_EXECUTABLE_PATH = YesPlease
 	CSPRNG_METHOD = arc4random
 	USE_ENHANCED_BASIC_REGULAR_EXPRESSIONS = YesPlease
+	HAVE_PLATFORM_PROCINFO = YesPlease
+	COMPAT_OBJS += compat/darwin/procinfo.o
 
 	# Workaround for `gettext` being keg-only and not even being linked via
 	# `brew link --force gettext`, should be obsolete as of

index edb0fc04ad..d489f0cada 100644
@@ -274,6 +274,8 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
 elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux")
 	add_compile_definitions(PROCFS_EXECUTABLE_PATH="/proc/self/exe" HAVE_DEV_TTY )
 	list(APPEND compat_SOURCES unix-socket.c unix-stream-server.c compat/linux/procinfo.c)
+elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
+	list(APPEND compat_SOURCES compat/darwin/procinfo.c)
 endif()
 
 if(CMAKE_SYSTEM_NAME STREQUAL "Windows")

index 1f95a06edb..32d470e4f7 100644
@@ -1292,6 +1292,8 @@ if host_machine.system() == 'linux'
   libgit_sources += 'compat/linux/procinfo.c'
 elif host_machine.system() == 'windows'
   libgit_sources += 'compat/win32/trace2_win32_process_info.c'
+elif host_machine.system() == 'darwin'
+  libgit_sources += 'compat/darwin/procinfo.c'
 else
   libgit_sources += 'compat/stub/procinfo.c'
 endif

gitgitgadget

"Matthew John Cheetham via GitGitGadget" <[email protected]>

Looking good. I wondered if the first two steps should be a single patch (as the tree will be with a totally unused file until the second patch is applied), but this organization will give better chances for the second patch to be viewed by folks who are good at build infrastructure who are not necessarily interested in macOS specific programming, so it probably is better presented this way.

From: Matthew John Cheetham <[email protected]>

In 353d3d77f4 (trace2: collect Windows-specific process information, 2019-02-22) we added process ancestry information for Windows to TRACE2 via a data_json event. It was only later in 2f732bf15e (tr2: log parent process name, 2021-07-21) that the specific cmd_ancestry event was added to TRACE2.

In a future commit we will emit the ancestry information with the newer cmd_ancestry TRACE2 event. Right now, we rework this implementation of trace2_collect_process_info to separate the calculation of ancestors from building and emiting the JSON array via a data_json event.

---
 compat/win32/trace2_win32_process_info.c | 50 ++++++++++++------------
 1 file changed, 25 insertions(+), 25 deletions(-)

index f147da706a..aceea05430 100644
@@ -3,6 +3,7 @@
 #include "../../git-compat-util.h"
 #include "../../json-writer.h"
 #include "../../repository.h"
+#include "../../strvec.h"
 #include "../../trace2.h"
 #include "lazyload.h"
 #include <psapi.h>

@@ -32,12 +33,7 @@ static int find_pid(DWORD pid, HANDLE hSnapshot, PROCESSENTRY32 *pe32)
 }
 
 /*
- * Accumulate JSON array of our parent processes:
- *     [
- *         exe-name-parent,
- *         exe-name-grand-parent,
- *         ...
- *     ]
+ * Accumulate array of our parent process names.
  *
  * Note: we only report the filename of the process executable; the
  *       only way to get its full pathname is to use OpenProcess()

@@ -73,7 +69,7 @@ static int find_pid(DWORD pid, HANDLE hSnapshot, PROCESSENTRY32 *pe32)
  * simple and avoid the alloc/realloc overhead.  It is OK if we
  * truncate the search and return a partial answer.
  */
-static void get_processes(struct json_writer *jw, HANDLE hSnapshot)
+static void get_processes(struct strvec *names, HANDLE hSnapshot)
 {
 	PROCESSENTRY32 pe32;
 	DWORD pid;

@@ -82,19 +78,19 @@ static void get_processes(struct json_writer *jw, HANDLE hSnapshot)
 
 	pid = GetCurrentProcessId();
 	while (find_pid(pid, hSnapshot, &pe32)) {
-		/* Only report parents. Omit self from the JSON output. */
+		/* Only report parents. Omit self from the output. */
 		if (nr_pids)
-			jw_array_string(jw, pe32.szExeFile);
+			strvec_push(names, pe32.szExeFile);
 
 		/* Check for cycle in snapshot. (Yes, it happened.) */
 		for (k = 0; k < nr_pids; k++)
 			if (pid == pid_list[k]) {
-				jw_array_string(jw, "(cycle)");
+				strvec_push(names, "(cycle)");
 				return;
 			}
 
 		if (nr_pids == NR_PIDS_LIMIT) {
-			jw_array_string(jw, "(truncated)");
+			strvec_push(names, "(truncated)");
 			return;
 		}
 

@@ -105,24 +101,14 @@ static void get_processes(struct json_writer *jw, HANDLE hSnapshot)
 }
 
 /*
- * Emit JSON data for the current and parent processes.  Individual
- * trace2 targets can decide how to actually print it.
+ * Collect the list of parent process names.
  */
-static void get_ancestry(void)
+static void get_ancestry(struct strvec *names)
 {
 	HANDLE hSnapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
 
 	if (hSnapshot != INVALID_HANDLE_VALUE) {
-		struct json_writer jw = JSON_WRITER_INIT;
-
-		jw_array_begin(&jw, 0);
-		get_processes(&jw, hSnapshot);
-		jw_end(&jw);
-
-		trace2_data_json("process", the_repository, "windows/ancestry",
-				 &jw);
-
-		jw_release(&jw);
+		get_processes(names, hSnapshot);
 		CloseHandle(hSnapshot);
 	}
 }

@@ -176,13 +162,27 @@ static void get_peak_memory_info(void)
 
 void trace2_collect_process_info(enum trace2_process_info_reason reason)
 {
+	struct strvec names = STRVEC_INIT;
+
 	if (!trace2_is_enabled())
 		return;
 
 	switch (reason) {
 	case TRACE2_PROCESS_INFO_STARTUP:
 		get_is_being_debugged();
-		get_ancestry();
+		get_ancestry(&names);
+		if (names.nr) {
+			struct json_writer jw = JSON_WRITER_INIT;
+			jw_array_begin(&jw, 0);
+			for (size_t i = 0; i < names.nr; i++)
+				jw_array_string(&jw, names.v[i]);
+			jw_end(&jw);
+			trace2_data_json("process", the_repository,
+					 "windows/ancestry", &jw);
+			jw_release(&jw);
+		}
+
+		strvec_clear(&names);
 		return;
 
 	case TRACE2_PROCESS_INFO_EXIT:

gitgitgadget

"Matthew John Cheetham via GitGitGadget" <[email protected]>

Looks quite straight-forward refactoring that even I (without Win32 specific knowledge) can follow with ease. Nicely done.

From: Matthew John Cheetham <[email protected]>

Since 2f732bf15e (tr2: log parent process name, 2021-07-21) it is now now possible to emit a specific process ancestry event in TRACE2. We should emit the Windows process ancestry data with the correct event type.

To not break existing consumers of the data_json "windows/ancestry" event, we continue to emit the ancestry data as a JSON event.

---
 compat/win32/trace2_win32_process_info.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index aceea05430..6a6a396078 100644
@@ -172,6 +172,11 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason)
 		get_is_being_debugged();
 		get_ancestry(&names);
 		if (names.nr) {
+			/*
+			  Emit the ancestry data as a data_json event to
+			  maintain compatibility for consumers of the older
+			  "windows/ancestry" event.
+			 */
 			struct json_writer jw = JSON_WRITER_INIT;
 			jw_array_begin(&jw, 0);
 			for (size_t i = 0; i < names.nr; i++)

@@ -180,6 +185,9 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason)
 			trace2_data_json("process", the_repository,
 					 "windows/ancestry", &jw);
 			jw_release(&jw);
+
+			/* Emit the ancestry data with the new event. */
+			trace2_cmd_ancestry(names.v);
 		}
 
 		strvec_clear(&names);

gitgitgadget

"Matthew John Cheetham via GitGitGadget" <[email protected]>

Quite straight-forward. Given the way json event readers are organized, I suppose that existing readers will not choke on seeing the same data twice? Are we officially deprecating the "old way", or keeping both to serve two different audiences (i.e., those who have been in the ecosystem long before this change and want to keep reading in the format they are used to, and those who are writing cross platform data analyzers that want to stick to a single format)? I do not think we need to decide it now, and if I were asked, I would probably vote for the latter, but just thinking aloud.

Thanks.

From: Matthew John Cheetham <[email protected]>

Add a new test helper "400ancestry" to the trace2 test-tool that spawns a child process with a controlled trace2 environment, capturing only the child's trace2 output (including cmd_ancestry events) in isolation.

The helper clears all inherited GIT_TRACE2* variables in the child and enables only the requested target (normal, perf, or event), directing output to a specified file. This gives the test suite a reliable way to capture cmd_ancestry events: the child always sees "test-tool" as its immediate parent in the process ancestry, providing a predictable value to verify in tests.

---
 t/helper/test-trace2.c | 59 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

index 415df078c1..3b12f4173e 100644
@@ -466,6 +466,63 @@ static int ut_303redact_def_param(int argc, const char **argv)
 	return 0;
 }
 
+/*
+ * Run a child process with specific trace2 environment settings so that
+ * we can capture its trace2 output (including cmd_ancestry) in isolation.
+ *
+ * test-tool trace2 400ancestry <target> <output_file> [<child_command_line>]
+ *
+ * <target> is one of: normal, perf, event
+ *
+ * For example:
+ *     test-tool trace2 400ancestry normal out.normal test-tool trace2 001return 0
+ *
+ * The child process inherits a controlled trace2 environment where only
+ * the specified target is directed to <output_file>. The parent's trace2
+ * environment variables are cleared in the child so that only the child's
+ * events are captured.
+ *
+ * This is used by t0213-trace2-ancestry.sh to test cmd_ancestry events.
+ * The child process will see "test-tool" as its immediate parent in the
+ * process ancestry, giving us a predictable value to verify.
+ */
+static int ut_400ancestry(int argc, const char **argv)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	const char *target;
+	const char *outfile;
+	int result;
+
+	if (argc < 3)
+		die("expect <target> <output_file> <child_command_line>");
+
+	target = argv[0];
+	outfile = argv[1];
+	argv += 2;
+	argc -= 2;
+
+	/* Clear all trace2 environment variables in the child. */
+	strvec_push(&cmd.env, "GIT_TRACE2=");
+	strvec_push(&cmd.env, "GIT_TRACE2_PERF=");
+	strvec_push(&cmd.env, "GIT_TRACE2_EVENT=");
+	strvec_push(&cmd.env, "GIT_TRACE2_BRIEF=1");
+
+	/* Set only the requested target. */
+	if (!strcmp(target, "normal"))
+		strvec_pushf(&cmd.env, "GIT_TRACE2=%s", outfile);
+	else if (!strcmp(target, "perf"))
+		strvec_pushf(&cmd.env, "GIT_TRACE2_PERF=%s", outfile);
+	else if (!strcmp(target, "event"))
+		strvec_pushf(&cmd.env, "GIT_TRACE2_EVENT=%s", outfile);
+	else
+		die("invalid target '%s', expected: normal, perf, event",
+		    target);
+
+	strvec_pushv(&cmd.args, argv);
+	result = run_command(&cmd);
+	exit(result);
+}
+
 /*
  * Usage:
  *     test-tool trace2 <ut_name_1> <ut_usage_1>

@@ -497,6 +554,8 @@ static struct unit_test ut_table[] = {
 	{ ut_301redact_child_start, "301redact_child_start", "<argv...>" },
 	{ ut_302redact_exec,        "302redact_exec",        "<exe> <argv...>" },
 	{ ut_303redact_def_param,   "303redact_def_param",   "<key> <value>" },
+
+	{ ut_400ancestry,           "400ancestry",           "<target> <output_file> [<child_command_line>]" },
 };
 /* clang-format on */
 

gitgitgadget

From: Matthew John Cheetham <[email protected]>

Add a new test script t0213-trace2-ancestry.sh that verifies cmd_ancestry events across all three trace2 output formats (normal, perf, and event).

The tests use the "400ancestry" test helper to spawn child processes with controlled trace2 environments. Git alias resolution (which spawns a child git process) creates a predictable multi-level process tree. Filter functions extract cmd_ancestry events from each format, truncating the ancestor list at the outermost "test-tool" so that only the controlled portion of the tree is verified, regardless of the test runner environment.

A runtime prerequisite (TRACE2_ANCESTRY) is used to detect whether the platform has a real procinfo implementation; platforms with only the stub are skipped.

We must pay attention to an extra ancestor on Windows (MINGW) when running without the bin-wrappers (such as we do in CI). In this situation we see an extra "sh.exe" ancestor after "test-tool.exe".

Also update the comment in t0210-trace2-normal.sh to reflect that ancestry testing now has its own dedicated test script.

---
 t/meson.build              |   1 +
 t/t0210-trace2-normal.sh   |   5 +-
 t/t0213-trace2-ancestry.sh | 180 +++++++++++++++++++++++++++++++++++++
 3 files changed, 184 insertions(+), 2 deletions(-)
 create mode 100755 t/t0213-trace2-ancestry.sh

index a5531df415..551c3036c0 100644
@@ -131,6 +131,7 @@ integration_tests = [
   't0210-trace2-normal.sh',
   't0211-trace2-perf.sh',
   't0212-trace2-event.sh',
+  't0213-trace2-ancestry.sh',
   't0300-credentials.sh',
   't0301-credential-cache.sh',
   't0302-credential-store.sh',

index 96c68f65df..7e1e7af862 100755
@@ -74,8 +74,9 @@ scrub_normal () {
 	#      This line is only emitted when RUNTIME_PREFIX is defined,
 	#      so just omit it for testing purposes.
 	#
-	#   4. 'cmd_ancestry' is not implemented everywhere, so for portability's
-	#      sake, skip it when parsing normal.
+	#   4. 'cmd_ancestry' output depends on how the test is run and
+	#      is not relevant to the features we are testing here.
+	#      Ancestry tests are covered in t0213-trace2-ancestry.sh instead.
 	sed \
 		-e 's/elapsed:[0-9]*\.[0-9][0-9]*\([eE][-+]\{0,1\}[0-9][0-9]*\)\{0,1\}/elapsed:_TIME_/g' \
 		-e "s/^start '[^']*' \(.*\)/start _EXE_ \1/" \

gitgitgadget

I took a look at the chnages and the new test code. Everything satisfies my concerns from v1.

Thanks,
-Stole