Re: [PATCH 01/14] trace2: Documentation/technical/api-trace2.txt

[Junio C Hamano <gitster@pobox.com>]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +These high-level events are written to one or more Trace2 Targets
> +in a target-specific format.  Each Trace2 Target defines a different
> +purpose-specific view onto the event data stream.  In this mannor,

"In this manner"

> +a single set of Trace2 API event calls in the Git source can drive
> +different types of analysis.
> +
> ...
> +$ cat ~/log.perf
> +12:28:42.620675 common-main.c:38                  | d0 | main                     | version      |     |           |           |            | 2.20.1.155.g426c96fcdb
> +12:28:42.621001 common-main.c:39                  | d0 | main                     | start        |     |           |           |            | git version
> +12:28:42.621111 git.c:432                         | d0 | main                     | cmd_verb     |     |           |           |            | version (version)
> +12:28:42.621225 git.c:662                         | d0 | main                     | exit         |     |  0.001227 |           |            | code:0
> +12:28:42.621259 trace2/tr2_tgt_perf.c:211         | d0 | main                     | atexit       |     |  0.001265 |           |            | code:0
> +------------
> ...
> +
> +trace2_def_param(...)

Not limited to this single one, but please either

 - omit "..." in parens, unless all of these functions take varargs
   of unspecified type (which I do not think is the case), or

 - write a proper prototype for these functions, explain what the
   function itself and what the parameters are for.

I'll complain about lack of info around here later X-<.

> +trace2_def_repo(...)

> +----------------
> +
> +Git Child Process Events::
> +
> +	These are concerned with the various spawned child processes,
> +	including sub-git processes and hooks,
> ++
> +----------------
> +trace2_child_start(...)
> +trace2_child_exit(...)
> +
> +trace2_exec(...)
> +trace2_exec_result(...)
> +----------------
> +
> +Git Thread Events::
> +
> +	These are concerned with Git thread usage.
> ++
> +----------------
> +trace2_thread_start(...)
> +trace2_thread_exit(...)
> +----------------

Lack of _wait()/_join() feels a bit curious, but _exit() from what
is being waited would suffice as a substitute.

> +Initialization::
> +
> +	Initialization happens in `main()`.  The initialization code
> +	determines which, if any, Trace2 Targets should be enabled and
> +	emits the `version`, `start`, and `exit` events.  It causes an
> +	`atexit` function and `signal` handler to be registered that
> +	will emit `atexit` and `signal` events.
> ++
> +----------------
> +int main(int argc, const char **argv)
> +{
> +	int exit_code;
> +
> +	trace2_initialize();
> +	trace2_cmd_start(argv);

Nobody other than trace2 integration would make a call to this
helper, so it may not matter, but sending av alone without ac, even
though ac is almost always redundant, feels somewhat unexpected.

> +Command Details::
> +
> +	After the basics are established, additional process
> +	information can be sent to Trace2 as it is discovered, such as
> +	the command verb, alias expansion, interesting config
> +	settings, the repository worktree, error messages, and etc.
> ++
> +----------------
> +int cmd_checkout(int argc, const char **argv)
> +{
> +	// emit a single "def_param" event
> +	trace2_def_param("core.editor", "emacs");

Without knowing what "def_param event" is, this example is hard to
fathom.  At this point in the doc, the reader does not even know
what "def" stands for.  Is this call to define a param called
core.editor?  Is it reporting that the default value for core.editor
is emacs?

> +	// emit def_params for any config setting matching a pattern
> +	// in GIT_TR2_CONFIG_PARAMS.
> +	trace2_cmd_list_config();

As the reader does not know what def_param is, this is also hard to
follow.

> +	trace2_cmd_verb("checkout");
> +	trace2_cmd_subverb("branch");

These are guessable.  It probably reports what the codepath is
doing.

> +	trace2_def_repo(the_repository);

Again, this is not easy to guess.  Is it reporting what the default
repository is?

> +	if (do_something(...))
> +	    trace2_cmd_error("Path '%s': cannot do something", path);

This is guessable, which is good.

> +void run_child(...)
> +{
> +	int child_exit_code;
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> +	...
> +
> +	trace2_child_start(&cmd);
> +	child_exit_code = ...();
> +	trace2_child_exit(&cmd, child_exit_code);

Even though child_exit() has not been explained just like
def_param(), this is far more guessable.  I wonder why it is.  The
name of the variable passed to it in the example certainly helps, as
it is totally unclear where the string constant "emacs" in the
earlier example came from (e.g. is it hardcoded in the program?  is
it merely illustrating "here is how you report the value you have
decided to use for 'core.editor' variable"?).

> ...
> +$ cat ~/log.perf
> +d0 | main                     | version      |     |           |           |            | 2.20.1.160.g5676107ecd.dirty
> +d0 | main                     | start        |     |           |           |            | git status
> +d0 | main                     | def_repo     | r1  |           |           |            | worktree:/Users/jeffhost/work/gfw
> +d0 | main                     | cmd_verb     |     |           |           |            | status (status)
> +...
> +d0 | main                     | region_enter | r1  |  0.010988 |           | status     | label:worktrees

It is hard to guess what "d0" and "r1" stand for here.  

In an earlier example we also saw an unexplained "d0" there, which I
think was OK because the illustration was merely to give the "feel"
of the format up there.  But now we are giving a bit more detailed
explanation, we probably would want to at least briefly mention what
each column means.

I'd stop here for now, as I am more interested in reading the code
;-)