Web lists-archives.com

Re: [PATCH v3 00/10] trace2: load trace2 settings from system config






On 4/11/2019 10:29 PM, Junio C Hamano wrote:
"Jeff Hostetler via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:

[...]
      @@ -512,19 +454,28 @@
       + */
       +/* clang-format off */

      ++	[TR2_SYSENV_CFG_PARAM]     = { "GIT_TR2_CONFIG_PARAMS",
      ++				       "trace2.configparams" },
      ++
      ++	[TR2_SYSENV_DST_DEBUG]     = { "GIT_TR2_DST_DEBUG",
      ++				       "trace2.destinationdebug" },
      ++
      ++	[TR2_SYSENV_NORMAL]        = { "GIT_TR2",
      ++				       "trace2.normaltarget" },
      ++	[TR2_SYSENV_NORMAL_BRIEF]  = { "GIT_TR2_BRIEF",
      ++				       "trace2.normalbrief" },
      ++
      ++	[TR2_SYSENV_EVENT]         = { "GIT_TR2_EVENT",
      ++				       "trace2.eventtarget" },
      ++	[TR2_SYSENV_EVENT_BRIEF]   = { "GIT_TR2_EVENT_BRIEF",
      ++				       "trace2.eventbrief" },
      ++	[TR2_SYSENV_EVENT_NESTING] = { "GIT_TR2_EVENT_NESTING",
      ++				       "trace2.eventnesting" },
      ++
      ++	[TR2_SYSENV_PERF]          = { "GIT_TR2_PERF",
      ++				       "trace2.perftarget" },
      ++	[TR2_SYSENV_PERF_BRIEF]    = { "GIT_TR2_PERF_BRIEF",
      ++				       "trace2.perfbrief" },


With use of designated initializers, the table got a lot cleaner to
read.  Is the above "format off" still needed (I am a bit curious
how clang-format wants these entries to look like)?

clang-format suggests getting rid of the extra whitespace on the
lines, so we lose all of the column alignment.  Then it wants to
line-wrap some but not all of the lines.  So it is a bit of a mess
to look at.



      ++	if (pid > 999999)
      ++		strbuf_addf(&tr2sid_buf, "W%06d", (int)(pid % 1000000));
      ++	else
      ++		strbuf_addf(&tr2sid_buf, "P%06d", (int)pid);

I do not think it matters too much, but this is kind-of curious.

How would the users of the log utilize the distinction between W and
P?  Do they discard the ones with W when they care about the exact
process that left the trace entries, or something?  If it's not a
plausibly useful use pattern (and I do not think it is), I wonder if
we want to go with only W (i.e. truncated to the lower N digits)
entries, if you are shooting for a fixed-width output from this
function.  If you want less chance of collisions, you obviously
could use hexadecimal to gain back a few more bits.

After all, if the application does care the PID, that could be in
the log data itself (i.e. an "start" event can say "my pid is blah").

Right. Ævar suggested adding the full or wrapped PID so that the SID
would be a fixed length.  I stuck with decimal rather than hex because
it's easier to match up a running command with '/usr/bin/ps' output,
but that's no big deal either way.  It might be simpler to just %08lx
it and be done with it.

Jeff