# [PATCH] branch as a builtin (again)

13 messages from 2006-08-20 to 2006-08-22. Participants: Kristian Høgsberg, Johannes Schindelin, David Rientjes, Shawn Pearce, Jonas Fonseca, Junio C Hamano.
Thread: https://gitlist.dev/t/5336

## Kristian Høgsberg, 2006-08-20 21:22

Subject: [PATCH] branch as a builtin (again)
Message-ID: <59ad55d30608201422h4a6d40f7y7782212637380438@mail.gmail.com>
URL: https://gitlist.dev/e/59ad55d30608201422h4a6d40f7y7782212637380438%40mail.gmail.com

```
Hi,

I sent a patch to rewrite branch in C and make it a builtin a couple
of months ago.  Junio had a few comments about the patch that I now
finally had the time to address.  One of the problems was that
merge-base didn't clean up its state, which has now been fixed.
Here's the updated version.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>


commit 8153d336ad9643507922932a2ac2277073c66432
Author: Kristian Høgsberg <krh@redhat.com>
Date:   Sun Aug 20 17:04:14 2006 -0400

    Rewrite branch in C and make it a builtin.

diff --git a/Makefile b/Makefile
index 23cd8a0..adf043e 100644
--- a/Makefile
+++ b/Makefile
@@ -149,7 +149,7 @@ SPARSE_FLAGS = -D__BIG_ENDIAN__ -D__powe
 ### --- END CONFIGURATION SECTION ---
 
 SCRIPT_SH = \
-	git-bisect.sh git-branch.sh git-checkout.sh \
+	git-bisect.sh git-checkout.sh \
 	git-cherry.sh git-clean.sh git-clone.sh git-commit.sh \
 	git-fetch.sh \
 	git-ls-remote.sh \
@@ -253,6 +253,7 @@ LIB_OBJS = \
 BUILTIN_OBJS = \
 	builtin-add.o \
 	builtin-apply.o \
+	builtin-branch.o \
 	builtin-cat-file.o \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \
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";
+
+
+static int remote_only = 0;
+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
+		 * 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);
+
+	if (resolve_ref(ref, sha1, 1)) {
+		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, 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);
+	}
+
+	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");
+	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);
+
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index ade58c4..eb28986 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_apply(int argc, const char **argv, const char *prefix);
+extern int cmd_branch(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);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
diff --git a/git-branch.sh b/git-branch.sh
deleted file mode 100755
index e0501ec..0000000
--- a/git-branch.sh
+++ /dev/null
@@ -1,130 +0,0 @@
-#!/bin/sh
-
-USAGE='[-l] [(-d | -D) <branchname>] | [[-f] <branchname> [<start-point>]] | -r'
-LONG_USAGE='If no arguments, show available branches and mark current branch with a star.
-If one argument, create a new branch <branchname> based off of current HEAD.
-If two arguments, create a new branch <branchname> based off of <start-point>.'
-
-SUBDIRECTORY_OK='Yes'
-. git-sh-setup
-
-headref=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
-
-delete_branch () {
-    option="$1"
-    shift
-    for branch_name
-    do
-	case ",$headref," in
-	",$branch_name,")
-	    die "Cannot delete the branch you are on." ;;
-	,,)
-	    die "What branch are you on anyway?" ;;
-	esac
-	branch=$(cat "$GIT_DIR/refs/heads/$branch_name") &&
-	    branch=$(git-rev-parse --verify "$branch^0") ||
-		die "Seriously, what branch are you talking about?"
-	case "$option" in
-	-D)
-	    ;;
-	*)
-	    mbs=$(git-merge-base -a "$branch" HEAD | tr '\012' ' ')
-	    case " $mbs " in
-	    *' '$branch' '*)
-		# the merge base of branch and HEAD contains branch --
-		# which means that the HEAD contains everything in both.
-		;;
-	    *)
-		echo >&2 "The branch '$branch_name' is not a strict subset of your current HEAD.
-If you are sure you want to delete it, run 'git branch -D $branch_name'."
-		exit 1
-		;;
-	    esac
-	    ;;
-	esac
-	rm -f "$GIT_DIR/logs/refs/heads/$branch_name"
-	rm -f "$GIT_DIR/refs/heads/$branch_name"
-	echo "Deleted branch $branch_name."
-    done
-    exit 0
-}
-
-ls_remote_branches () {
-    git-rev-parse --symbolic --all |
-    sed -ne 's|^refs/\(remotes/\)|\1|p' |
-    sort
-}
-
-force=
-create_log=
-while case "$#,$1" in 0,*) break ;; *,-*) ;; *) break ;; esac
-do
-	case "$1" in
-	-d | -D)
-		delete_branch "$@"
-		exit
-		;;
-	-r)
-		ls_remote_branches
-		exit
-		;;
-	-f)
-		force="$1"
-		;;
-	-l)
-		create_log="yes"
-		;;
-	--)
-		shift
-		break
-		;;
-	-*)
-		usage
-		;;
-	esac
-	shift
-done
-
-case "$#" in
-0)
-	git-rev-parse --symbolic --branches |
-	sort |
-	while read ref
-	do
-		if test "$headref" = "$ref"
-		then
-			pfx='*'
-		else
-			pfx=' '
-		fi
-		echo "$pfx $ref"
-	done
-	exit 0 ;;
-1)
-	head=HEAD ;;
-2)
-	head="$2^0" ;;
-esac
-branchname="$1"
-
-rev=$(git-rev-parse --verify "$head") || exit
-
-git-check-ref-format "heads/$branchname" ||
-	die "we do not like '$branchname' as a branch name."
-
-if [ -e "$GIT_DIR/refs/heads/$branchname" ]
-then
-	if test '' = "$force"
-	then
-		die "$branchname already exists."
-	elif test "$branchname" = "$headref"
-	then
-		die "cannot force-update the current branch."
-	fi
-fi
-if test "$create_log" = 'yes'
-then
-	mkdir -p $(dirname "$GIT_DIR/logs/refs/heads/$branchname")
-	touch "$GIT_DIR/logs/refs/heads/$branchname"
-fi
-git update-ref -m "branch: Created from $head" "refs/heads/$branchname" $rev
diff --git a/git.c b/git.c
index 930998b..5738cb4 100644
--- a/git.c
+++ b/git.c
@@ -226,6 +226,7 @@ static void handle_internal_command(int 
 	} commands[] = {
 		{ "add", cmd_add, RUN_SETUP },
 		{ "apply", cmd_apply },
+		{ "branch", cmd_branch, RUN_SETUP },
 		{ "cat-file", cmd_cat_file, RUN_SETUP },
 		{ "checkout-index", cmd_checkout_index, RUN_SETUP },
 		{ "check-ref-format", cmd_check_ref_format },

```

## Johannes Schindelin, 2006-08-20 23:55

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <Pine.LNX.4.63.0608210145200.28360@wbgn013.biozentrum.uni-wuerzburg.de>
URL: https://gitlist.dev/e/Pine.LNX.4.63.0608210145200.28360%40wbgn013.biozentrum.uni-wuerzburg.de
In-Reply-To: <59ad55d30608201422h4a6d40f7y7782212637380438@mail.gmail.com>

```
Hi,

On Sun, 20 Aug 2006, Kristian Høgsberg wrote:

> Hi,
> 
> I sent a patch to rewrite branch in C and make it a builtin a couple
> of months ago.  Junio had a few comments about the patch that I now
> finally had the time to address.  One of the problems was that
> merge-base didn't clean up its state, which has now been fixed.
> Here's the updated version.
> 
> Signed-off-by: Kristian Høgsberg <krh@redhat.com>

Micro-nit: in delete_branches(), you can assign rev2 outside the loop.

Also, "git branch" no longer sorts the names alphabetically (you could fix 
this by putting the names into a path_list, which sorts them).

The "-l" option (create a log) seems to be forgotten, but then, I never 
use that anyway.

AFAICT the rest is fine.

Ciao,
Dscho

```

## David Rientjes, 2006-08-21 07:49

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <Pine.LNX.4.63.0608210045450.17336@chino.corp.google.com>
URL: https://gitlist.dev/e/Pine.LNX.4.63.0608210045450.17336%40chino.corp.google.com
In-Reply-To: <59ad55d30608201422h4a6d40f7y7782212637380438@mail.gmail.com>

```
On Sun, 20 Aug 2006, Kristian Høgsberg wrote:

> 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";
> +
> +
> +static int remote_only = 0;

Unnecessary initialization

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

Make it cleaner for the future:
{
	int ret = 0;
	...
	for (b = bases; b; b = b->next) {
		if (!hashcmp(sha1, b->item->object.sha1)) {
			ret = 1;
			break;
		}
	}

	free_commit_list(bases);
	return ret;
}

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

p is unnecessary:
	if (!resolve_ref(name, sha1, 1))
		die(...);

> +
> +		rev1 = lookup_commit_reference(sha1);
> +		rev2 = lookup_commit_reference(head_sha1);

Are these both needed inside every iteration?

> +		if (!rev1 || !rev2)
> +			die("Couldn't look up commit objects.");
> +
> +		/* This checks wether the merge bases of branch and
> +		 * 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;

Unnecessary variable

> +}
> +
> +static void create_branch (const char *name, const char *start, int force)

No space

		David
```

## Shawn Pearce, 2006-08-21 08:03

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <20060821080345.GA26191@spearce.org>
URL: https://gitlist.dev/e/20060821080345.GA26191%40spearce.org
In-Reply-To: <Pine.LNX.4.63.0608210045450.17336@chino.corp.google.com>

```
> On Sun, 20 Aug 2006, Kristian Høgsberg wrote:
> > diff --git a/builtin-branch.c b/builtin-branch.c
> > +static const char builtin_branch_usage[] =
> > +    "git-branch [(-d | -D) <branchname>] | [[-f] <branchname> [<start-point>]] | -r";

What happened to the -l switch to create a reflog for the new branch?

> > +static void delete_branches(int argc, const char **argv, int force)

What happened to deleting the reflog when the branch gets deleted?


In both cases please see the existing git-branch documentation and
shell script.

I applaud the effort of removing some of these smaller shell scripts
with more portable C code but at the same time I'd hate to see a
loss of functionality, especially something that I use!  :-)

-- 
Shawn.

```

## Jonas Fonseca, 2006-08-21 10:13

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <20060821101346.GA527@diku.dk>
URL: https://gitlist.dev/e/20060821101346.GA527%40diku.dk
In-Reply-To: <59ad55d30608201422h4a6d40f7y7782212637380438@mail.gmail.com>

```
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

```

## Kristian Høgsberg, 2006-08-21 20:12

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <59ad55d30608211312u51a4657eyd52311314a6ee03c@mail.gmail.com>
URL: https://gitlist.dev/e/59ad55d30608211312u51a4657eyd52311314a6ee03c%40mail.gmail.com
In-Reply-To: <20060821101346.GA527@diku.dk>

```
Thanks to all who reviewed the patch, here's an updated version which
should address all issues.  In particular, thanks to Jonas who spotted
the missing git_path() before resolve_ref() - this caused git branch
to always overwrite existing branches because it failed to resolve the
ref.

As for the missing reflog functionality - when I first did the shell
to C port, git branch didn't have this feature, and this time I just
updated the old patch which is how I forgot about the reflog option.
The new patch attached here should have the same reflog behavior as
the current shell script.  Shawn, could you check that it has the
correct sematics?  I wasn't sure whether git branch -f foo should
truncate the old log or just keep appending.  The shell script does a
'touch <logfile>' on creation, which means keep appending when
forcibly overwriting a branch ref, so I kept that behavior.

cheers,
Kristian


commit d5d82b9c4493df467ee31776cadff808563f00b1
Author: Kristian Høgsberg <krh@redhat.com>
Date:   Sun Aug 20 17:04:14 2006 -0400

    Rewrite branch in C and make it a builtin.

diff --git a/Makefile b/Makefile
index 23cd8a0..adf043e 100644
--- a/Makefile
+++ b/Makefile
@@ -149,7 +149,7 @@ SPARSE_FLAGS = -D__BIG_ENDIAN__ -D__powe
 ### --- END CONFIGURATION SECTION ---
 
 SCRIPT_SH = \
-	git-bisect.sh git-branch.sh git-checkout.sh \
+	git-bisect.sh git-checkout.sh \
 	git-cherry.sh git-clean.sh git-clone.sh git-commit.sh \
 	git-fetch.sh \
 	git-ls-remote.sh \
@@ -253,6 +253,7 @@ LIB_OBJS = \
 BUILTIN_OBJS = \
 	builtin-add.o \
 	builtin-apply.o \
+	builtin-branch.o \
 	builtin-cat-file.o \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \
diff --git a/builtin-branch.c b/builtin-branch.c
new file mode 100644
index 0000000..4e8fa7d
--- /dev/null
+++ b/builtin-branch.c
@@ -0,0 +1,227 @@
+/*
+ * 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";
+
+
+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;
+	int ret = 0;
+
+	bases = get_merge_bases(rev1, rev2, 1);
+	for (b = bases; b != NULL; b = b->next) {
+		if (!hashcmp(sha1, b->item->object.sha1)) {
+			ret = 1;
+			break;
+		}
+	}
+
+	free_commit_list(bases);
+	return ret;
+}
+
+static void delete_branches(int argc, const char **argv, int force)
+{
+	struct commit *rev, *head_rev;
+	unsigned char sha1[20];
+	const char *name, *reflog;
+	int i;
+
+	head_rev = lookup_commit_reference(head_sha1);
+	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]);
+		if (!resolve_ref(name, sha1, 1))
+			die("Branch '%s' not found.", argv[i]);
+
+		rev = lookup_commit_reference(sha1);
+		if (!rev || !head_rev)
+			die("Couldn't look up commit objects.");
+
+		/* This checks whether the merge bases of branch and
+		 * HEAD contains branch -- which means that the HEAD
+		 * contains everything in both.
+		 */
+
+		if (!force &&
+		    !in_merge_bases(sha1, rev, head_rev)) {
+			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);
+
+		/* Unlink reflog if it exists. */
+		reflog = git_path("logs/refs/heads/%s", argv[i]);
+		unlink(reflog);
+
+		printf("Deleted branch %s.\n", argv[i]);
+	}
+}
+
+static int ref_index, ref_alloc;
+static char **ref_list;
+
+static int append_ref(const char *refname, const unsigned char *sha1)
+{
+	if (ref_index >= ref_alloc) {
+		ref_alloc = ref_alloc > 0 ? ref_alloc * 2 : 16;
+		ref_list = realloc(ref_list, ref_alloc * sizeof (char *));
+	}
+
+	ref_list[ref_index++] = strdup(refname);
+
+	return 0;
+}
+
+static int ref_cmp (const void *r1, const void *r2)
+{
+	return strcmp (*(char **)r1, *(char **)r2);
+}
+
+static void print_ref_list(int remote_only)
+{
+	int i;
+
+	if (remote_only)
+		for_each_remote_ref(append_ref);
+	else
+		for_each_branch_ref(append_ref);
+
+	qsort(ref_list, ref_index, sizeof (char *), ref_cmp);
+
+	for (i = 0; i < ref_index; i++) {
+		if (!strcmp(ref_list[i], head))
+			printf("* %s\n", ref_list[i]);
+		else
+			printf("  %s\n", ref_list[i]);
+	}
+}
+
+static void create_reflog(struct ref_lock *lock)
+{
+	struct stat stbuf;
+	int fd;
+
+	if (!stat(lock->log_file, &stbuf) && S_ISREG(stbuf.st_mode))
+		return;
+	if (safe_create_leading_directories(lock->log_file) < 0)
+		die("Unable to create directory for %s.", lock->log_file);
+	fd = open(lock->log_file, O_CREAT | O_TRUNC | O_WRONLY, 0666);
+	if (fd < 0)
+		die("Unable to create ref log %s: %s.",
+		    lock->log_file, strerror(errno));
+	close(fd);
+}
+
+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(git_path(ref), sha1, 1)) {
+		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, 0);
+	if (!lock)
+		die("Failed to lock ref for update: %s.", strerror(errno));
+	if (reflog)
+		create_reflog(lock);
+	snprintf(msg, sizeof msg, "branch: Created from %s", start);
+	if (write_ref_sha1(lock, sha1, msg) < 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, remote_only = 0;
+	int reflog = 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;
+		}
+		if (!strcmp(arg, "-l")) {
+			reflog = 1;
+			continue;
+		}
+		usage(builtin_branch_usage);
+	}
+
+	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.");
+	head = strdup(p + prefix_length);
+
+	if (delete)
+		delete_branches(argc - i, argv + i, force_delete);
+	else if (i == argc)
+		print_ref_list(remote_only);
+	else if (argc - i == 1)
+		create_branch(argv[i], head, force_create, reflog);
+	else
+		create_branch(argv[i], argv[i + 1], force_create, reflog);
+
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index ade58c4..eb28986 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_apply(int argc, const char **argv, const char *prefix);
+extern int cmd_branch(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);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
diff --git a/git-branch.sh b/git-branch.sh
deleted file mode 100755
index e0501ec..0000000
--- a/git-branch.sh
+++ /dev/null
@@ -1,130 +0,0 @@
-#!/bin/sh
-
-USAGE='[-l] [(-d | -D) <branchname>] | [[-f] <branchname> [<start-point>]] | -r'
-LONG_USAGE='If no arguments, show available branches and mark current branch with a star.
-If one argument, create a new branch <branchname> based off of current HEAD.
-If two arguments, create a new branch <branchname> based off of <start-point>.'
-
-SUBDIRECTORY_OK='Yes'
-. git-sh-setup
-
-headref=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
-
-delete_branch () {
-    option="$1"
-    shift
-    for branch_name
-    do
-	case ",$headref," in
-	",$branch_name,")
-	    die "Cannot delete the branch you are on." ;;
-	,,)
-	    die "What branch are you on anyway?" ;;
-	esac
-	branch=$(cat "$GIT_DIR/refs/heads/$branch_name") &&
-	    branch=$(git-rev-parse --verify "$branch^0") ||
-		die "Seriously, what branch are you talking about?"
-	case "$option" in
-	-D)
-	    ;;
-	*)
-	    mbs=$(git-merge-base -a "$branch" HEAD | tr '\012' ' ')
-	    case " $mbs " in
-	    *' '$branch' '*)
-		# the merge base of branch and HEAD contains branch --
-		# which means that the HEAD contains everything in both.
-		;;
-	    *)
-		echo >&2 "The branch '$branch_name' is not a strict subset of your current HEAD.
-If you are sure you want to delete it, run 'git branch -D $branch_name'."
-		exit 1
-		;;
-	    esac
-	    ;;
-	esac
-	rm -f "$GIT_DIR/logs/refs/heads/$branch_name"
-	rm -f "$GIT_DIR/refs/heads/$branch_name"
-	echo "Deleted branch $branch_name."
-    done
-    exit 0
-}
-
-ls_remote_branches () {
-    git-rev-parse --symbolic --all |
-    sed -ne 's|^refs/\(remotes/\)|\1|p' |
-    sort
-}
-
-force=
-create_log=
-while case "$#,$1" in 0,*) break ;; *,-*) ;; *) break ;; esac
-do
-	case "$1" in
-	-d | -D)
-		delete_branch "$@"
-		exit
-		;;
-	-r)
-		ls_remote_branches
-		exit
-		;;
-	-f)
-		force="$1"
-		;;
-	-l)
-		create_log="yes"
-		;;
-	--)
-		shift
-		break
-		;;
-	-*)
-		usage
-		;;
-	esac
-	shift
-done
-
-case "$#" in
-0)
-	git-rev-parse --symbolic --branches |
-	sort |
-	while read ref
-	do
-		if test "$headref" = "$ref"
-		then
-			pfx='*'
-		else
-			pfx=' '
-		fi
-		echo "$pfx $ref"
-	done
-	exit 0 ;;
-1)
-	head=HEAD ;;
-2)
-	head="$2^0" ;;
-esac
-branchname="$1"
-
-rev=$(git-rev-parse --verify "$head") || exit
-
-git-check-ref-format "heads/$branchname" ||
-	die "we do not like '$branchname' as a branch name."
-
-if [ -e "$GIT_DIR/refs/heads/$branchname" ]
-then
-	if test '' = "$force"
-	then
-		die "$branchname already exists."
-	elif test "$branchname" = "$headref"
-	then
-		die "cannot force-update the current branch."
-	fi
-fi
-if test "$create_log" = 'yes'
-then
-	mkdir -p $(dirname "$GIT_DIR/logs/refs/heads/$branchname")
-	touch "$GIT_DIR/logs/refs/heads/$branchname"
-fi
-git update-ref -m "branch: Created from $head" "refs/heads/$branchname" $rev
diff --git a/git.c b/git.c
index 930998b..5738cb4 100644
--- a/git.c
+++ b/git.c
@@ -226,6 +226,7 @@ static void handle_internal_command(int 
 	} commands[] = {
 		{ "add", cmd_add, RUN_SETUP },
 		{ "apply", cmd_apply },
+		{ "branch", cmd_branch, RUN_SETUP },
 		{ "cat-file", cmd_cat_file, RUN_SETUP },
 		{ "checkout-index", cmd_checkout_index, RUN_SETUP },
 		{ "check-ref-format", cmd_check_ref_format },

```

## David Rientjes, 2006-08-21 20:23

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <Pine.LNX.4.63.0608211319180.8662@chino.corp.google.com>
URL: https://gitlist.dev/e/Pine.LNX.4.63.0608211319180.8662%40chino.corp.google.com
In-Reply-To: <59ad55d30608211312u51a4657eyd52311314a6ee03c@mail.gmail.com>

```
On Mon, 21 Aug 2006, Kristian Høgsberg wrote:

> diff --git a/builtin-branch.c b/builtin-branch.c
> new file mode 100644
> index 0000000..4e8fa7d
> --- /dev/null
> +++ b/builtin-branch.c
> @@ -0,0 +1,227 @@
> +/*
> + * 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";
> +
> +
> +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;
> +	int ret = 0;
> +
> +	bases = get_merge_bases(rev1, rev2, 1);
> +	for (b = bases; b != NULL; b = b->next) {

for (b = bases; b; b = b->next) {

> +		if (!hashcmp(sha1, b->item->object.sha1)) {
> +			ret = 1;
> +			break;
> +		}
> +	}
> +
> +	free_commit_list(bases);
> +	return ret;
> +}
> +
> +static void delete_branches(int argc, const char **argv, int force)
> +{
> +	struct commit *rev, *head_rev;
> +	unsigned char sha1[20];
> +	const char *name, *reflog;
> +	int i;
> +
> +	head_rev = lookup_commit_reference(head_sha1);
> +	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]);
> +		if (!resolve_ref(name, sha1, 1))
> +			die("Branch '%s' not found.", argv[i]);
> +
> +		rev = lookup_commit_reference(sha1);
> +		if (!rev || !head_rev)
> +			die("Couldn't look up commit objects.");
> +
> +		/* This checks whether the merge bases of branch and
> +		 * HEAD contains branch -- which means that the HEAD
> +		 * contains everything in both.
> +		 */
> +
> +		if (!force &&
> +		    !in_merge_bases(sha1, rev, head_rev)) {
> +			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);
> +
> +		/* Unlink reflog if it exists. */
> +		reflog = git_path("logs/refs/heads/%s", argv[i]);
> +		unlink(reflog);
> +
> +		printf("Deleted branch %s.\n", argv[i]);
> +	}
> +}
> +
> +static int ref_index, ref_alloc;
> +static char **ref_list;
> +
> +static int append_ref(const char *refname, const unsigned char *sha1)
> +{
> +	if (ref_index >= ref_alloc) {
> +		ref_alloc = ref_alloc > 0 ? ref_alloc * 2 : 16;
> +		ref_list = realloc(ref_list, ref_alloc * sizeof (char *));

No space

> +	}
> +
> +	ref_list[ref_index++] = strdup(refname);
> +
> +	return 0;
> +}
> +
> +static int ref_cmp (const void *r1, const void *r2)

No space

> +{
> +	return strcmp (*(char **)r1, *(char **)r2);

No space

> +}
> +
> +static void print_ref_list(int remote_only)
> +{
> +	int i;
> +
> +	if (remote_only)
> +		for_each_remote_ref(append_ref);
> +	else
> +		for_each_branch_ref(append_ref);
> +
> +	qsort(ref_list, ref_index, sizeof (char *), ref_cmp);
> +

No space

> +	for (i = 0; i < ref_index; i++) {
> +		if (!strcmp(ref_list[i], head))
> +			printf("* %s\n", ref_list[i]);
> +		else
> +			printf("  %s\n", ref_list[i]);
> +	}
> +}
> +
> +static void create_reflog(struct ref_lock *lock)
> +{
> +	struct stat stbuf;
> +	int fd;
> +
> +	if (!stat(lock->log_file, &stbuf) && S_ISREG(stbuf.st_mode))
> +		return;
> +	if (safe_create_leading_directories(lock->log_file) < 0)
> +		die("Unable to create directory for %s.", lock->log_file);
> +	fd = open(lock->log_file, O_CREAT | O_TRUNC | O_WRONLY, 0666);
> +	if (fd < 0)
> +		die("Unable to create ref log %s: %s.",
> +		    lock->log_file, strerror(errno));
> +	close(fd);
> +}
> +
> +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(git_path(ref), sha1, 1)) {
> +		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, 0);
> +	if (!lock)
> +		die("Failed to lock ref for update: %s.", strerror(errno));
> +	if (reflog)
> +		create_reflog(lock);
> +	snprintf(msg, sizeof msg, "branch: Created from %s", start);
> +	if (write_ref_sha1(lock, sha1, msg) < 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, remote_only = 0;
> +	int reflog = 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;
> +		}
> +		if (!strcmp(arg, "-l")) {
> +			reflog = 1;
> +			continue;
> +		}
> +		usage(builtin_branch_usage);
> +	}
> +
> +	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.");
> +	head = strdup(p + prefix_length);
> +
> +	if (delete)
> +		delete_branches(argc - i, argv + i, force_delete);
> +	else if (i == argc)
> +		print_ref_list(remote_only);
> +	else if (argc - i == 1)
> +		create_branch(argv[i], head, force_create, reflog);
> +	else
> +		create_branch(argv[i], argv[i + 1], force_create, reflog);
> +
> +	return 0;
> +}
```

## Johannes Schindelin, 2006-08-21 20:27

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <Pine.LNX.4.63.0608212227040.28360@wbgn013.biozentrum.uni-wuerzburg.de>
URL: https://gitlist.dev/e/Pine.LNX.4.63.0608212227040.28360%40wbgn013.biozentrum.uni-wuerzburg.de
In-Reply-To: <59ad55d30608211312u51a4657eyd52311314a6ee03c@mail.gmail.com>

```
Hi,

On Mon, 21 Aug 2006, Kristian Høgsberg wrote:

> Thanks to all who reviewed the patch, here's an updated version which
> should address all issues.

I would have preferred the use of path_list instead of rolling your own 
thing with qsort(), but oh well.

Rest looks fine to me.

Ciao,
Dscho

```

## Shawn Pearce, 2006-08-21 20:41

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <20060821204110.GA24465@spearce.org>
URL: https://gitlist.dev/e/20060821204110.GA24465%40spearce.org
In-Reply-To: <59ad55d30608211312u51a4657eyd52311314a6ee03c@mail.gmail.com>

```
Kristian H?gsberg <krh@bitplanet.net> wrote:
> +static void delete_branches(int argc, const char **argv, int force)
[snip]
> +		name = git_path("refs/heads/%s", argv[i]);
> +		if (!resolve_ref(name, sha1, 1))
> +			die("Branch '%s' not found.", argv[i]);
[snip]
> +		unlink(name);
> +
> +		/* Unlink reflog if it exists. */
> +		reflog = git_path("logs/refs/heads/%s", argv[i]);
> +		unlink(reflog);

Hmm.  So git-branch.sh doesn't deal with symrefs, eh?  I guess this
is OK but I'm wondering why not put this code into refs.c to lock
the ref (refs.c:lock_ref_sha1) then instead of unlocking it delete
it and its log (add new function to do this).

The downside of this is that we'll chase a symref, which means that
if refs/heads/FOO is a symref to refs/heads/master and the user calls
`git-branch -D FOO` we'll kill refs/heads/master.  Maybe that's not
what the the user would want to have happen.  :-)

> +static void create_reflog(struct ref_lock *lock)
> +{
> +	struct stat stbuf;
> +	int fd;
> +
> +	if (!stat(lock->log_file, &stbuf) && S_ISREG(stbuf.st_mode))
> +		return;
> +	if (safe_create_leading_directories(lock->log_file) < 0)
> +		die("Unable to create directory for %s.", lock->log_file);
> +	fd = open(lock->log_file, O_CREAT | O_TRUNC | O_WRONLY, 0666);
> +	if (fd < 0)
> +		die("Unable to create ref log %s: %s.",
> +		    lock->log_file, strerror(errno));
> +	close(fd);
> +}

This probably should move into refs.c.  Look at log_ref_write,
specifically around the if (log_all_ref_updates).  If this took
an additional parameter to force creation of the log even if the log
isn't present and OR'd against log_all_ref_updates then it would
be possible to have the refs.c code create the log for you in the
"library" part of GIT.

Or maybe it is better to add this as a flag to the struct ref_lock,
defaulting to false and letting the caller set it to true before
invoking write_ref_sha1.  I only suggest this because of the number
of parameters already in play here.

> +static void create_branch(const char *name, const char *start,
> +			  int force, int reflog)

This all looked correct to me, at least as far as dealing with
the reflog.  :-)

```

## Kristian Høgsberg, 2006-08-21 20:45

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <59ad55d30608211345k7956a219m8ab450c6f2023461@mail.gmail.com>
URL: https://gitlist.dev/e/59ad55d30608211345k7956a219m8ab450c6f2023461%40mail.gmail.com
In-Reply-To: <Pine.LNX.4.63.0608211319180.8662@chino.corp.google.com>

```
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.

cheers,
Kristian

```

## Kristian Høgsberg, 2006-08-21 21:07

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <59ad55d30608211407v74e1369dw17fae16933c0356f@mail.gmail.com>
URL: https://gitlist.dev/e/59ad55d30608211407v74e1369dw17fae16933c0356f%40mail.gmail.com
In-Reply-To: <Pine.LNX.4.63.0608212227040.28360@wbgn013.biozentrum.uni-wuerzburg.de>

```
On 8/21/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Mon, 21 Aug 2006, Kristian Hxgsberg wrote:
>
> > Thanks to all who reviewed the patch, here's an updated version which
> > should address all issues.
>
> I would have preferred the use of path_list instead of rolling your own
> thing with qsort(), but oh well.

Yeah, I saw that, but since I got flack for computing
lookup_commit_reference(head_sha1) inside the delete_branches loop, I
couldn't possibly risk the performance bottle neck of listing the
branches using a O(n^2) insertion sort.

Kristian

```

## Johannes Schindelin, 2006-08-21 21:25

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <Pine.LNX.4.63.0608212323420.28360@wbgn013.biozentrum.uni-wuerzburg.de>
URL: https://gitlist.dev/e/Pine.LNX.4.63.0608212323420.28360%40wbgn013.biozentrum.uni-wuerzburg.de
In-Reply-To: <59ad55d30608211337jabd515bra3566fbd0f7ba5a0@mail.gmail.com>

```
Hi,

On Mon, 21 Aug 2006, Kristian Høgsberg wrote:

> On 8/21/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> > 
> > On Mon, 21 Aug 2006, Kristian Høgsberg wrote:
> > 
> > > Thanks to all who reviewed the patch, here's an updated version which
> > > should address all issues.
> > 
> > I would have preferred the use of path_list instead of rolling your own
> > thing with qsort(), but oh well.
> 
> Yeah, I saw that, but since I got flack for computing
> lookup_commit_reference(head_sha1) inside the delete_branches loop, I
> couldn't possibly risk the performance bottle neck of listing the
> branches using a O(n^2) insertion sort.

Insertion sort, as implemented in path_list, has an average O(n log(n)) 
runtime, and a worst-case O(n^2) runtime. Same as qsort...

Besides, it is not like we are dealing with millions of branches here. And 
using path_list _would_ make the code shorter.

Ciao,
Dscho

```

## Junio C Hamano, 2006-08-22 07:00

Subject: Re: [PATCH] branch as a builtin (again)
Message-ID: <7vveolqmpb.fsf@assigned-by-dhcp.cox.net>
URL: https://gitlist.dev/e/7vveolqmpb.fsf%40assigned-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.

> 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?

```