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

[Jonas Fonseca <fonseca@diku.dk>]

Kristian Høgsberg <hoegsberg@gmail.com> wrote Sun, Aug 20, 2006:
> diff --git a/builtin-branch.c b/builtin-branch.c
> new file mode 100644
> index 0000000..25c6313
> --- /dev/null
> +++ b/builtin-branch.c
> @@ -0,0 +1,170 @@
> +/*
> + * Builtin "git branch"
> + *
> + * Copyright (c) 2006 Kristian Høgsberg <krh@redhat.com>
> + * Based on git-branch.sh by Junio C Hamano.
> + */
> +
> +#include "cache.h"
> +#include "refs.h"
> +#include "commit.h"
> +#include "builtin.h"
> +
> +static const char builtin_branch_usage[] =
> +    "git-branch [(-d | -D) <branchname>] | [[-f] <branchname> [<start-point>]] | -r";

The norm seems to be that the usage string should not be indented.

> +
> +
> +static int remote_only = 0;

This could be local to cmd_branch. 

> +static const char *head;
> +static unsigned char head_sha1[20];
> +
> +static int in_merge_bases(const unsigned char *sha1,
> +			  struct commit *rev1,
> +			  struct commit *rev2)
> +{
> +	struct commit_list *bases, *b;
> +
> +	bases = get_merge_bases(rev1, rev2, 1);
> +	for (b = bases; b != NULL; b = b->next) {
> +		if (!hashcmp(sha1, b->item->object.sha1)) {
> +			free_commit_list(bases);
> +			return 1;
> +		}
> +	}
> +
> +	free_commit_list(bases);
> +	return 0;
> +}
> +
> +static void delete_branches(int argc, const char **argv, int force)
> +{
> +	struct commit *rev1, *rev2;
> +	unsigned char sha1[20];
> +	const char *p, *name;
> +	int i;
> +
> +	for (i = 0; i < argc; i++) {
> +		if (!strcmp(head, argv[i]))
> +			die("Cannot delete the branch you are currently on.");
> +
> +		name = git_path("refs/heads/%s", argv[i]);
> +		p = resolve_ref(name, sha1, 1);
> +		if (p == NULL)
> +			die("Branch '%s' not found.", argv[i]);
> +
> +		rev1 = lookup_commit_reference(sha1);
> +		rev2 = lookup_commit_reference(head_sha1);
> +		if (!rev1 || !rev2)
> +			die("Couldn't look up commit objects.");
> +
> +		/* This checks wether the merge bases of branch and

			       whether

> +		 * HEAD contains branch -- which means that the HEAD
> +		 * contains everything in both.
> +		 */
> +
> +		if (!force &&
> +		    !in_merge_bases(sha1, rev1, rev2)) {
> +			fprintf(stderr,
> +				"The branch '%s' is not a strict subset of your current HEAD.\n"
> +				"If you are sure you want to delete it, run 'git branch -D %s'.\n",
> +				argv[i], argv[i]);
> +			exit(1);
> +		}
> +
> +		unlink(name);
> +		printf("Deleted branch %s.\n", argv[i]);
> +	}
> +}
> +
> +static int show_reference(const char *refname, const unsigned char *sha1)
> +{
> +	int is_head = !strcmp(refname, head);
> +
> +	printf("%c %s\n", (is_head ? '*' : ' '), refname);
> +
> +	return 0;
> +}
> +
> +static void create_branch (const char *name, const char *start, int force)
> +{
> +	struct ref_lock *lock;
> +	unsigned char sha1[20];
> +	char ref[PATH_MAX];
> +
> +	snprintf(ref, sizeof ref, "refs/heads/%s", name);
> +	if (check_ref_format(ref + 5))
> +		die("'%s' is not a valid branch name.", name);

Why not simply check_ref_format(name)?

> +
> +	if (resolve_ref(ref, sha1, 1)) {

All other places that call resolve_ref passes a ref created with
git_path. I don't know if this should too.

> +		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);

Missing punctuation at the end.

> +
> +	lock = lock_any_ref_for_update(ref, NULL, 0);
> +	if (!lock)
> +		die("Failed to lock ref for update: %s.", strerror(errno));
> +	if (write_ref_sha1(lock, sha1, NULL) < 0)
> +		die("Failed to write ref: %s.", strerror(errno));
> +}
> +
> +int cmd_branch(int argc, const char **argv, const char *prefix)
> +{
> +	int delete = 0, force_delete = 0, force_create = 0;
> +	int i, prefix_length;
> +	const char *p;
> +
> +	git_config(git_default_config);
> +
> +	for (i = 1; i < argc; i++) {
> +		const char *arg = argv[i];
> +
> +		if (arg[0] != '-')
> +			break;
> +		if (!strcmp(arg, "--")) {
> +			i++;
> +			break;
> +		}
> +		if (!strcmp(arg, "-d")) {
> +			delete = 1;
> +			continue;
> +		}
> +		if (!strcmp(arg, "-D")) {
> +			delete = 1;
> +			force_delete = 1;
> +			continue;
> +		}
> +		if (!strcmp(arg, "-f")) {
> +			force_create = 1;
> +			continue;
> +		}
> +		if (!strcmp(arg, "-r")) {
> +			remote_only = 1;
> +			continue;
> +		}
> +		die(builtin_branch_usage);

Perhaps usage() would be more appropriate here.

> +	}
> +
> +	prefix_length = strlen(git_path("refs/heads/"));
> +	p = resolve_ref(git_path("HEAD"), head_sha1, 0);
> +	if (!p)
> +		die("Failed to resolve HEAD as a valid ref");

Ending punctuation.

> +	head = strdup(p + prefix_length);
> +
> +	if (delete)
> +		delete_branches(argc - i, argv + i, force_delete);
> +	else if (i == argc && remote_only)
> +		for_each_remote_ref(show_reference);
> +	else if (i == argc)
> +		for_each_branch_ref(show_reference);
> +	else if (argc - i == 1)
> +		create_branch (argv[i], head, force_create);
> +	else
> +		create_branch (argv[i], argv[i + 1], force_create);

It would be more consistent to leave out the space before the
paranthesis. Also goes for the implementation of create_branch as
already mentioned.

> +
> +	return 0;
> +}

-- 
Jonas Fonseca