On Fri, May 21, 2021 at 11:09:49AM +0900, Junio C Hamano wrote:
>
> Emily Shaffer <emilyshaffer@xxxxxxxxxx> writes:
>
> > Unfortunately, there's no cross-platform reliable way to gather the name
> > of the parent process. If procfs is present, we can use that; otherwise
> > we will need to discover the name another way. However, the process ID
> > should be sufficient regardless of platform.
>
> Not a strong objection, but I wonder if seeing random integer(s) is
> better than not having cmd_ancestry info at all. The latter better
> signals that the platform does not yet have the "parent process
> name" feature, I would think.
Hm, we could; I guess then analyzers could still correlate "all these
things were called by some kind of wrapper, even if we don't know if
that was an IDE or a script or just the user or what, we can guess based
on other heuristics". Ok.
>
> > Git for Windows also gathers information about more than one parent. In
> > Linux further ancestry info can be gathered with procfs, but it's
> > unwieldy to do so. In the interest of later moving Git for Windows
> > ancestry logging to the 'cmd_ancestry' event, and in the interest of
> > later adding more ancestry to the Linux implementation - or of adding
> > this functionality to other platforms which have an easier time walking
> > the process tree - let's make 'cmd_ancestry' accept an array of
> > parentage.
>
> Could we rephrase "more than one parent" at the beginning to
> clarify? I initially had to wonder what "an array of parentage"
> contains (father and mother, or a sole parent and its sole parent,
> which is a sole grandparent). Since there is no "multiple processes
> meet and spawn a single process", I take it is the latter. Perhaps
> "more than one generation of" or something?
Good point, will refer to "more than one generation". Thanks.
> > + if (!strbuf_read_file(&out, procfs_path.buf, 0))
> > + {
>
> Place this opening brace at the end of the previous line.
Will polish up this and others for v3, hopefully today.
> > + if (!names[0])
> > + return;
>
> OK, so if there is no name given, we do not show pid as a
> placeholder.
Based on your suggestion above I think it will make sense to show pid as
placeholder after all, though. So I will change that for v3.
> > PROCFS_EXECUTABLE_PATH = /proc/self/exe
> > + HAVE_PROCFS_LINUX = YesPlease
>
> Have all Linux instances procfs enabled and mounted? It might be
> that we need to detect this at runtime anyway?
>
> ... goes and thinks ...
>
> Ah, OK, that "try reading from proc/%d/comm" is the runtime
> detection, so it is only this Makefile variable is slightly
> misnamed (it is not "HAVE" but "is worth checking for it").
I wonder what is better. "MAYBE_PROCFS_LINUX"? I don't see any other
vars in config.mak.uname that indicate "pretty sure but not totally
sure" in a quick scan. However, right above this line we seem to feel
certain in our guess about "PROCFS_EXECUTABLE_PATH"...
...but when it is used in exec-cmd.c:git_get_exec_path_procfs(), invoked
by exec-cmd.c:git_get_exec_path(), we're very tolerant to faults if it's
not there:
static int git_get_exec_path(struct strbuf *buf, const char *argv0)
{
/*
* [snip]
* Each of these functions returns 0 on success, so evaluation will stop
* after the first successful method.
*/
if (
#ifdef HAVE_BSD_KERN_PROC_SYSCTL
git_get_exec_path_bsd_sysctl(buf) &&
#endif /* HAVE_BSD_KERN_PROC_SYSCTL */
#ifdef HAVE_NS_GET_EXECUTABLE_PATH
git_get_exec_path_darwin(buf) &&
#endif /* HAVE_NS_GET_EXECUTABLE_PATH */
#ifdef PROCFS_EXECUTABLE_PATH
git_get_exec_path_procfs(buf) && /*** <- OK if fails ***/
#endif /* PROCFS_EXECUTABLE_PATH */
#ifdef HAVE_WPGMPTR
git_get_exec_path_wpgmptr(buf) &&
#endif /* HAVE_WPGMPTR */
git_get_exec_path_from_argv0(buf, argv0)) {
return -1;
}
[snip]
#ifdef PROCFS_EXECUTABLE_PATH
/*
* Resolves the executable path by examining a procfs symlink.
*
* Returns 0 on success, -1 on failure.
*/
static int git_get_exec_path_procfs(struct strbuf *buf)
{
if (strbuf_realpath(buf, PROCFS_EXECUTABLE_PATH, 0)) {
trace_printf(
"trace: resolved executable path from procfs: %s\n",
buf->buf);
return 0;
}
return -1;
}
#endif /* PROCFS_EXECUTABLE_PATH */
So it seems this other procfs bit takes the "probably but not
definitely" step and is tolerant at runtime as well. Which doesn't help
me much to decide how to rename HAVE_PROCFS_LINUX.
I'll switch it to MAYBE_PROCFS_LINUX for v3 unless someone yells, I
guess.
- Emily
