On Wed, Sep 19, 2007 at 06:37:31AM +0000, Andreas Ericsson wrote:
> Pierre Habouzit wrote:
> > diff --git a/builtin-blame.c b/builtin-blame.c
> >index e364b6c..16c0ca8 100644
> >--- a/builtin-blame.c
> >+++ b/builtin-blame.c
> >@@ -1430,8 +1430,7 @@ static void get_commit_info(struct commit *commit,
> > static void write_filename_info(const char *path)
> > {
> > 	printf("filename ");
> >-	write_name_quoted(NULL, 0, path, 1, stdout);
> >-	putchar('\n');
> >+	write_name_quoted(path, stdout, '\n');
> > }
> > 
> 
> This looks like a candidate for a macro. I'm not sure if gcc optimizes
> sibling calls in void functions with -O2, and it doesn't inline without
> -O3.

  Well, there is little point. write_name_quoted behaviour changes if
the last argument is \0 or non-\0 (see patch comment and quote.c code),
so it does not really matter to inline the "putchar" IMHO.

> > -static void diff_flush_raw(struct diff_filepair *p,
> >-			   struct diff_options *options)
> >+static void diff_flush_raw(struct diff_filepair *p, struct diff_options 
> >*opt)
> 
> Parameter rename? I'd have thought the patch was big enough as it is ;-)

  I'm anal when it comes to code: the rule of the least surprise should
apply, and consistency is fundamental. And it happens that diff_options
are always called `opt' in diff.c, except in that place (and it allows
to write the prototype of the function on one line).

> Other than that, the diffstat calls this a good patch, and given the
> fact that all your previous series passed all tests, I assume this one
> does too.

  Yes, before submitting a series I check the testsuite passes at each
step, so that it doesn't break git-bisect in obvious ways.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org