* [PATCH] branch as a builtin (again)
@ 2006-08-20 21:22 Kristian Høgsberg
2006-08-20 23:55 ` Johannes Schindelin
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Kristian Høgsberg @ 2006-08-20 21:22 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 349 bytes --]
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>
[-- Attachment #2: builtin-branch.patch --]
[-- Type: application/octet-stream, Size: 9011 bytes --]
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 },
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-20 21:22 [PATCH] branch as a builtin (again) Kristian Høgsberg
@ 2006-08-20 23:55 ` Johannes Schindelin
2006-08-21 7:49 ` David Rientjes
2006-08-21 10:13 ` Jonas Fonseca
2 siblings, 0 replies; 13+ messages in thread
From: Johannes Schindelin @ 2006-08-20 23:55 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
[-- Attachment #1: Type: TEXT/PLAIN, Size: 760 bytes --]
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-20 21:22 [PATCH] branch as a builtin (again) Kristian Høgsberg
2006-08-20 23:55 ` Johannes Schindelin
@ 2006-08-21 7:49 ` David Rientjes
2006-08-21 8:03 ` Shawn Pearce
2006-08-21 10:13 ` Jonas Fonseca
2 siblings, 1 reply; 13+ messages in thread
From: David Rientjes @ 2006-08-21 7:49 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2958 bytes --]
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-21 7:49 ` David Rientjes
@ 2006-08-21 8:03 ` Shawn Pearce
0 siblings, 0 replies; 13+ messages in thread
From: Shawn Pearce @ 2006-08-21 8:03 UTC (permalink / raw)
To: hoegsberg; +Cc: git
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-20 21:22 [PATCH] branch as a builtin (again) Kristian Høgsberg
2006-08-20 23:55 ` Johannes Schindelin
2006-08-21 7:49 ` David Rientjes
@ 2006-08-21 10:13 ` Jonas Fonseca
2006-08-21 20:12 ` Kristian Høgsberg
2 siblings, 1 reply; 13+ messages in thread
From: Jonas Fonseca @ 2006-08-21 10:13 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-21 10:13 ` Jonas Fonseca
@ 2006-08-21 20:12 ` Kristian Høgsberg
2006-08-21 20:23 ` David Rientjes
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Kristian Høgsberg @ 2006-08-21 20:12 UTC (permalink / raw)
To: Jonas Fonseca, Shawn Pearce; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 905 bytes --]
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
[-- Attachment #2: builtin-branch.patch --]
[-- Type: application/octet-stream, Size: 10364 bytes --]
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 },
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-21 20:12 ` Kristian Høgsberg
@ 2006-08-21 20:23 ` David Rientjes
2006-08-21 20:45 ` Kristian Høgsberg
2006-08-21 20:27 ` Johannes Schindelin
2006-08-21 20:41 ` Shawn Pearce
2 siblings, 1 reply; 13+ messages in thread
From: David Rientjes @ 2006-08-21 20:23 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: Jonas Fonseca, Shawn Pearce, git
[-- Attachment #1: Type: TEXT/PLAIN, Size: 6228 bytes --]
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;
> +}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-21 20:12 ` Kristian Høgsberg
2006-08-21 20:23 ` David Rientjes
@ 2006-08-21 20:27 ` Johannes Schindelin
2006-08-21 21:07 ` Kristian Høgsberg
[not found] ` <59ad55d30608211337jabd515bra3566fbd0f7ba5a0@mail.gmail.com>
2006-08-21 20:41 ` Shawn Pearce
2 siblings, 2 replies; 13+ messages in thread
From: Johannes Schindelin @ 2006-08-21 20:27 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
[-- Attachment #1: Type: TEXT/PLAIN, Size: 297 bytes --]
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-21 20:12 ` Kristian Høgsberg
2006-08-21 20:23 ` David Rientjes
2006-08-21 20:27 ` Johannes Schindelin
@ 2006-08-21 20:41 ` Shawn Pearce
2 siblings, 0 replies; 13+ messages in thread
From: Shawn Pearce @ 2006-08-21 20:41 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: Jonas Fonseca, git
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. :-)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-21 20:23 ` David Rientjes
@ 2006-08-21 20:45 ` Kristian Høgsberg
2006-08-22 7:00 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Kristian Høgsberg @ 2006-08-21 20:45 UTC (permalink / raw)
To: David Rientjes; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 241 bytes --]
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
[-- Attachment #2: builtin-branch.c --]
[-- Type: application/octet-stream, Size: 5250 bytes --]
/*
* 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; 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 = xrealloc(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;
}
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-21 20:27 ` Johannes Schindelin
@ 2006-08-21 21:07 ` Kristian Høgsberg
[not found] ` <59ad55d30608211337jabd515bra3566fbd0f7ba5a0@mail.gmail.com>
1 sibling, 0 replies; 13+ messages in thread
From: Kristian Høgsberg @ 2006-08-21 21:07 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
[not found] ` <59ad55d30608211337jabd515bra3566fbd0f7ba5a0@mail.gmail.com>
@ 2006-08-21 21:25 ` Johannes Schindelin
0 siblings, 0 replies; 13+ messages in thread
From: Johannes Schindelin @ 2006-08-21 21:25 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
[-- Attachment #1: Type: TEXT/PLAIN, Size: 928 bytes --]
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] branch as a builtin (again)
2006-08-21 20:45 ` Kristian Høgsberg
@ 2006-08-22 7:00 ` Junio C Hamano
0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2006-08-22 7:00 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
"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?
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2006-08-22 7:00 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-08-20 21:22 [PATCH] branch as a builtin (again) Kristian Høgsberg
2006-08-20 23:55 ` Johannes Schindelin
2006-08-21 7:49 ` David Rientjes
2006-08-21 8:03 ` Shawn Pearce
2006-08-21 10:13 ` Jonas Fonseca
2006-08-21 20:12 ` Kristian Høgsberg
2006-08-21 20:23 ` David Rientjes
2006-08-21 20:45 ` Kristian Høgsberg
2006-08-22 7:00 ` Junio C Hamano
2006-08-21 20:27 ` Johannes Schindelin
2006-08-21 21:07 ` Kristian Høgsberg
[not found] ` <59ad55d30608211337jabd515bra3566fbd0f7ba5a0@mail.gmail.com>
2006-08-21 21:25 ` Johannes Schindelin
2006-08-21 20:41 ` Shawn Pearce
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).