Re: [PATCH] Make git-branch a builtin

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Lars Hjemli <hjemli@xxxxxxxxx> writes:

> This replaces git-branch.sh with builtin-branch.c

> +static const char builtin_branch_usage[] =
> +"git-branch [(-d | -D) <branchname>] | [[-f] <branchname> [<start-point>]] | -r";

Probably needs a matching change to your recent clean-up here.

> +static int append_ref(const char *refname, const unsigned char *sha1, int flags, 
> +		void *cb_data)
> +{
> +	if (ref_index >= ref_alloc) {
> +		ref_alloc = ref_alloc > 0 ? ref_alloc * 2 : 16;
> +		ref_list = xrealloc(ref_list, ref_alloc * sizeof(char *));
> +	}

It would be more consistent to use alloc_nr() from cache.h here
unless you have a specific need for allocation growing pattern,
which you don't.  Other parts of the system even just grows by
one and let xrealloc() (and in turn underlying realloc()) take
care of the optimization of repeated reallocations, which may
also be fine.

> +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];
> +
> +	snprintf(ref, sizeof ref, "refs/heads/%s", name);
> +	if (check_ref_format(ref))
> +		die("'%s' is not a valid branch name.", name);
> +
> +	if (resolve_ref(ref, sha1, 1, NULL)) {
> +		if (!force)
> +			die("A branch named '%s' already exists.", name);
> +		else if (!strcmp(head, name))
> +			die("Cannot force update the current branch.");
> +	}
> +
> +	if (get_sha1(start, sha1))
> +		die("Not a valid branch point: '%s'.", start);
> +
> +	lock = lock_any_ref_for_update(ref, NULL);
> +	if (!lock)
> +		die("Failed to lock ref for update: %s.", strerror(errno));

This is not enough.  "$2^0" in git-branch.sh makes sure the
"start" specified dereferences to a commit object, and object
name we use to initialize the new branch is of the commit object
(IOW, test your code with "git branch new v1.0.0" and make sure
"git rev-parse new" returns the commit object not the tag that
points at it).

> +		
> +	if (reflog){

Style; (1) remove trailing TAB on the empty line; (2) one SP
between ')' and '{' please.

> diff --git a/builtin.h b/builtin.h
> index 9683a7c..144d299 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -15,6 +15,7 @@ extern int write_tree(unsigned char *sha
>  extern int cmd_add(int argc, const char **argv, const char *prefix);
>  extern int cmd_annotate(int argc, const char **argv, const char *prefix);
>  extern int cmd_apply(int argc, const char **argv, const char *prefix);
> +extern int cmd_branch(int argc, const char **argv, const char *prefix);
>  extern int cmd_archive(int argc, const char **argv, const char *prefix);
>  extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
>  extern int cmd_checkout_index(int argc, const char **argv, const char *prefix);

Style; order them alphabetically please...

> diff --git a/git.c b/git.c
> index 308252a..671b212 100644
> --- a/git.c
> +++ b/git.c
> @@ -222,6 +222,7 @@ static void handle_internal_command(int 
>  		{ "annotate", cmd_annotate, },
>  		{ "apply", cmd_apply },
>  		{ "archive", cmd_archive },
> +		{ "branch", cmd_branch },
>  		{ "cat-file", cmd_cat_file, RUN_SETUP },
>  		{ "checkout-index", cmd_checkout_index, RUN_SETUP },
>  		{ "check-ref-format", cmd_check_ref_format },

In other words, match this.

We may someday want to do binary search into the command list,
and before that happens we should make sure this list is really
ordered.

-
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

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]