Re: [RFC PATCH v2 00/22] Add blame to libgit

From: Junio C Hamano <gitster@pobox.com>
Date: May 16, 2017, 03:21 UTC
Message-ID: <xmqqd1b94hc1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPX7N=5=M=wjTbKQoctndqrHxYSb86bEwej8ofoOovsYHWFkKA@mail.gmail.com>

Jeffrey Smith <whydoubt@gmail.com> writes:

Show 22 quoted lines

> On Mon, May 15, 2017 at 7:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jeffrey Smith <whydoubt@gmail.com> writes:
>>
>>> 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.
>
> Hm, I really thought I had split some of those out.  Patches 4, 5, 17, and 19
> could all be reasonably split into rename vs move patches.  Patch 18 only
> does moving.
>
> I do not see much benefit to splitting up the 'move xyz to scoreboard'
> subset (6-12) that way as moving an item to scoreboard is already causing
> each use to change (from xyz to sb->xyz).

Perhaps it would be better to repeat and clarify what I suggested once more.

 * Keep mechanical changes separate from real changes,
 * Keep each real change separate from each other,
 * Keep the same kind of mechnical changes together, and
 * Keep different kinds of mechanical changes separate.

So, if you are moving some globals into a structure, we want a patch that does that for a set of related globals and adjust their uses, and do nothing else. You'll probably have a handful of them in the end. For each of these real changes, it is usually better to have more and smaller patches than to have fewer and larger patches.

Then if you are remaing a bunch of structure types and/or variable names, we want a patch that does that mechanical change (i.e. rename "struct foo" and "struct bar" to "struct blame_foo" and "struct blame_bar") that does not do anything else (i.e. do not move lines across files in the same patch). As I said already, this does not have to be one per struct type. We can say "renaming symbols" is just one kind of mechanical changes, which is different from "moving lines across files".

Finally, move the lines across files (and again, do nothing else). Having to turn "static something" into "extern something" is part of moving the lines across files, and we do want to see that in the same patch. But renaming something to blame_something should have already be done while these lines were in builtin/blame.c in an earlier step.

Thanks.