Re: [PATCH RFC 0/20] cat-file: start using formatting logic from ref-filter

From: Jeff King <peff@peff.net>
Date: Feb 28, 2019, 21:41 UTC
Message-ID: <20190228214112.GK12723@sigill.intra.peff.net>
In-Reply-To: <CAL21BmnVkKtYWa1cRL1EJAwtchGcVUzhu0136AuV8uXAi5Kuew@mail.gmail.com>

On Fri, Feb 22, 2019 at 06:50:06PM +0300, Olga Telezhnaya wrote:

> It was a long way for me, I got older (by 1 year) and smarter
> (hopefully), and maybe I will finish my Outreachy Internship task for
> now. (I am doing it just for one year and a half, that's OK)

Welcome back!

Sorry to be a bit slow on the review. I've read through and commented on patch 10. Some of my comments were "I'll have to see how this plays out later in the series", so you may want to hold off on responding until I read the rest. :)

Show 7 quoted lines

> If serious:
> In this patch we remove cat-file formatting logic and reuse ref-filter
> logic there. As a positive side effect, cat-file now has many new
> formatting tokens (all from ref-filter formatting), including deref
> (like %(*objectsize:disk)). I have already tried to do this task one
> year ago, and it was bad attempt. I feel that today's patch is much
> better.

I'm still concerned that this is going to regress the performance of cat-file noticeably without some big cleanups in ref-filter. Here are timings on linux.git before and after your patches:

  [before]
  $ time git cat-file --unordered --batch-all-objects --batch-check >/dev/null
  real	0m16.602s
  user	0m15.545s
  sys	0m0.495s

  [after]
  $ time git cat-file --unordered --batch-all-objects --batch-check >/dev/null
  real	0m27.301s
  user	0m24.549s
  sys	0m2.752s

I don't think that's anything particularly wrong with your patches. It's the existing strategy of ref-filter (in particular how it is very eager to allocate lots of separate strings). And it may be too early to switch cat-file over to it.

Show 5 quoted lines

> I also have a question about site https://git-scm.com/docs/
> I thought it is updated automatically based on Documentation folder in
> the project, but it is not true. I edited docs for for-each-ref in
> December, I still see my patch in master, but for-each-ref docs in
> git-csm is outdated. Is it OK?

Yeah, as Eric noted, we only build docs for the tagged releases. In theory it would be easy to just build the tip of master nightly, but the data model for the site would need quite a bit of adjustment.

-Peff