Re: read_branches_file ()

From: H.Merijn Brand <h.m.brand@xs4all.nl>
Date: Feb 8, 2008, 16:49 UTC
Message-ID: <20080208174924.24203ff3@pc09.procura.nl>
In-Reply-To: <alpine.LSU.1.00.0802081618210.11591@racer.site>

On Fri, 8 Feb 2008 16:22:45 +0000 (GMT), Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 14 quoted lines

> Hi,
> 
> On Fri, 8 Feb 2008, H.Merijn Brand wrote:
> 
> > --8<--- remote.c.diff
> > --- remote.c.org        2008-01-27 09:04:18 +0100
> > +++ remote.c    2008-02-08 17:01:09 +0100
> > @@ -1,6 +1,7 @@
> >  #include "cache.h"
> >  #include "remote.h"
> >  #include "refs.h"
> > +#include <sys/stat.h>
> 
> This should not be necessary; we include all system headers in cache.h.

dropped

Show 9 quoted lines

> > @@ -173,11 +174,15 @@ static void read_branches_file(struct re
> >         char *frag;
> >         char *branch;
> >         int n = slash ? slash - remote->name : 1000;
> > -       FILE *f = fopen(git_path("branches/%.*s", n, remote->name), "r");
> > +       char *gp = git_path ("branches/%.*s", n, remote->name);
> 
> Please use a more descriptive variable name, such as "branches_file" or 
> "branches_path".

took another approach, as that also addresses the other fopen () call

> Also, we only leave a space after operators like "for", "while", but not 
> after function names.

This patch was not sent to be applied as-is, only as a proof-of-concept Not that I agree to the layout/indentation, the new patch is sent trying to follow what you use.

> > +       if (stat (gp, &st_buf) || S_ISDIR (st_buf.st_mode))
> 
> Again, please remove the spaces after "stat" and "S_ISDIR".

you said please :)

> Other than that, the patch looks obviously correct: please resubmit with a 
> nice commit message and a sign-off.

Done

-- 
H.Merijn Brand         Amsterdam Perl Mongers (http://amsterdam.pm.org/)
using & porting perl 5.6.2, 5.8.x, 5.10.x  on HP-UX 10.20, 11.00, 11.11,
& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin.       http://qa.perl.org
http://mirrors.develooper.com/hpux/            http://www.test-smoke.org
                        http://www.goldmark.org/jeff/stupid-disclaimers/