Re: [PATCH] branch as a builtin (again)
- From
Jonas Fonseca <fonseca@diku.dk>
- Date
- Aug 21, 2006, 10:13 UTC
- Message-ID
- <20060821101346.GA527@diku.dk>
- In-Reply-To
- <59ad55d30608201422h4a6d40f7y7782212637380438@mail.gmail.com>
Kristian Høgsberg <hoegsberg@gmail.com> wrote Sun, Aug 20, 2006:
Show 20 quoted lines
> 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.
Show 43 quoted lines
> +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 andwhether
Show 36 quoted lines
> + * 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.
Show 8 quoted lines
> + 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.
Show 43 quoted lines
> +
> + 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.
Show 6 quoted lines
> + }
> +
> + 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.
Show 12 quoted lines
> + 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