From: Matthew John Cheetham <mjcheetham@outlook.com>
Date: Mon, 09 Feb 2026 15:13:07 GMT
Subject: Re: [PATCH 1/4] trace2: add macOS process ancestry tracing
Message-ID: <VI0PR03MB1163484F620092CD9CE36C836C065A@VI0PR03MB11634.eurprd03.prod.outlook.com>
In-Reply-To: <7390e189-16ac-43b3-a63c-a8b942d5934b@gmail.com>

On 09/02/2026 14:36, Derrick Stolee wrote:

> On 2/5/2026 11:05 AM, Matthew John Cheetham via GitGitGadget wrote:
>> 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.
> 
>> +#define USE_THE_REPOSITORY_VARIABLE
> 
> 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.

>> +/*
>> + * 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;
> 
> Here is the recursion limit check.
> 
>> +	if (pid <= 0)
>> +		return;
>> +
>> +	if (get_proc_info(pid, &name, &ppid) < 0)
>> +		goto cleanup;
>> +
>> +	strvec_push(names, name.buf);
> 
> This is copying the buffer, which is why you release it later.
> 
> Question: could we stop copying here and use strbuf_detach() at this
> point? That would be a very minor improvement, so feel free to ignore!
> 
> I took a look and rediscovered that strvecs do not have an option to not
> copy. I'm thinking about string_list. I'm not sure if there is any value
> in converting your code just to avoid some string duplication at this
> scale.
> 
>> +	/*
>> +	 * 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);
> 
> This kind of tail recursion could be easily converted into a loop. I
> usually prefer loops to recursion when possible, in case we want to allow
> an unlimited number of parents in the future.

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?

>> +cleanup:
>> +	strbuf_release(&name);
>> +}
> 
> I got a little confused by the lack of a .h file, but that's probably due
> to the extra magic being done at compile time to pick this file on a per-
> platform basis.
> 
> Indeed, trace2_collect_process_info() is defined in trace2.h.
> 
> Thanks,
> -Stolee

Thanks,
Matthew