Re: [RFC PATCH v2 00/22] Add blame to libgit
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 16, 2017, 00:23 UTC
- Message-ID
- <xmqqefvp6458.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <CAPX7N=4jidx5pr3BfFoTL27gbHDT6sdm1q2Z2YxfjsBuN01XaQ@mail.gmail.com>
Jeffrey Smith <whydoubt@gmail.com> writes:
Show 23 quoted lines
> On Mon, May 15, 2017 at 4:24 AM, Junio C Hamano <gitster@pobox.com> wrote: >> Jeff Smith <whydoubt@gmail.com> writes: >> >>> Rather than duplicate large portions of builtin/blame.c in cgit, it >>> would be better to shift its core functionality into libgit.a. The >>> functionality left in builtin/blame.c mostly relates to terminal >>> presentation. >> >> As I said in my review of 04/22, it is a bit hard/tedious to sift >> the changes to refactoring that actually changes code and pure >> renaming and movement of lines across files with the current >> sequence of the series, so it is very possible that I may have >> missed something, but from a cursory read through the series, from >> the comparison between master and the result of applying all >> patches, and inspection of the resulting builtin/blame.c file, I >> think what this series does is very sensible in general. blame.h >> does not seem to expose anything that is not needed, which is a good >> sign. >> > I did try to keep any unnecessary changes out of the big movement > patches (17-19). Would you prefer I break down 19 further? I had > been holding back because of the added churn that would introduce from > lots of changing function visibility in blame.h and blame.c.
To be honest, the "rename symbols while moving" done starting around 04/22 made the series too noisy to read and I had hard time reviewing the later ones like 17-19. I think they share exactly the same issue as 04/22, e.g. 17/22 renames origin_decref to blame_origin_decref while moving the function and some of its callers across files.
Show 5 quoted lines
> > (And if gmail would quit trying to make my messages 'rich text' -- > a.k.a. HTML -- that would be great...) > >