From: pi song <pi.songs@gmail.com>
Date: Tue, 10 Mar 2009 11:29:49 GMT
Subject: Re: [PATCH 1/2] grep Added --blame so that grep can show result tagged with blame entries
Message-ID: <1b29507a0903100429v5b4a66d7sa9e3b32010fae2e0@mail.gmail.com>
In-Reply-To: <7v7i2xaewh.fsf@gitster.siamese.dyndns.org>

OK, I'll move my work to "next" branch. BTW, can anyone tell me what
the "next" and "pu" branch are for ?

Pi Song

On Tue, Mar 10, 2009 at 8:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> pi song <pi.songs@gmail.com> writes:
>
>> This part:-
>> 1) Move reusable bits from builtin-blame.c into blame.c, blame.h
>> 2) Replace static variables with context struct
>>
>> Signed-off-by: Pi Song <pi.songs@gmail.com>
>> ---
>> Makefile        |    2 +
>> blame.c         | 1919 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> blame.h         |  222 ++++++
>> builtin-blame.c | 2072
>> +++----------------------------------------------------
>> 4 files changed, 2245 insertions(+), 1970 deletions(-)
>> create mode 100644 blame.c
>> create mode 100644 blame.h
>
> This is simply too big to ask anybody sane to review (besides as we can
> clearly see in the above the patch is severely whitespace damaged to be
> usable nor mechanically reviewable).
>
> I suspect that most of the file-scope static variables can be moved to the
> scoreboard, and when the reusable parts are made public, some structure
> and function names may need to become a bit more blame specific to avoid
> namespace contamination.
>
> Perhaps the first two patches in an equivalent series that is rerolled to
> be reviewable would look like:
>
>  (1) move file-scope static variables to the scoreboard, and necessary
>     code changes associated with it;
>
>     renaming of some structures and functions (if needed---I didn't
>     check);
>
>  (2) create blame.c and blame.h, and move lines from builtin-blame.c *and*
>     do NOTHING OTHER THAN
>
>     - adding #include "blame.h" to builtin-blame.c,
>
>     - adding necessary #include at the top of blame.c,
>
>     - surrounding blame.h with necessary
>
>         #ifndef BLAME_H
>         #define BLAME_H
>         ...
>         #endif
>
>       and finally
>
>     - updating Makefile to add blame.c and blame.h
>
>     This step will make a HUGE patch, and it is crucial for reviewability
>     for it not to do anything other than line movement.  Ideally, the
>     patch shouldn't even reorder the structures and function definitions
>     during this step.
>
>     Then we can use "git blame" itself to make sure that no other changes
>     were sneaked in by mistake.  "git blame -C blame.h" and "git blame -C
>     blame.c" would show everything came from builtin-blame.c.
>
> At this point, there shouldn't still be any behaviour change nor new
> feature.  And the titles of these preparatory step will never say anything
> about "grep".  They are only refactoring "blame".
>
> Once this becomes solid, you can start adding features to blame.c to
> support your new caller, and we can be reasonably sure that such patches
> can be reviewed to decide if its change breaks the existing (and so far
> the only) caller builtin_blame() or not.
>