git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff Hostetler <git@jeffhostetler.com>
To: Elijah Newren <newren@gmail.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Derrick Stolee <stolee@gmail.com>, Jeff King <peff@peff.net>,
	Philip Oakley <philipoakley@iee.email>,
	Jeff Hostetler <jeffhost@microsoft.com>,
	Josh Steadmon <steadmon@google.com>
Subject: Re: [PATCH v4 1/8] dir: convert trace calls to trace2 equivalents
Date: Wed, 12 May 2021 08:26:46 -0400	[thread overview]
Message-ID: <d9690dfc-2883-6f0a-e66b-89066b660c8d@jeffhostetler.com> (raw)
In-Reply-To: <CABPp-BF9ptJio+NSi-i-sQJqjsOiKfFxzVHQ=RTPQkV9hjPrMQ@mail.gmail.com>



On 5/11/21 8:44 PM, Elijah Newren wrote:
> On Tue, May 11, 2021 at 4:12 PM Jeff Hostetler <git@jeffhostetler.com> wrote:
>>
>> On 5/11/21 4:12 PM, Elijah Newren wrote:
>>> On Tue, May 11, 2021 at 12:06 PM Jeff Hostetler <git@jeffhostetler.com> wrote:
>>>>
>>>> On 5/11/21 2:34 PM, Elijah Newren via GitGitGadget wrote:
>>>>> From: Elijah Newren <newren@gmail.com>
>>>>>
>>>>> Signed-off-by: Elijah Newren <newren@gmail.com>
>>>>> ---
>>>>>     dir.c                             |  43 +++++--
>>>>>     t/t7063-status-untracked-cache.sh | 205 ++++++++++++++++++------------
>>>>>     t/t7519-status-fsmonitor.sh       |   8 +-
>>>>>     3 files changed, 155 insertions(+), 101 deletions(-)
>>>>>
>>>>> diff --git a/dir.c b/dir.c
>>>>> index 3474e67e8f3c..122fcbffdf89 100644
>>>>> --- a/dir.c
>>>>> +++ b/dir.c
>>>>> @@ -2760,15 +2760,34 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
>>>>>         return root;
>>>>>     }
>>>>>
>>>>> +static void trace2_read_directory_statistics(struct dir_struct *dir,
>>>>> +                                          struct repository *repo,
>>>>> +                                          const char *path)
>>>>> +{
>>>>> +     if (!dir->untracked)
>>>>> +             return;
>>>>> +     trace2_data_string("read_directory", repo, "path", path);
>>>>
>>>> I'm probably just nit-picking here, but should this look more like:
>>>
>>> nit-picking and questions are totally fine.  :-)  Thanks for reviewing.
>>>
>>>>
>>>>           if (path && *path)
>>>>                   trace2_data_string(...)
>>>
>>> path is always non-NULL (it'd be an error to call read_directory()
>>> with a NULL path).  So the first part of the check isn't meaningful
>>> for this particular code.  The second half is interesting.  Do we want
>>> to omit the path when it happens to be the toplevel directory (the
>>> case where !*path)?  The original trace_performance_leave() calls
>>> certainly didn't, and I was just trying to provide the same info they
>>> do, as you suggested.  I guess people could determine the path by
>>> knowing that the code doesn't print it when it's empty, but do we want
>>> trace2 users to need to read the code to figure out statistics and
>>> info?
>>
>> that's fine.  it might be easier to just always print it (even if
>> blank) so that post-processors know that rather than have to assume
>> it.
>>
>>>
>>>>           if (!dir->untracked)
>>>>                   return;
>>>>
>>>> Then when you add the visitied fields in the next commit,
>>>> you'll have the path with them (when present).
>>>
>>> There is always a path with them, it's just that the empty string
>>> denotes the toplevel directory.
>>>
>>>> (and it would let you optionally avoid the tmp strbuf in
>>>> the caller.)
>>>
>>> The path in read_directory() is not necessarily NUL-delimited, so
>>> attempting to use it as-is, or even with your checks, would cause us
>>> to possibly print garbage and do out-of-bounds reads.  We need the tmp
>>> strbuf.
>>>
>>
>> I just meant, "if (!len) pass NULL, else build and pass tmp.buf".
> 
> Ah, gotcha, that's why you were checking non-NULL.
> 
> However, what about the other case when len is nonzero.  Let's say
> that len = 8 and path points at
> "filename*%&#)aWholeBunchOfTotalGarbageAfterTheRealFilenameThatShouldNotBeReadOrIncluded\0\0\0\0\0\0\0\0\0\0"
> ?
> 
> How do you make it print "filename" and only "filename" without the
> other stuff without using the tmp strbuf?
> 

I was still saying to use the "strbuf tmp" in the non-zero len case,
but just pass NULL (or "") for the len==0 case.

Alternatively, since `trace2_read_directory_statistics() a static
local function, we could move all of the path manipulation into it.

static void emit_stats(
	struct dir_struct *dir,
	struct repository *repo,
	const char* path_buf,
	size_t path_len)
{
	if (!path_len)
		trace2_data_string("read_directory", repo,
			"path", "");
	else {
		struct strbuf tmp = STRBUF_INIT;
		strbuf_add(&tmp, path_buf, path_len);
		trace2_data_string("read_directory", repo,
			"path", tmp.buf);
		strbuf_release(&tmp);
	}
	... the rest of intmax stats ...
}


BTW, could we also rename your stats function?  I've been trying
to keep the "trace2_" prefix reserved for the Trace2 API.


Thanks,
Jeff

  reply	other threads:[~2021-05-12 12:26 UTC|newest]

Thread overview: 90+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-07  4:04 [PATCH 0/5] Directory traversal fixes Elijah Newren via GitGitGadget
2021-05-07  4:04 ` [PATCH 1/5] t7300: add testcase showing unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-07  4:27   ` Eric Sunshine
2021-05-07  5:00     ` Elijah Newren
2021-05-07  5:31       ` Eric Sunshine
2021-05-07  5:42         ` Elijah Newren
2021-05-07  5:56           ` Eric Sunshine
2021-05-07 23:05       ` Jeff King
2021-05-07 23:15         ` Eric Sunshine
2021-05-08  0:04         ` Elijah Newren
2021-05-08  0:10           ` Eric Sunshine
2021-05-08 17:20             ` Elijah Newren
2021-05-08 11:13   ` Philip Oakley
2021-05-08 17:20     ` Elijah Newren
2021-05-07  4:04 ` [PATCH 2/5] t3001, t7300: add testcase showcasing missed directory traversal Elijah Newren via GitGitGadget
2021-05-07  4:04 ` [PATCH 3/5] dir: avoid unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-07  4:04 ` [PATCH 4/5] dir: traverse into untracked directories if they may have ignored subfiles Elijah Newren via GitGitGadget
2021-05-07  4:05 ` [PATCH 5/5] [RFC] ls-files: error out on -i unless -o or -c are specified Elijah Newren via GitGitGadget
2021-05-07 16:22 ` [PATCH 6/5] dir: update stale description of treat_directory() Derrick Stolee
2021-05-07 17:57   ` Elijah Newren
2021-05-07 16:27 ` [PATCH 0/5] Directory traversal fixes Derrick Stolee
2021-05-08  0:08 ` [PATCH v2 0/8] " Elijah Newren via GitGitGadget
2021-05-08  0:08   ` [PATCH v2 1/8] t7300: add testcase showing unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-08 10:13     ` Junio C Hamano
2021-05-08 17:34       ` Elijah Newren
2021-05-08 10:19     ` Junio C Hamano
2021-05-08 17:41       ` Elijah Newren
2021-05-08  0:08   ` [PATCH v2 2/8] t3001, t7300: add testcase showcasing missed directory traversal Elijah Newren via GitGitGadget
2021-05-08  0:08   ` [PATCH v2 3/8] dir: avoid unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-08  0:08   ` [PATCH v2 4/8] dir: traverse into untracked directories if they may have ignored subfiles Elijah Newren via GitGitGadget
2021-05-08  0:08   ` [PATCH v2 5/8] [RFC] ls-files: error out on -i unless -o or -c are specified Elijah Newren via GitGitGadget
2021-05-08  0:08   ` [PATCH v2 6/8] dir: update stale description of treat_directory() Derrick Stolee via GitGitGadget
2021-05-08  0:08   ` [PATCH v2 7/8] [RFC] dir: convert trace calls to trace2 equivalents Elijah Newren via GitGitGadget
2021-05-08  0:08   ` [PATCH v2 8/8] [RFC] dir: reported number of visited directories and paths with trace2 Elijah Newren via GitGitGadget
2021-05-08 19:58   ` [PATCH v3 0/8] Directory traversal fixes Elijah Newren via GitGitGadget
2021-05-08 19:58     ` [PATCH v3 1/8] [RFC] dir: convert trace calls to trace2 equivalents Elijah Newren via GitGitGadget
2021-05-10  4:49       ` Junio C Hamano
2021-05-11 17:23         ` Elijah Newren
2021-05-11 16:17       ` Jeff Hostetler
2021-05-11 17:29         ` Elijah Newren
2021-05-08 19:58     ` [PATCH v3 2/8] [RFC] dir: report number of visited directories and paths with trace2 Elijah Newren via GitGitGadget
2021-05-10  5:00       ` Junio C Hamano
2021-05-08 19:58     ` [PATCH v3 3/8] [RFC] ls-files: error out on -i unless -o or -c are specified Elijah Newren via GitGitGadget
2021-05-10  5:09       ` Junio C Hamano
2021-05-11 17:40         ` Elijah Newren
2021-05-11 22:32           ` Junio C Hamano
2021-05-08 19:59     ` [PATCH v3 4/8] t7300: add testcase showing unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-10  5:28       ` Junio C Hamano
2021-05-11 17:45         ` Elijah Newren
2021-05-11 22:43           ` Junio C Hamano
2021-05-12  2:07             ` Elijah Newren
2021-05-12  3:17               ` Junio C Hamano
2021-05-08 19:59     ` [PATCH v3 5/8] t3001, t7300: add testcase showcasing missed directory traversal Elijah Newren via GitGitGadget
2021-05-08 19:59     ` [PATCH v3 6/8] dir: avoid unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-10  5:48       ` Junio C Hamano
2021-05-11 17:57         ` Elijah Newren
2021-05-08 19:59     ` [PATCH v3 7/8] dir: traverse into untracked directories if they may have ignored subfiles Elijah Newren via GitGitGadget
2021-05-08 19:59     ` [PATCH v3 8/8] dir: update stale description of treat_directory() Derrick Stolee via GitGitGadget
2021-05-11 18:34     ` [PATCH v4 0/8] Directory traversal fixes Elijah Newren via GitGitGadget
2021-05-11 18:34       ` [PATCH v4 1/8] dir: convert trace calls to trace2 equivalents Elijah Newren via GitGitGadget
2021-05-11 19:06         ` Jeff Hostetler
2021-05-11 20:12           ` Elijah Newren
2021-05-11 23:12             ` Jeff Hostetler
2021-05-12  0:44               ` Elijah Newren
2021-05-12 12:26                 ` Jeff Hostetler [this message]
2021-05-12 15:24                   ` Elijah Newren
2021-05-11 18:34       ` [PATCH v4 2/8] dir: report number of visited directories and paths with trace2 Elijah Newren via GitGitGadget
2021-05-11 18:34       ` [PATCH v4 3/8] ls-files: error out on -i unless -o or -c are specified Elijah Newren via GitGitGadget
2021-05-11 18:34       ` [PATCH v4 4/8] t7300: add testcase showing unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-11 18:34       ` [PATCH v4 5/8] t3001, t7300: add testcase showcasing missed directory traversal Elijah Newren via GitGitGadget
2021-05-11 18:34       ` [PATCH v4 6/8] dir: avoid unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-11 18:34       ` [PATCH v4 7/8] dir: traverse into untracked directories if they may have ignored subfiles Elijah Newren via GitGitGadget
2021-05-11 18:34       ` [PATCH v4 8/8] dir: update stale description of treat_directory() Derrick Stolee via GitGitGadget
2021-05-12 17:28       ` [PATCH v5 0/9] Directory traversal fixes Elijah Newren via GitGitGadget
2021-05-12 17:28         ` [PATCH v5 1/9] dir: convert trace calls to trace2 equivalents Elijah Newren via GitGitGadget
2021-05-12 17:28         ` [PATCH v5 2/9] dir: report number of visited directories and paths with trace2 Elijah Newren via GitGitGadget
2021-05-12 17:28         ` [PATCH v5 3/9] ls-files: error out on -i unless -o or -c are specified Elijah Newren via GitGitGadget
2021-05-12 17:28         ` [PATCH v5 4/9] t7300: add testcase showing unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-12 17:28         ` [PATCH v5 5/9] t3001, t7300: add testcase showcasing missed directory traversal Elijah Newren via GitGitGadget
2021-05-12 17:28         ` [PATCH v5 6/9] dir: avoid unnecessary traversal into ignored directory Elijah Newren via GitGitGadget
2021-05-12 17:28         ` [PATCH v5 7/9] dir: traverse into untracked directories if they may have ignored subfiles Elijah Newren via GitGitGadget
2021-05-12 17:28         ` [PATCH v5 8/9] dir: update stale description of treat_directory() Derrick Stolee via GitGitGadget
2021-05-17 17:20           ` Derrick Stolee
2021-05-17 19:44             ` Junio C Hamano
2021-05-18  3:32               ` Elijah Newren
2021-05-19  1:44                 ` Junio C Hamano
2021-05-12 17:28         ` [PATCH v5 9/9] dir: introduce readdir_skip_dot_and_dotdot() helper Elijah Newren via GitGitGadget
2021-05-17 17:22           ` Derrick Stolee
2021-05-18  3:34             ` Elijah Newren
2021-05-17 17:23         ` [PATCH v5 0/9] Directory traversal fixes Derrick Stolee

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

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

  git send-email \
    --in-reply-to=d9690dfc-2883-6f0a-e66b-89066b660c8d@jeffhostetler.com \
    --to=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jeffhost@microsoft.com \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    --cc=philipoakley@iee.email \
    --cc=steadmon@google.com \
    --cc=stolee@gmail.com \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH v4 1/8] dir: convert trace calls to trace2 equivalents' \
    /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

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 NNTP newsgroup(s).