Make git-branch a builtin
PATCH 1/1 Make git-branch a builtin
This replaces git-branch.sh with builtin-branch.c
The changes is basically a patch from Kristian Høgsberg, updated to apply onto current 'next'
--- Makefile | 3 +- builtin-branch.c | 217 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ builtin.h | 1 + git-branch.sh | 131 -------------------------------- git.c | 1 + 5 files changed, 221 insertions(+), 132 deletions(-)
Makefile +2 -1
@@ -158,7 +158,7 @@ BASIC_CFLAGS = BASIC_LDFLAGS = 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 \
@@ -270,6 +270,7 @@ BUILTIN_OBJS = \ builtin-annotate.o \ builtin-apply.o \ builtin-archive.o \ + builtin-branch.o \ builtin-cat-file.o \ builtin-checkout-index.o \ builtin-check-ref-format.o \
builtin-branch.c new file +217 -0
217 lines not on this page · raw message
builtin.h +1 -0
@@ -15,6 +15,7 @@ extern int write_tree(unsigned char *shaextern int cmd_add (int argc,const char **argv,const char *prefix);extern int cmd_annotate (int argc,const char **argv,const char *prefix);extern int cmd_apply (int argc,const char **argv,const char *prefix); +extern int cmd_branch (int argc,const char **argv,const char *prefix);extern int cmd_archive (int argc,const char **argv,const char *prefix);extern int cmd_cat_file (int argc,const char **argv,const char *prefix);extern int cmd_checkout_index (int argc,const char **argv,const char *prefix);
git-branch.sh deleted +0 -131
131 lines not on this page · raw message
git.c +1 -0
@@ -222,6 +222,7 @@ static void handle_internal_command(int { "annotate", cmd_annotate, }, { "apply", cmd_apply }, { "archive", cmd_archive }, + { "branch", cmd_branch }, { "cat-file", cmd_cat_file, RUN_SETUP }, { "checkout-index", cmd_checkout_index, RUN_SETUP }, { "check-ref-format", cmd_check_ref_format },
Signature · 1 line
1.4.3.1.g9164-dirty
Lars Hjemli <[email protected]> wrote:
[snip]
Quoted 7 lines
--- Makefile | 3 +- builtin-branch.c | 217 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ builtin.h | 1 + git-branch.sh | 131 -------------------------------- git.c | 1 + 5 files changed, 221 insertions(+), 132 deletions(-)
[snip]
git-branch.sh
Quoted 6 lines
git-branch.sh deleted +0
-0@@ -1,131 +0,0 @@
[snip 131 lines]
Aside from compatability with non-Git tools...
Wouldn't it make more sense to just include the full SHA1 of the file we are deleting rather than the entire 131 line negative diff?
Signature · 1 line
Shawn.
Dear diary, on Mon, Oct 23, 2006 at 11:55:06PM CEST, I got a letter where Shawn Pearce <[email protected]> said that...
Quoted 18 lines
Lars Hjemli <[email protected]> wrote:
[snip]Quoted 7 lines
--- Makefile | 3 +- builtin-branch.c | 217 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ builtin.h | 1 + git-branch.sh | 131 -------------------------------- git.c | 1 + 5 files changed, 221 insertions(+), 132 deletions(-)[snip]
git-branch.sh
Quoted 6 lines
git-branch.sh deleted +0
-0@@ -1,131 +0,0 @@[snip 131 lines]
Quoted 4 lines
Aside from compatability with non-Git tools...
Wouldn't it make more sense to just include the full SHA1 of the file we are deleting rather than the entire 131 line negative diff?
I think this way it's easier to review since you can check what is going away with the new stuff you introduce instead.
Signature · 5 lines
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)Quoted 18 lines · Shawn Pearce
Lars Hjemli <[email protected]> wrote:
[snip]Quoted 7 lines
--- Makefile | 3 +- builtin-branch.c | 217 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ builtin.h | 1 + git-branch.sh | 131 -------------------------------- git.c | 1 + 5 files changed, 221 insertions(+), 132 deletions(-)[snip]
git-branch.sh
Quoted 6 lines
git-branch.sh deleted +0
-0@@ -1,131 +0,0 @@[snip 131 lines]
Quoted 4 lines
Aside from compatability with non-Git tools...
Wouldn't it make more sense to just include the full SHA1 of the file we are deleting rather than the entire 131 line negative diff?
How would you do "git apply -R" on something like that?
Quoted 20 lines · Junio C Hamano
Quoted 18 lines · Shawn Pearce
Lars Hjemli <[email protected]> wrote:
[snip]Quoted 7 lines
--- Makefile | 3 +- builtin-branch.c | 217 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ builtin.h | 1 + git-branch.sh | 131 -------------------------------- git.c | 1 + 5 files changed, 221 insertions(+), 132 deletions(-)[snip]
git-branch.sh
Quoted 6 lines
git-branch.sh deleted +0
-0@@ -1,131 +0,0 @@[snip 131 lines]
Quoted 6 lines
Quoted 4 lines
Aside from compatability with non-Git tools...
Wouldn't it make more sense to just include the full SHA1 of the file we are deleting rather than the entire 131 line negative diff?
How would you do "git apply -R" on something like that?
Uh, you have the full SHA1 in the index line. So you just have to reattach that blob to the named path... pretty simple actually.
Of course this is yet another code path in apply and as Petr pointed out on this same thread its nice to know what you are deleting so you can compare it to what else is changing in the same patch... so this suggestion of mine is probably just a waste of bytes.
Signature · 1 line
Shawn.
Quoted 7 lines · Shawn Pearce
Quoted 4 lines
Quoted 2 lines
Wouldn't it make more sense to just include the full SHA1 of the file we are deleting rather than the entire 131 line negative diff?
How would you do "git apply -R" on something like that?
Uh, you have the full SHA1 in the index line. So you just have to reattach that blob to the named path... pretty simple actually.
Bzzzt; wrong answer.
Think of a future when you can shallowly clone near the tip of git repository that does not have shell-script git-branch.sh anymore. You cannot expect to already have the preimage of the patch in such a case. You would still want to be able to revert the change with "git apply -R".
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Shawn Pearce
Quoted 4 lines
Quoted 2 lines
Wouldn't it make more sense to just include the full SHA1 of the file we are deleting rather than the entire 131 line negative diff?
How would you do "git apply -R" on something like that?
Uh, you have the full SHA1 in the index line. So you just have to reattach that blob to the named path... pretty simple actually.
Quoted 7 lines
Bzzzt; wrong answer.
Think of a future when you can shallowly clone near the tip of git repository that does not have shell-script
git-branch.shanymore. You cannot expect to already have the preimage of the patch in such a case. You would still want to be able to revert the change with "git apply -R".
Yet another valid argument for having the data needed for the preimage available.
I've already agreed with Petr that having the data there is probably better than not. Now I'm also agreeing with you. There is little need to beat a dead horse. :-)
Signature · 1 line
Shawn.
Dear diary, on Tue, Oct 24, 2006 at 08:43:35AM CEST, I got a letter where Junio C Hamano <[email protected]> said that...
Quoted 10 lines
Quoted 7 lines · Shawn Pearce
Quoted 4 lines
Quoted 2 lines
Wouldn't it make more sense to just include the full SHA1 of the file we are deleting rather than the entire 131 line negative diff?
How would you do "git apply -R" on something like that?
Uh, you have the full SHA1 in the index line. So you just have to reattach that blob to the named path... pretty simple actually.
Quoted 7 lines
Bzzzt; wrong answer.
Think of a future when you can shallowly clone near the tip of git repository that does not have shell-script
git-branch.shanymore. You cannot expect to already have the preimage of the patch in such a case. You would still want to be able to revert the change with "git apply -R".
Hmm, how is this argument not applying to binary diffs you can't revert either?
Signature · 5 lines
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)Quoted 8 lines · Petr Baudis
Quoted 5 lines
Think of a future when you can shallowly clone near the tip of git repository that does not have shell-script
git-branch.shanymore. You cannot expect to already have the preimage of the patch in such a case. You would still want to be able to revert the change with "git apply -R".Hmm, how is this argument not applying to binary diffs you can't revert either?
Earlier you cannot, but because now you can, perhaps? ;-)
Quoted 4 lines · Lars Hjemli
This replaces
git-branch.shwithbuiltin-branch.c+static const char builtin_branch_usage[] =
+"git-branch [(-d | -D) <branchname>] | [[-f] <branchname> [<start-point>]] | -r";
Probably needs a matching change to your recent clean-up here.
builtin-branch.c line 91
Quoted 7 lines
builtin-branch.c new file +7
-0+static int append_ref (const char *refname,const unsigned char *sha1,int flags, +void *cb_data) +{ +if (ref_index >= ref_alloc) { + ref_alloc = ref_alloc >0 ? ref_alloc *2 :16 ; + ref_list =xrealloc (ref_list, ref_alloc *sizeof (char *)); + }
It would be more consistent to use alloc_nr() from cache.h here unless you have a specific need for allocation growing pattern, which you don't. Other parts of the system even just grows by one and let xrealloc() (and in turn underlying realloc()) take care of the optimization of repeated reallocations, which may also be fine.
Quoted 14 lines
builtin-branch.c new file +14
-0+static void create_branch (const char *name,const char *start, +int force,int reflog) +{ +struct ref_lock *lock; +unsigned char sha1[20 ]; +char ref[PATH_MAX ], msg[PATH_MAX +20 ]; + +snprintf (ref,sizeof ref,"refs/heads/%s" , name); +if (check_ref_format (ref)) +die ("'%s' is not a valid branch name." , name); + +if (resolve_ref (ref, sha1,1 ,NULL )) { +if (!force) +die ("A branch named '%s' already exists." , name);
builtin-branch.c line 145
Quoted 10 lines
builtin-branch.c new file +10
-0+else if (!strcmp (head, name)) +die ("Cannot force update the current branch." ); + } + +if (get_sha1 (start, sha1)) +die ("Not a valid branch point: '%s'." , start); + + lock =lock_any_ref_for_update (ref,NULL ); +if (!lock) +die ("Failed to lock ref for update: %s." ,strerror (errno));
This is not enough. "$2^0" in git-branch.sh makes sure the "start" specified dereferences to a commit object, and object name we use to initialize the new branch is of the commit object (IOW, test your code with "git branch new v1.0.0" and make sure "git rev-parse new" returns the commit object not the tag that points at it).
Review
Quoted 2 lines
(quoted diff) +2
-0+ + if (reflog){
Style; (1) remove trailing TAB on the empty line; (2) one SP between ')' and '{' please.
builtin.h extern int write_tree(unsigned char *sha · line 21
Quoted 12 lines
builtin.h +1
-0@@ -15,6 +15,7 @@ extern int write_tree(unsigned char *shaextern int cmd_add (int argc,const char **argv,const char *prefix);extern int cmd_annotate (int argc,const char **argv,const char *prefix);extern int cmd_apply (int argc,const char **argv,const char *prefix); +extern int cmd_branch (int argc,const char **argv,const char *prefix);extern int cmd_archive (int argc,const char **argv,const char *prefix);extern int cmd_cat_file (int argc,const char **argv,const char *prefix);extern int cmd_checkout_index (int argc,const char **argv,const char *prefix);
Style; order them alphabetically please...
git.c static void handle_internal_command(int · line 228
Quoted 12 lines
git.c +1
-0@@ -222,6 +222,7 @@ static void handle_internal_command(int { "annotate", cmd_annotate, }, { "apply", cmd_apply }, { "archive", cmd_archive }, + { "branch", cmd_branch }, { "cat-file", cmd_cat_file, RUN_SETUP }, { "checkout-index", cmd_checkout_index, RUN_SETUP }, { "check-ref-format", cmd_check_ref_format },
In other words, match this.
We may someday want to do binary search into the command list, and before that happens we should make sure this list is really ordered.
PATCH 1/1 Git-branch: fix regression and style issues
This is on top of my previuos patch to builtin-branch.c
--- builtin-branch.c | 17 ++++++++--------- builtin.h | 2 +- 2 files changed, 9 insertions(+), 10 deletions(-)
builtin-branch.c +8 -9
@@ -11,7 +11,7 @@ #include "commit.h"#include "builtin.h" static const char builtin_branch_usage[] = -"git-branch [(-d | -D) <branchname>] | [[-f] <branchname> [<start-point>]] | -r" ; +"git-branch (-d | -D) <branchname> | [-l] [-f] <branchname> [<start-point>] | [-r]" ;static const char *head;
@@ -74,7 +74,7 @@ static void delete_branches(int argc, coprintf ("Error deleting branch '%s'\n" , argv[i]); else printf("Deleted branch %s.\n", argv[i]); - +free (name); } }
@@ -82,11 +82,11 @@ static void delete_branches(int argc, costatic int ref_index, ref_alloc;static char **ref_list; -static int append_ref (const char *refname,const unsigned char *sha1,int flags, +static int append_ref (const char *refname,const unsigned char *sha1,int flags,void *cb_data) {if (ref_index >= ref_alloc) { - ref_alloc = ref_alloc >0 ? ref_alloc *2 :16 ; + ref_alloc =alloc_nr (ref_alloc); ref_list =xrealloc (ref_list, ref_alloc *sizeof (char *)); }
@@ -137,14 +137,14 @@ static void create_branch(const char *nadie ("Cannot force update the current branch." ); } -if (get_sha1 (start, sha1)) +if (get_sha1 (mkpath ("%s^0" , start), sha1))die ("Not a valid branch point: '%s'." , start); lock =lock_any_ref_for_update (ref,NULL );if (!lock)die ("Failed to lock ref for update: %s." ,strerror (errno)); - -if (reflog){ + +if (reflog) { log_all_ref_updates =1 ;snprintf (msg,sizeof msg,"branch: Created from %s" , start); }
@@ -199,9 +199,8 @@ int cmd_branch(int argc, const char **ardie ("Failed to resolve HEAD as a valid ref." );if (strncmp (head,"refs/heads/" ,11 ))die ("HEAD not found below refs/heads!" ); - head +=11 ; - +if (delete)delete_branches (argc - i, argv + i, force_delete);else if (i == argc)
builtin.h +1 -1
@@ -15,8 +15,8 @@ extern int write_tree(unsigned char *shaextern int cmd_add (int argc,const char **argv,const char *prefix);extern int cmd_annotate (int argc,const char **argv,const char *prefix);extern int cmd_apply (int argc,const char **argv,const char *prefix); -extern int cmd_branch (int argc,const char **argv,const char *prefix);extern int cmd_archive (int argc,const char **argv,const char *prefix); +extern int cmd_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);
Signature · 1 line
1.4.3.1.g4604-dirty