Re: [PATCH] upload-pack: add tracing for fetches

From: Robert Coup <robert@coup.net.nz>
Date: Oct 11, 2023, 22:33 UTC
Message-ID: <CACf-nVfMLRy6DGbX+G+eJ-LFNEywTmJkpdem8no=TTziLOV+Jw@mail.gmail.com>
In-Reply-To: <ZScUWrj6CQTg05HL@nand.local>

Hi Taylor,

Thanks for taking a look.

On Wed, 11 Oct 2023 at 22:32, Taylor Blau <me@ttaylorr.com> wrote:

>
> One of the custom patches that GitHub is carrying in its private
> fork is something similar to this that dumps information from
> upload-pack into a custom logging system specific to GitHub

I suspected as much. My initial goal is clones vs fetches, filter use, and shallow use... since I'm changing code is there anything else that you think is worthwhile instrumenting? I thought about casting the ints to booleans (ie: >0 haves; >0 wants; etc) but I figured the actual numbers might come in handy at some point, and changing them back in future would be incompatible. I deliberately didn't add thin/ofs/etc flags: most of that stuff is on by default now so it felt like noise.

Show 6 quoted lines

> > + jw_object_begin(&jw, 0);
> > + {
>
> Is there a reason that we have a separate scope here? I think we may
> want to drop this as unnecessary, but it's entirely possible that I'm
> missing something here...

I just followed the patterns in test-json-writer.c - personally I think since JSON objects/arrays have starts & ends then it's easy to read. But one person's readable is another's coding style violation ;-)

Show 5 quoted lines

> > + jw_object_string(&jw, "filter", list_object_filter_config_name(data->filter_options.choice));
>
> I'm pretty sure that list_object_filter_config_name() returns characters
> that are safe for JSON-encoding, and/or that jw_object_string() does any
> quoting beforehand, but worth checking nonetheless.

Yes, jw_object_string() calls append_quoted_string() which deals with escaping and quoting.

Thanks,

Rob :)