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@xxxxxxxxx> wrote: > pi song <pi.songs@xxxxxxxxx> 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@xxxxxxxxx> >> --- >> 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. > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
