Re: [PATCH] branch as a builtin (again)

From: Junio C Hamano <junkio@cox.net>
Date: Aug 22, 2006, 07:00 UTC
Message-ID: <7vveolqmpb.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <59ad55d30608211345k7956a219m8ab450c6f2023461@mail.gmail.com>

"Kristian Høgsberg" <krh@bitplanet.net> writes:

> Ok, once more without the spaces.  I have to state that it's against
> my personal beliefs using pointers as boolean values, but I can go
> with the flow here.  For extra bonus, I'm using xrealloc instead of
> plain realloc now.

My preferences (pretty much procedural):

 - Documentation/SubmittingPatches
   - Attachments discouraged;
   - With a proper commit log message;
   - With a proper signed-off line;

 - Names in source encoded in utf8 if needed (I think you got
   this one right, but application/octet-stream does not give
   charset information so I cannot really tell).

 - No spaces between a function name and open parenthesis.

Some nitpicks.

> static void create_reflog(struct ref_lock *lock)
> {
>...
> }

Probably reflog interface should supply ways to create new ones (and delete or truncate existing ones) to users like this program. Please work with Shawn Pearce to refactor this part.

Show 6 quoted lines

> static void create_branch(const char *name, const char *start,
> 			  int force, int reflog)
> {
> 	struct ref_lock *lock;
> 	unsigned char sha1[20];
> 	char ref[PATH_MAX], msg[PATH_MAX + 20];

You are using snprintf so I think it is safe, but I think using PATH_MAX for msg length is wrong. start can be an arbitrary extended object name expression (HEAD^12~24^2~4^^2~13...) and does not have much to do with pathname.

> 	snprintf(ref, sizeof ref, "refs/heads/%s", name);

Maybe barf if snprintf steps over the buffer?