git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Emily Shaffer <emilyshaffer@google.com>
Subject: "I don't know what the author meant by that..." (was "Re: [PATCH 6/6] tr2: log N parent process names on Linux")
Date: Thu, 26 Aug 2021 14:24:10 +0200	[thread overview]
Message-ID: <87k0k8z7er.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YScTaDcPTs1nrP2Y@nand.local>


On Thu, Aug 26 2021, Taylor Blau wrote:

> On Thu, Aug 26, 2021 at 01:19:24AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> In 2f732bf15e6 (tr2: log parent process name, 2021-07-21) we started
>> logging parent process names, but only logged all parents on Windows.
>> on Linux only the name of the immediate parent process was logged.
>>
>> Extend the functionality added there to also log full parent chain on
>> Linux. In 2f732bf15e6 it was claimed that "further ancestry info can
>> be gathered with procfs, but it's unwieldy to do so.".
>>
>> I don't know what the author meant by that, but I think it probably
>> referred to needing to slurp this up from the FS, as opposed to having
>> an API.
>
> I don't think that this (specifically, "I don't know what the author
> meant by that") is necessary information to include in a patch message.
>
> If you're looking for a replacement (and you may not be, but just my
> $.02) I would suggest:
>
>     "2f732bf15e6 does not log the full parent chain on Linux; implement
>     that functionality here."

I hope Taylor doesn't mind me quoting it, but he sent me this follow-up
off-list:

    For what it's worth, I really struggled to write this. What I was trying
    to say was something along the lines of "let's give Emily a little more
    credit for not doing this right off the bat", but I didn't want to write
    that exactly, since I don't think it was your original intention.
    
    At the very least, saying something to the effect of "I don't know what
    the original author thought was so tough, here's a patch to implement
    it" isn't helping anybody, so I tried to focus on that.
    
    Anyway, just writing to you off-list to say that I do think you had good
    intentions, but that what you wrote may be read differently by others in
    a way that you didn't intend it to be.

First, thanks to Taylor for pointing this out, and second I'd like to
apologize for that comment.

More than some "sorry someone read it that way", this really does read
even to me, its author, shortly after having written it like something
that's way more on the side of sneakiness than a charitable comment.

I.e. like some snipe-y paraphrasing of "maybe they found this problem
too complex, but look how easy it is!" than something charitable that's
meant to barely pass under the radar.

Hopefully it helps that I'm honestly just being a bit of an
inconsiderate idiot here than actively malicious.

What I meant to accomplish here was to guide a reader of these patches
through the same mental states I went through when reading the original
patch.

I.e. I took it from its description that there were some unstated
special-cases in reading procfs that made it harder to deal with than
not. I think those comments were rather just a way to say that scraping
procfs was a bit of a pain, and the MVP in 2f732bf15e6 was good enough
for now, which is fair enough.

I rephrased those comments in v2 of this patch[1]. The summary starting
at the 4th paragraph ("It's possible given the semantics[...]") is an
edge case I hadn't considered when I wrote v1. I in turn copied that
"unweildy" comment from an even older version[2], where the difficulties
of parsing the "comm" field out of "/proc/*/stat" weren't apparent to
me.

I.e. I think if I had to describe that interface now I would describe it
as unwieldy, but wouldn't when I wrote v1[1] or that v0[2]. I.e. I think
there's no convenient way to get full atomic snapshot of the process
tree, so "give me the names of all my parents as an array" is definitely
somewhere between brittle and unwieldy, in particular considering the
hassle of parsing "/proc/*/stat" properly.

But I digress, which is also a problem I have with being overly verbose
sometimes and weaving enough rope to hang myself with.

The point isn't the difficulties of the procfs(5) interface, but that
particularly with a medium like the text-based communication we mainly
use in this project it behooves the sender to think not only about what
they're saying, but how what they're saying is going to be interpreted
by the recipient and bystanders alike.

I think this is a case where I clearly failed at that, sorry Emily, and
thanks Taylor for pointing this out to me.

1. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20210826T121820Z-avarab@gmail.com/
2. https://lore.kernel.org/git/87o8agp29o.fsf@evledraar.gmail.com/

  reply	other threads:[~2021-08-26 13:01 UTC|newest]

Thread overview: 87+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-07  0:29 [PATCH] tr2: log parent process name Emily Shaffer
2021-05-07  3:25 ` Bagas Sanjaya
2021-05-07 17:09 ` Emily Shaffer
2021-05-10 12:29 ` Ævar Arnfjörð Bjarmason
2021-05-11 21:31   ` Junio C Hamano
2021-05-14 22:06   ` Emily Shaffer
2021-05-16  3:48     ` Junio C Hamano
2021-05-17 20:17       ` Emily Shaffer
2021-05-11 17:28 ` Jeff Hostetler
2021-05-14 22:07   ` Emily Shaffer
2021-05-20 21:05 ` [PATCH v2] " Emily Shaffer
2021-05-20 21:36   ` Randall S. Becker
2021-05-20 23:23     ` Emily Shaffer
2021-05-21 13:20       ` Randall S. Becker
2021-05-21 16:24         ` Randall S. Becker
2021-05-21  2:09   ` Junio C Hamano
2021-05-21 19:02     ` Emily Shaffer
2021-05-21 23:22       ` Junio C Hamano
2021-05-24 18:37         ` Emily Shaffer
2021-05-21 19:15   ` Jeff Hostetler
2021-05-21 20:05     ` Emily Shaffer
2021-05-21 20:23       ` Randall S. Becker
2021-05-22 11:18       ` Jeff Hostetler
2021-05-24 23:33       ` Ævar Arnfjörð Bjarmason
2021-05-24 20:10   ` [PATCH v3] " Emily Shaffer
2021-05-24 20:49     ` Emily Shaffer
2021-05-25  3:54     ` Junio C Hamano
2021-05-25 13:33       ` Randall S. Becker
2021-06-08 18:58     ` [PATCH v4] " Emily Shaffer
2021-06-08 20:56       ` Emily Shaffer
2021-06-08 22:10       ` [PATCH v5] " Emily Shaffer
2021-06-08 22:16         ` Randall S. Becker
2021-06-08 22:24           ` Emily Shaffer
2021-06-08 22:39             ` Randall S. Becker
2021-06-09 20:17               ` Emily Shaffer
2021-06-16  8:42         ` Junio C Hamano
2021-06-28 16:45         ` Jeff Hostetler
2021-06-29 23:51           ` Emily Shaffer
2021-06-30  6:10             ` Ævar Arnfjörð Bjarmason
2021-07-22  0:21               ` Emily Shaffer
2021-07-22  1:27         ` [PATCH v6 0/2] " Emily Shaffer
2021-07-22  1:27           ` [PATCH v6 1/2] tr2: make process info collection platform-generic Emily Shaffer
2021-08-02  9:34             ` Ævar Arnfjörð Bjarmason
2021-07-22  1:27           ` [PATCH v6 2/2] tr2: log parent process name Emily Shaffer
2021-07-22 21:02             ` Junio C Hamano
2021-08-02  9:38             ` Ævar Arnfjörð Bjarmason
2021-08-02 12:45               ` Ævar Arnfjörð Bjarmason
2021-08-02 10:22             ` Ævar Arnfjörð Bjarmason
2021-08-02 12:47               ` Ævar Arnfjörð Bjarmason
2021-08-02 15:23               ` Jeff Hostetler
2021-08-02 16:10               ` Randall S. Becker
2021-08-02 18:41                 ` Ævar Arnfjörð Bjarmason
2021-08-25 23:19               ` [PATCH 0/6] tr2: plug memory leaks + logic errors + Win32 & Linux feature parity Ævar Arnfjörð Bjarmason
2021-08-25 23:19                 ` [PATCH 1/6] tr2: remove NEEDSWORK comment for "non-procfs" implementations Ævar Arnfjörð Bjarmason
2021-08-25 23:19                 ` [PATCH 2/6] tr2: clarify TRACE2_PROCESS_INFO_EXIT comment under Linux Ævar Arnfjörð Bjarmason
2021-08-25 23:19                 ` [PATCH 3/6] tr2: stop leaking "thread_name" memory Ævar Arnfjörð Bjarmason
2021-08-26  3:09                   ` Taylor Blau
2021-08-25 23:19                 ` [PATCH 4/6] tr2: fix memory leak & logic error in 2f732bf15e6 Ævar Arnfjörð Bjarmason
2021-08-26  3:21                   ` Taylor Blau
2021-08-25 23:19                 ` [PATCH 5/6] tr2: do compiler enum check in trace2_collect_process_info() Ævar Arnfjörð Bjarmason
2021-08-26  3:23                   ` Taylor Blau
2021-08-25 23:19                 ` [PATCH 6/6] tr2: log N parent process names on Linux Ævar Arnfjörð Bjarmason
2021-08-25 23:49                   ` Eric Sunshine
2021-08-26  4:07                   ` Taylor Blau
2021-08-26 12:24                     ` Ævar Arnfjörð Bjarmason [this message]
2021-08-26 12:22                 ` [PATCH v2 0/6] tr2: plug memory leaks + logic errors + Win32 & Linux feature parity Ævar Arnfjörð Bjarmason
2021-08-26 12:22                   ` [PATCH v2 1/6] tr2: remove NEEDSWORK comment for "non-procfs" implementations Ævar Arnfjörð Bjarmason
2021-08-26 12:22                   ` [PATCH v2 2/6] tr2: clarify TRACE2_PROCESS_INFO_EXIT comment under Linux Ævar Arnfjörð Bjarmason
2021-08-26 12:22                   ` [PATCH v2 3/6] tr2: stop leaking "thread_name" memory Ævar Arnfjörð Bjarmason
2021-08-26 12:22                   ` [PATCH v2 4/6] tr2: fix memory leak & logic error in 2f732bf15e6 Ævar Arnfjörð Bjarmason
2021-08-26 15:58                     ` Eric Sunshine
2021-08-26 16:42                     ` Junio C Hamano
2021-08-26 12:22                   ` [PATCH v2 5/6] tr2: do compiler enum check in trace2_collect_process_info() Ævar Arnfjörð Bjarmason
2021-08-26 12:22                   ` [PATCH v2 6/6] tr2: log N parent process names on Linux Ævar Arnfjörð Bjarmason
2021-08-26 22:38                   ` [PATCH v2 0/6] tr2: plug memory leaks + logic errors + Win32 & Linux feature parity Taylor Blau
2021-08-27  8:02                   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2021-08-27  8:02                     ` [PATCH v3 1/6] tr2: remove NEEDSWORK comment for "non-procfs" implementations Ævar Arnfjörð Bjarmason
2021-08-27  8:02                     ` [PATCH v3 2/6] tr2: clarify TRACE2_PROCESS_INFO_EXIT comment under Linux Ævar Arnfjörð Bjarmason
2021-08-27  8:02                     ` [PATCH v3 3/6] tr2: stop leaking "thread_name" memory Ævar Arnfjörð Bjarmason
2021-08-27  8:02                     ` [PATCH v3 4/6] tr2: leave the parent list empty upon failure & don't leak memory Ævar Arnfjörð Bjarmason
2021-08-27  8:02                     ` [PATCH v3 5/6] tr2: do compiler enum check in trace2_collect_process_info() Ævar Arnfjörð Bjarmason
2021-08-27  8:02                     ` [PATCH v3 6/6] tr2: log N parent process names on Linux Ævar Arnfjörð Bjarmason
2021-08-31  0:17                     ` [PATCH v3 0/6] tr2: plug memory leaks + logic errors + Win32 & Linux feature parity Taylor Blau
2021-08-02 10:30             ` [PATCH v6 2/2] tr2: log parent process name Ævar Arnfjörð Bjarmason
2021-08-02 16:24               ` Junio C Hamano
2021-08-02 18:42                 ` Ævar Arnfjörð Bjarmason
2021-07-22 16:59           ` [PATCH v6 0/2] " Jeff Hostetler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87k0k8z7er.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).