Build in merge
cover Build in merge
This series is a rewrite of git-merge in C.
I already sent a WIP version of this series, it already worked at that time, but I had numerous internal TODOs in the code.
I also tried to use the internal API wherever it was possible to avoid the expensive run_command() calls.
At the moment I am not aware of any bugs in ugly parts in the code, so I would appreciate if I could get some comments (positive or negative) on it.
And yes, I'm aware that it's already late for 1.5.6, I'm sending it out so that I can work on issues pointed out by others in the meantime.
Thanks.
Miklos Vajna (10):
Move split_cmdline() to alias.c
Move commit_list_count() to commit.c
Move builtin-remote's skip_prefix() to git-compat-util.h Add new test to ensure git-merge handles pull.twohead and
pull.octopus
parseopt: add a new PARSE_OPT_ARGV0_IS_AN_OPTION option Move read_cache_unmerged() to read-cache.c
git-fmt-merge-msg: make it useable from other builtins Introduce commit_list_append() in commit.c Introduce get_octopus_merge_bases() in commit.c Build in merge
Makefile | 2 +- alias.c | 54 ++ builtin-fmt-merge-msg.c | 157 ++-- builtin-merge-recursive.c | 8 - builtin-merge.c | 1147 +++++++++++++++++++++++++ builtin-read-tree.c | 24 - builtin-remote.c | 6 - builtin.h | 4 + cache.h | 3 + commit.c | 59 ++ commit.h | 3 + git-merge.sh => contrib/examples/git-merge.sh | 0 git-compat-util.h | 6 + git.c | 54 +-- parse-options.c | 19 +- parse-options.h | 1 + read-cache.c | 24 + t/t7601-merge-pull-config.sh | 57 ++ 18 files changed, 1460 insertions(+), 168 deletions(-) create mode 100644 builtin-merge.c rename git-merge.sh => contrib/examples/git-merge.sh (100%) create mode 100755 t/t7601-merge-pull-config.sh
PATCH 1/10 Move split_cmdline() to alias.c
split_cmdline() is currently used for aliases only, but later it can be useful for other builtins as well. Move it to alias.c for now, indicating that originally it's for aliases, but we'll have it in libgit this way.
--- alias.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ cache.h | 1 + git.c | 53 ----------------------------------------------------- 3 files changed, 55 insertions(+), 53 deletions(-)
alias.c +54 -0
@@ -21,3 +21,57 @@ char *alias_lookup(const char *alias)git_config (alias_lookup_cb,NULL );return alias_val; } + +int split_cmdline (char *cmdline,const char ***argv) +{ +int src, dst, count =0 , size =16 ; +char quoted =0 ; + + *argv =xmalloc (sizeof (char *) * size); + +/* split alias_string */ + (*argv)[count++] = cmdline; +for (src = dst =0 ; cmdline[src];) { +char c = cmdline[src]; +if (!quoted &&isspace (c)) { + cmdline[dst++] =0 ; +while (cmdline[++src] + &&isspace (cmdline[src])) + ;/* skip */ +if (count >= size) { + size +=16 ; + *argv =xrealloc (*argv,sizeof (char *) * size); + } + (*argv)[count++] = cmdline + dst; + }else if (!quoted && (c =='\'' || c =='"' )) { + quoted = c; + src++; + }else if (c == quoted) { + quoted =0 ; + src++; + }else { +if (c =='\\' && quoted !='\'' ) { + src++; + c = cmdline[src]; +if (!c) { +free (*argv); + *argv =NULL ; +return error ("cmdline ends with \\" ); + } + } + cmdline[dst++] = c; + src++; + } + } + + cmdline[dst] =0 ; + +if (quoted) { +free (*argv); + *argv =NULL ; +return error ("unclosed quote" ); + } + +return count; +} +
cache.h +1 -0
@@ -831,5 +831,6 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_void overlay_tree_on_cache (const char *tree_name,const char *prefix);char *alias_lookup (const char *alias); +int split_cmdline (char *cmdline,const char ***argv);#endif /* CACHE_H */
git.c +0 -53
59 lines not on this page · raw message
Signature · 1 line
1.5.6.rc0.dirty
PATCH 2/10 Move commit_list_count() to commit.c
This function is useful outside builtin-merge-recursive, for example in builtin-merge.
--- builtin-merge-recursive.c | 8 -------- commit.c | 8 ++++++++ commit.h | 1 + 3 files changed, 9 insertions(+), 8 deletions(-)
builtin-merge-recursive.c +0 -8
@@ -42,14 +42,6 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two)* - *(int *)commit->object.sha1 set to the virtual id. */ -static unsigned commit_list_count (const struct commit_list *l) -{ -unsigned c =0 ; -for (; l; l = l->next ) - c++; -return c; -} -static struct commit *make_virtual_commit (struct tree *tree,const char *comment) {struct commit *commit =xcalloc (1 ,sizeof (struct commit ));
commit.c +8 -0
@@ -331,6 +331,14 @@ struct commit_list *commit_list_insert(struct commit *item, struct commit_list *return new_list; } +unsigned commit_list_count (const struct commit_list *l) +{ +unsigned c =0 ; +for (; l; l = l->next ) + c++; +return c; +} +void free_commit_list (struct commit_list *list) {while (list) {
commit.h +1 -0
@@ -41,6 +41,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size);int parse_commit (struct commit *item);struct commit_list *commit_list_insert (struct commit *item,struct commit_list **list_p); +unsigned commit_list_count (const struct commit_list *l);struct commit_list *insert_by_date (struct commit *item,struct commit_list **list);void free_commit_list (struct commit_list *list);
Signature · 1 line
1.5.6.rc0.dirty
PATCH 3/10 Move builtin-remote's skip_prefix() to git-compat-util.h
builtin-remote.c and parse-options.c both have a skip_prefix() function, for the same purpose. Move builtin-remote's one to git-compat-util.h and let parse-options use it as well.
--- builtin-remote.c | 6 ------ git-compat-util.h | 6 ++++++ parse-options.c | 14 ++++---------- 3 files changed, 10 insertions(+), 16 deletions(-)
builtin-remote.c +0 -6
@@ -29,12 +29,6 @@ static inline int postfixcmp(const char *string, const char *postfix)return strcmp (string + len1 - len2, postfix); } -static inline const char *skip_prefix (const char *name,const char *prefix) -{ -return !name ?"" : -prefixcmp (name, prefix) ? name : name +strlen (prefix); -} -static int opt_parse_track (const struct option *opt,const char *arg,int not) {struct path_list *list = opt->value;
git-compat-util.h +6 -0
@@ -127,6 +127,12 @@ extern void set_warn_routine(void (*routine)(const char *warn, va_list params));extern int prefixcmp (const char *str,const char *prefix); +static inline const char *skip_prefix (const char *name,const char *prefix) +{ +return !name ?"" : +prefixcmp (name, prefix) ? name : name +strlen (prefix); +} +#ifdef NO_MMAP #ifndef PROT_READ
parse-options.c +4 -10
@@ -22,12 +22,6 @@ static inline const char *get_arg(struct optparse_t *p)return *++p->argv; } -static inline const char *skip_prefix (const char *str,const char *prefix) -{ -size_t len =strlen (prefix); -return strncmp (str, prefix, len) ?NULL : str + len; -} -static int opterror (const struct option *opt,const char *reason,int flags) {if (flags &OPT_SHORT )
@@ -161,7 +155,7 @@ static int parse_long_opt(struct optparse_t *p, const char *arg, rest =skip_prefix (arg, options->long_name);if (options->type ==OPTION_ARGUMENT ) { -if (!rest) +if (!strcmp (rest, arg))continue ;if (*rest =='=' )return opterror (options,"takes no value" , flags);
@@ -170,7 +164,7 @@ static int parse_long_opt(struct optparse_t *p, const char *arg, p->out[p->cpidx++] = arg -2 ;return 0 ; } -if (!rest) { +if (!strcmp (rest, arg)) {/* abbreviated? */ if (!strncmp (options->long_name, arg, arg_end - arg)) { is_abbreviated:
@@ -201,9 +195,9 @@ is_abbreviated: flags |=OPT_UNSET ; rest =skip_prefix (arg +3 , options->long_name);/* abbreviated and negated? */ -if (!rest && !prefixcmp (options->long_name, arg +3 )) +if (!strcmp (rest, arg +3 ) && !prefixcmp (options->long_name, arg +3 )) goto is_abbreviated; -if (!rest) +if (!strcmp (rest, arg +3 ))continue ; }if (*rest) {
Signature · 1 line
1.5.6.rc0.dirty
PATCH 4/10 Add new test to ensure git-merge handles pull.twohead and pull.octopus
Test if the given strategies are used and test the case when multiple strategies are configured using a space separated list.
--- t/t7601-merge-pull-config.sh | 57 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 57 insertions(+), 0 deletions(-) create mode 100755 t/t7601-merge-pull-config.sh
t/t7601-merge-pull-config.sh new file +57 -0
@@ -0,0 +1,57 @@ +#!/bin/sh + +test_description='git-merge + +Testing pull.* configuration parsing.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo c0 >c0.c && + git add c0.c && + git commit -m c0 && + git tag c0 && + echo c1 >c1.c && + git add c1.c && + git commit -m c1 && + git tag c1 && + git reset --hard c0 && + echo c2 >c2.c && + git add c2.c && + git commit -m c2 && + git tag c2 + git reset --hard c0 && + echo c3 >c3.c && + git add c3.c && + git commit -m c3 && + git tag c3 +' + +test_expect_success 'merge c1 with c2' ' + git reset --hard c1 && + git merge c2 && + test -e c1.c && + test -e c2.c +' + +test_expect_success 'merge c1 with c2 (ours in pull.twohead)' ' + git reset --hard c1 && + git config pull.twohead ours && + git merge c2 && + test -e c1.c && + ! test -e c2.c +' + +test_expect_success 'merge c1 with c2 and c3 (recursive in pull.octopus)' ' + git reset --hard c1 && + git config pull.octopus "recursive" && + ! git merge c2 c3 +' + +test_expect_success 'merge c1 with c2 and c3 (recursive and octopus in pull.octopus)' ' + git reset --hard c1 && + git config pull.octopus "recursive octopus" && + git merge c2 c3 +' + +test_done
Signature · 1 line
1.5.6.rc0.dirty
PATCH 5/10 parseopt: add a new PARSE_OPT_ARGV0_IS_AN_OPTION option
This new option tells parse-options not to ignore argv[0]. This is useful when argv cames from split_cmdline(), as in that case argv[0] contains a valuable option as well.
--- parse-options.c | 5 +++++ parse-options.h | 1 + 2 files changed, 6 insertions(+), 0 deletions(-)
parse-options.c +5 -0
@@ -249,6 +249,11 @@ int parse_options(int argc, const char **argv, const struct option *options, {struct optparse_t args = { argv +1 , argv, argc -1 ,0 ,NULL }; +if (flags &PARSE_OPT_ARGV0_IS_AN_OPTION ) { + args.argv = argv; + args.argc = argc; + } +for (; args.argc; args.argc--, args.argv++) {const char *arg = args.argv[0 ];
parse-options.h +1 -0
@@ -20,6 +20,7 @@ enum parse_opt_type {enum parse_opt_flags {PARSE_OPT_KEEP_DASHDASH =1 ,PARSE_OPT_STOP_AT_NON_OPTION =2 , +PARSE_OPT_ARGV0_IS_AN_OPTION =4 , };enum parse_opt_option_flags {
Signature · 1 line
1.5.6.rc0.dirty
PATCH 6/10 Move read_cache_unmerged() to read-cache.c
builtin-read-tree has a read_cache_unmerged() which is useful for other builtins, for example builtin-merge uses it as well. Move it to read-cache.c to avoid code duplication.
--- builtin-read-tree.c | 24 ------------------------ cache.h | 2 ++ read-cache.c | 24 ++++++++++++++++++++++++ 3 files changed, 26 insertions(+), 24 deletions(-)
builtin-read-tree.c +0 -24
@@ -29,30 +29,6 @@ static int list_tree(unsigned char *sha1)return 0 ; } -static int read_cache_unmerged (void ) -{ -int i; -struct cache_entry **dst; -struct cache_entry *last =NULL ; - -read_cache (); - dst = active_cache; -for (i =0 ; i < active_nr; i++) { -struct cache_entry *ce = active_cache[i]; -if (ce_stage (ce)) { -remove_name_hash (ce); -if (last && !strcmp (ce->name, last->name)) -continue ; -cache_tree_invalidate_path (active_cache_tree, ce->name); - last = ce; -continue ; - } - *dst++ = ce; - } - active_nr = dst - active_cache; -return !!last; -} -static void prime_cache_tree_rec (struct cache_tree *it,struct tree *tree) {struct tree_desc desc;
cache.h +2 -0
@@ -254,6 +254,7 @@ static inline void remove_name_hash(struct cache_entry *ce)#define read_cache () read_index(&the_index)#define read_cache_from (path) read_index_from(&the_index, (path)) +#define read_cache_unmerged () read_index_unmerged(&the_index)#define write_cache (newfd, cache, entries) write_index(&the_index, (newfd))#define discard_cache () discard_index(&the_index)#define unmerged_cache () unmerged_index(&the_index)
@@ -357,6 +358,7 @@ extern int init_db(const char *template_dir, unsigned int flags);/* Initialize and use the cache information */ extern int read_index (struct index_state *);extern int read_index_from (struct index_state *,const char *path); +extern int read_index_unmerged (struct index_state *);extern int write_index (const struct index_state *,int newfd);extern int discard_index (struct index_state *);extern int unmerged_index (const struct index_state *);
read-cache.c +24 -0
@@ -1394,3 +1394,27 @@ int write_index(const struct index_state *istate, int newfd) }return ce_flush (&c, newfd); } + +int read_index_unmerged (struct index_state *istate) +{ +int i; +struct cache_entry **dst; +struct cache_entry *last =NULL ; + +read_index (istate); + dst = istate->cache; +for (i =0 ; i < istate->cache_nr; i++) { +struct cache_entry *ce = istate->cache[i]; +if (ce_stage (ce)) { +remove_name_hash (ce); +if (last && !strcmp (ce->name, last->name)) +continue ; +cache_tree_invalidate_path (istate->cache_tree, ce->name); + last = ce; +continue ; + } + *dst++ = ce; + } + istate->cache_nr = dst - istate->cache; +return !!last; +}
Signature · 1 line
1.5.6.rc0.dirty
PATCH 7/10 git-fmt-merge-msg: make it useable from other builtins
Move all functionality (except config and option parsing) from cmd_fmt_merge_msg() to fmt_merge_msg(), so that other builtins can use it without a child process.
All functions have been changed to use strbufs, and now only cmd_fmt_merge_msg() reads directly from a file / writes anything to stdout.
--- builtin-fmt-merge-msg.c | 157 +++++++++++++++++++++++++++-------------------- builtin.h | 3 + 2 files changed, 94 insertions(+), 66 deletions(-)
builtin-fmt-merge-msg.c +91 -66
@@ -159,23 +159,24 @@ static int handle_line(char *line) }static void print_joined (const char *singular,const char *plural, -struct list *list) +struct list *list,struct strbuf *out) {if (list->nr ==0 )return ;if (list->nr ==1 ) { -printf ("%s%s" , singular, list->list[0 ]); +strbuf_addf (out,"%s%s" , singular, list->list[0 ]); }else {int i; -printf ("%s" , plural); +strbuf_addf (out,"%s" , plural);for (i =0 ; i < list->nr -1 ; i++) -printf ("%s%s" , i >0 ?", " :"" , list->list[i]); -printf (" and %s" , list->list[list->nr -1 ]); +strbuf_addf (out,"%s%s" , i >0 ?", " :"" , list->list[i]); +strbuf_addf (out," and %s" , list->list[list->nr -1 ]); } }static void shortlog (const char *name,unsigned char *sha1, -struct commit *head,struct rev_info *rev,int limit) +struct commit *head,struct rev_info *rev,int limit, +struct strbuf *out) {int i, count =0 ;struct commit *commit;
@@ -232,15 +233,15 @@ static void shortlog(const char *name, unsigned char *sha1, }if (count > limit) -printf ("\n* %s: (%d commits)\n" , name, count); +strbuf_addf (out,"\n* %s: (%d commits)\n" , name, count);else -printf ("\n* %s:\n" , name); +strbuf_addf (out,"\n* %s:\n" , name);for (i =0 ; i < subjects.nr; i++)if (i >= limit) -printf (" ...\n" ); +strbuf_addf (out," ...\n" );else -printf (" %s\n" , subjects.list[i]); +strbuf_addf (out," %s\n" , subjects.list[i]);clear_commit_marks ((struct commit *)branch, flags);clear_commit_marks (head, flags);
@@ -251,43 +252,13 @@ static void shortlog(const char *name, unsigned char *sha1,free_list (&subjects); } -int cmd_fmt_merge_msg (int argc,const char **argv,const char *prefix) -{ -int limit =20 , i =0 ; +int fmt_merge_msg (int merge_summary,struct strbuf *in,struct strbuf *out) { +int limit =20 , i =0 , pos =0 ;char line[1024 ]; -FILE *in = stdin; -const char *sep ="" ; +char *p = line, *sep ="" ;unsigned char head_sha1[20 ];const char *current_branch; -git_config (fmt_merge_msg_config,NULL ); - -while (argc >1 ) { -if (!strcmp (argv[1 ],"--log" ) || !strcmp (argv[1 ],"--summary" )) - merge_summary =1 ; -else if (!strcmp (argv[1 ],"--no-log" ) - || !strcmp (argv[1 ],"--no-summary" )) - merge_summary =0 ; -else if (!strcmp (argv[1 ],"-F" ) || !strcmp (argv[1 ],"--file" )) { -if (argc <3 ) -die ("Which file?" ); -if (!strcmp (argv[2 ],"-" )) - in = stdin; -else { -fclose (in); - in =fopen (argv[2 ],"r" ); -if (!in) -die ("cannot open %s" , argv[2 ]); - } - argc--; argv++; - }else -break ; - argc--; argv++; - } - -if (argc >1 ) -usage (fmt_merge_msg_usage); -/* get current branch */ current_branch =resolve_ref ("HEAD" , head_sha1,1 ,NULL );if (!current_branch)
@@ -295,75 +266,129 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)if (!prefixcmp (current_branch,"refs/heads/" )) current_branch +=11 ; -while (fgets (line,sizeof (line), in)) { +/* get a line */ +for (;;) { +int len; +char *newline; + +if (pos >= in->len) +break ; + p = in->buf + pos; + newline =strchr (p,'\n' ); + len = newline ? newline - p :strlen (p); + pos += len + !!newline; i++; -if (line[0 ] ==0 ) -continue ; -if (handle_line (line)) -die ("Error in line %d: %s" , i, line); + p[len] =0 ; +if (handle_line (p)) +die ("Error in line %d: %.*s" , i, len, p); } -printf ("Merge " ); +strbuf_addstr (out,"Merge " );for (i =0 ; i < srcs.nr; i++) {struct src_data *src_data = srcs.payload[i];const char *subsep ="" ; -printf (sep); +strbuf_addstr (out, sep); sep ="; " ;if (src_data->head_status ==1 ) { -printf (srcs.list[i]); +strbuf_addstr (out, srcs.list[i]);continue ; }if (src_data->head_status ==3 ) { subsep =", " ; -printf ("HEAD" ); +strbuf_addstr (out,"HEAD" ); }if (src_data->branch.nr) { -printf (subsep); +strbuf_addstr (out, subsep); subsep =", " ; -print_joined ("branch " ,"branches " , &src_data->branch); +print_joined ("branch " ,"branches " , &src_data->branch, + out); }if (src_data->r_branch.nr) { -printf (subsep); +strbuf_addstr (out, subsep); subsep =", " ;print_joined ("remote branch " ,"remote branches " , - &src_data->r_branch); + &src_data->r_branch, out); }if (src_data->tag.nr) { -printf (subsep); +strbuf_addstr (out, subsep); subsep =", " ; -print_joined ("tag " ,"tags " , &src_data->tag); +print_joined ("tag " ,"tags " , &src_data->tag, out); }if (src_data->generic.nr) { -printf (subsep); -print_joined ("commit " ,"commits " , &src_data->generic); +strbuf_addstr (out, subsep); +print_joined ("commit " ,"commits " , &src_data->generic, + out); }if (strcmp ("." , srcs.list[i])) -printf (" of %s" , srcs.list[i]); +strbuf_addf (out," of %s" , srcs.list[i]); }if (!strcmp ("master" , current_branch)) -putchar ('\n' ); +strbuf_addch (out,'\n' );else -printf (" into %s\n" , current_branch); +strbuf_addf (out," into %s\n" , current_branch);if (merge_summary) {struct commit *head;struct rev_info rev; head =lookup_commit (head_sha1); -init_revisions (&rev, prefix); +init_revisions (&rev,NULL ); rev.commit_format =CMIT_FMT_ONELINE ; rev.ignore_merges =1 ; rev.limited =1 ;for (i =0 ; i < origins.nr; i++)shortlog (origins.list[i], origins.payload[i], - head, &rev, limit); + head, &rev, limit, out); } +return 0 ; +} + +int cmd_fmt_merge_msg (int argc,const char **argv,const char *prefix) +{ +FILE *in = stdin; +struct strbuf input, output; +int ret; + +git_config (fmt_merge_msg_config,NULL ); + +while (argc >1 ) { +if (!strcmp (argv[1 ],"--log" ) || !strcmp (argv[1 ],"--summary" )) + merge_summary =1 ; +else if (!strcmp (argv[1 ],"--no-log" ) + || !strcmp (argv[1 ],"--no-summary" )) + merge_summary =0 ; +else if (!strcmp (argv[1 ],"-F" ) || !strcmp (argv[1 ],"--file" )) { +if (argc <3 ) +die ("Which file?" ); +if (!strcmp (argv[2 ],"-" )) + in = stdin; +else { +fclose (in); + in =fopen (argv[2 ],"r" ); +if (!in) +die ("cannot open %s" , argv[2 ]); + } + argc--; argv++; + }else +break ; + argc--; argv++; + } + +if (argc >1 ) +usage (fmt_merge_msg_usage); -/* No cleanup yet; is standalone anyway */ +strbuf_init (&input,0 ); +if (strbuf_read (&input,fileno (in),0 ) <0 ) +die ("could not read input file %s" ,strerror (errno)); +strbuf_init (&output,0 ); + ret =fmt_merge_msg (merge_summary, &input, &output); +if (ret) +return ret; +printf ("%s" , output.buf);return 0 ; }
builtin.h +3 -0
@@ -2,6 +2,7 @@#define BUILTIN_H #include "git-compat-util.h" +#include "strbuf.h" extern const char git_version_string[];extern const char git_usage_string[];
@@ -10,6 +11,8 @@ extern void list_common_cmds_help(void);extern void help_unknown_cmd (const char *cmd);extern void prune_packed_objects (int );extern int read_line_with_nul (char *buf,int size,FILE *file); +extern int fmt_merge_msg (int merge_summary,struct strbuf *in, +struct strbuf *out);extern int cmd_add (int argc,const char **argv,const char *prefix);extern int cmd_annotate (int argc,const char **argv,const char *prefix);
Signature · 1 line
1.5.6.rc0.dirty
Re: [PATCH 07/10] git-fmt-merge-msg: make it useable from other builtins
Quoted 9 lines · Miklos Vajna
Move all functionality (except config and option parsing) from
cmd_fmt_merge_msg()tofmt_merge_msg(), so that other builtins can use it without a child process.All functions have been changed to use strbufs, and now only
cmd_fmt_merge_msg()reads directly from a file / writes anything to stdout.Signed-off-by Miklos Vajna <[email protected]>
The word in the title should be "usable" (s/sea/sa/).
Other than that, looks like a quite straightforward conversion.
Re: [PATCH 07/10] git-fmt-merge-msg: make it useable from other builtins
Quoted 3 lines · Junio C Hamano
The word in the title should be "usable" (s/sea/sa/).
Other than that, looks like a quite straightforward conversion.
OK, I fixed up the patch title in git://repo.or.cz/git/vmiklos.git.
PATCH 8/10 Introduce commit_list_append() in commit.c
In reply to Miklos VajnaThis is like commit_list_insert() but it appends the new commit to the end of the list, rather than insert to the start of it.
--- commit.c | 19 +++++++++++++++++++ commit.h | 1 + 2 files changed, 20 insertions(+), 0 deletions(-)
commit.c +19 -0
@@ -331,6 +331,25 @@ struct commit_list *commit_list_insert(struct commit *item, struct commit_list *return new_list; } +struct commit_list *commit_list_append (struct commit *item, +struct commit_list **list_p) +{ +struct commit_list *i, *prev =NULL , *list = *list_p; +struct commit_list *new_list =xmalloc (sizeof (struct commit_list )); + + new_list->item = item; + new_list->next =NULL ; + +if (!list) + *list_p = new_list; +else { +for (i = list; i; i = i->next) + prev = i; + prev->next = new_list; + } +return list; +} +unsigned commit_list_count (const struct commit_list *l) {unsigned c =0 ;
commit.h +1 -0
@@ -41,6 +41,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size);int parse_commit (struct commit *item);struct commit_list *commit_list_insert (struct commit *item,struct commit_list **list_p); +struct commit_list *commit_list_append (struct commit *item,struct commit_list **list_p);unsigned commit_list_count (const struct commit_list *l);struct commit_list *insert_by_date (struct commit *item,struct commit_list **list);
Signature · 1 line
1.5.6.rc0.dirty
Re: [PATCH 08/10] Introduce commit_list_append() in commit.c
Quoted 25 lines · Miklos Vajna
This is like
commit_list_insert()but it appends the new commit to the end of the list, rather than insert to the start of it.Signed-off-by Miklos Vajna <[email protected]>--- commit.c | 19 +++++++++++++++++++ commit.h | 1 + 2 files changed, 20 insertions(+), 0 deletions(-)commit.c +8
-0@@ -331,6 +331,25 @@ struct commit_list *commit_list_insert(struct commit *item, struct commit_list *return new_list; } +struct commit_list *commit_list_append (struct commit *item, +struct commit_list **list_p) +{ +struct commit_list *i, *prev =NULL , *list = *list_p; +struct commit_list *new_list =xmalloc (sizeof (struct commit_list )); + + new_list->item = item; + new_list->next =NULL ;
commit.c struct commit_list *commit_list_insert(struct commit *item, struct commit_list * · line 351
Quoted 10 lines
commit.c +10
-0+ +if (!list) + *list_p = new_list; +else { +for (i = list; i; i = i->next) + prev = i; + prev->next = new_list; + } +return list; +}
Do you have a caller of this function that keeps a pointer to commit_list that needs to be appended at the tail or inserted at the beginning depending on the phase of the moon, or does the caller always append to that list?
Re: [PATCH 08/10] Introduce commit_list_append() in commit.c
Quoted 4 lines · Junio C Hamano
Do you have a caller of this function that keeps a pointer to
commit_listthat needs to be appended at the tail or inserted at the beginning depending on the phase of the moon, or does the caller always append to that list?
The later. I use it for appending a new parent for the merge commit and after parsing a new remote head. In both cases I always append a list.
Re: [PATCH 08/10] Introduce commit_list_append() in commit.c
Quoted 8 lines · Miklos Vajna
Quoted 4 lines · Junio C Hamano
Do you have a caller of this function that keeps a pointer to
commit_listthat needs to be appended at the tail or inserted at the beginning depending on the phase of the moon, or does the caller always append to that list?The later. I use it for appending a new parent for the merge commit and after parsing a new remote head. In both cases I always append a list.
If that is the case, you might want to check how parse_commit_buffer() in commit.c builds "parents" list. It is a standard pattern to append to the list using commit_list_insert() and that is the reason why it returns a pointer.
Re: [PATCH 08/10] Introduce commit_list_append() in commit.c
Quoted 4 lines · Junio C Hamano
If that is the case, you might want to check how
parse_commit_buffer()incommit.cbuilds "parents" list. It is a standard pattern to append to the list usingcommit_list_insert()and that is the reason why it returns a pointer.
Ah, great, then this patch is not necessary. I removed it from my working branch.
Thanks.
PATCH 9/10 Introduce get_octopus_merge_bases() in commit.c
In reply to Miklos VajnaThis is like get_merge_bases() but it works for multiple heads, like show-branch --merge-base.
--- commit.c | 32 ++++++++++++++++++++++++++++++++ commit.h | 1 + 2 files changed, 33 insertions(+), 0 deletions(-)
commit.c +32 -0
@@ -625,6 +625,38 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)return result; } +struct commit_list *get_octopus_merge_bases (struct commit_list *in,int cleanup) +{ +struct commit_list *i, *j, *k, *ret =NULL ; + +for (i = in; i; i = i->next) { +if (!ret) +commit_list_append (i->item, &ret); +else { +struct commit_list *new =NULL , *end =NULL ; + +for (j = ret; j; j = j->next) { +struct commit_list *bases; + bases =get_merge_bases (i->item, j->item, cleanup); +/* +* Now we just append bases to new, but +* calling commit_list_append() for each +* item would be expensive, so do it by +* hand. +*/ +if (!new) + new = bases; +else + end->next = bases; +for (k = bases; k; k = k->next) + end = k; + } + ret = new; + } + } +return ret; +} +struct commit_list *get_merge_bases (struct commit *one,struct commit *two,int cleanup) {
commit.h +1 -0
@@ -122,6 +122,7 @@ int read_graft_file(const char *graft_file);struct commit_graft *lookup_commit_graft (const unsigned char *sha1);extern struct commit_list *get_merge_bases (struct commit *rev1,struct commit *rev2,int cleanup); +extern struct commit_list *get_octopus_merge_bases (struct commit_list *in,int cleanup);extern int register_shallow (const unsigned char *sha1);extern int unregister_shallow (const unsigned char *sha1);
Signature · 1 line
1.5.6.rc0.dirty
PATCH 10/10 Build in merge
--- Makefile | 2 +- builtin-merge.c | 1147 +++++++++++++++++++++++++ builtin.h | 1 + git-merge.sh => contrib/examples/git-merge.sh | 0 git.c | 1 + 5 files changed, 1150 insertions(+), 1 deletions(-) create mode 100644 builtin-merge.c rename git-merge.sh => contrib/examples/git-merge.sh (100%)
Makefile +1 -1
@@ -240,7 +240,6 @@ SCRIPT_SH += git-lost-found.sh SCRIPT_SH += git-merge-octopus.sh SCRIPT_SH += git-merge-one-file.sh SCRIPT_SH += git-merge-resolve.sh -SCRIPT_SH += git-merge.sh SCRIPT_SH += git-merge-stupid.sh SCRIPT_SH += git-mergetool.sh SCRIPT_SH += git-parse-remote.sh
@@ -511,6 +510,7 @@ BUILTIN_OBJS += builtin-ls-remote.o BUILTIN_OBJS += builtin-ls-tree.o BUILTIN_OBJS += builtin-mailinfo.o BUILTIN_OBJS += builtin-mailsplit.o +BUILTIN_OBJS += builtin-merge.o BUILTIN_OBJS += builtin-merge-base.o BUILTIN_OBJS += builtin-merge-file.o BUILTIN_OBJS += builtin-merge-ours.o
builtin-merge.c new file +1147 -0
1,147 lines not on this page · raw message
builtin.h +1 -0
@@ -59,6 +59,7 @@ extern int cmd_ls_tree(int argc, const char **argv, const char *prefix);extern int cmd_ls_remote (int argc,const char **argv,const char *prefix);extern int cmd_mailinfo (int argc,const char **argv,const char *prefix);extern int cmd_mailsplit (int argc,const char **argv,const char *prefix); +extern int cmd_merge (int argc,const char **argv,const char *prefix);extern int cmd_merge_base (int argc,const char **argv,const char *prefix);extern int cmd_merge_ours (int argc,const char **argv,const char *prefix);extern int cmd_merge_file (int argc,const char **argv,const char *prefix);
contrib/examples/git-merge.sh renamed from git-merge.sh +0 -0
git.c +1 -0
@@ -268,6 +268,7 @@ static void handle_internal_command(int argc, const char **argv) { "ls-remote", cmd_ls_remote }, { "mailinfo", cmd_mailinfo }, { "mailsplit", cmd_mailsplit }, + { "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE }, { "merge-base", cmd_merge_base, RUN_SETUP }, { "merge-file", cmd_merge_file }, { "merge-ours", cmd_merge_ours, RUN_SETUP },
Signature · 1 line
1.5.6.rc0.dirty
Re: [PATCH 09/10] Introduce get_octopus_merge_bases() in commit.c
In reply to Miklos VajnaQuoted 2 lines · Miklos Vajna
This is like
get_merge_bases()but it works for multiple heads, like show-branch --merge-base.
In what sense is this "like show-branch --merge-base"?
The only similarlity I can spot is that it can take more than two heads, but what it computes and the way it computes it seem to be different. It certainly looks much less efficient as it does not walk the ancestry chain in one-go like show-branch does.
I haven't looked at [10/10] so I do not know yet how you intend to use this value, but I am not sure if it makes sense to use the computed value as the merge base when running an octopus.
When you want to merge A, B and C, because internally we always do pairwise, you would first compute:
T(A,B) == merge_3way(mb(A,B), A, B)
and then you would want to pick some merge base to merge that result with C:
T(A,B,C) == merge_3way(???, T(A,B), C)
I do not think using mb(mb(A,B),C) as ??? is necessary nor optimal.
o---o---A
/ .
----1---2---B...T(A,B)
\ .
o---C...T(A,B,C)"1" above is mb(A,B) (i.e. the merge base between A and B) that you would use when you merge A and B to compute an internal merge result T(A,B). And "1" also is mb(mb(A,B),C).
But if you are doing 3-way merge to arrive at T(A,B,C) by merging T(A,B) and C, don't you want to be using "2" as the merge base, not "1"?
The big comment on $MRC at the end of git-merge-octopus is about this issue. In earlier implementation, we used to use "1" to create T(A,B) and then also used it to update the variable $MRC, which is used to compute the merge base to use when the tentative tree T(A,B) is merged with the other remote in the next round. It was a mistake.
Ideally we should pick a better base between mb(A,C) and mb(B,C) (and if we used mb(B,C) that's "2" and is a much better base than "1" when merging T(A,B) and C). Using mb(mb(A,B),C) means we are guaranteed to pick the worst base for that last merge.
Re: [PATCH 09/10] Introduce get_octopus_merge_bases() in commit.c
Quoted 6 lines · Junio C Hamano
In what sense is this "like show-branch --merge-base"?
The only similarlity I can spot is that it can take more than two heads, but what it computes and the way it computes it seem to be different. It certainly looks much less efficient as it does not walk the ancestry chain in one-go like show-branch does.
Exactly. The original idea was to avoid the current limitation:
show-branch uses object flags for each branch, which is not something that works if you have a lot of ones.
Re: [PATCH 09/10] Introduce get_octopus_merge_bases() in commit.c
In reply to Junio C HamanoQuoted 31 lines · Junio C Hamano
When you want to merge A, B and C, because internally we always do pairwise, you would first compute:
T(A,B) == merge_3way(mb(A,B), A, B)and then you would want to pick some merge base to merge that result with C:
T(A,B,C) == merge_3way(???, T(A,B), C)I do not think using mb(mb(A,B),C) as ??? is necessary nor optimal.
o---o---A / . ----1---2---B...T(A,B) \ . o---C...T(A,B,C)"1" above is
mb(A,B)(i.e. the merge base between A and B) that you would use when you merge A and B to compute an internal merge resultT(A,B). And "1" also is mb(mb(A,B),C).But if you are doing 3-way merge to arrive at
T(A,B,C)by mergingT(A,B)and C, don't you want to be using "2" as the merge base, not "1"?The big comment on $MRC at the end of git-merge-octopus is about this issue. In earlier implementation, we used to use "1" to create
T(A,B)and then also used it to update the variable $MRC, which is used to compute the merge base to use when the tentative treeT(A,B)is merged with the other remote in the next round. It was a mistake.
Quoted 4 lines
Ideally we should pick a better base between
mb(A,C)andmb(B,C)(and if we usedmb(B,C)that's "2" and is a much better base than "1" when mergingT(A,B)and C). Using mb(mb(A,B),C) means we are guaranteed to pick the worst base for that last merge.
Something like this might be enough to get us started. Instead of finding out the merge bases between two commits, "one" and "two", this would find merge base between "one" and a commit that would be a merge across all the "two"s. To apply it to the above example, you would give "C" as "one", and "A" and "B" as "twos" to it, to compute "2".
commit.c | 32 ++++++++++++++++++++++---------- 1 files changed, 22 insertions(+), 10 deletions(-)
commit.c +22 -10
@@ -531,26 +531,33 @@ static struct commit *interesting(struct commit_list *list)return NULL ; } -static struct commit_list *merge_bases (struct commit *one,struct commit *two) +static struct commit_list *merge_bases_many (struct commit *one,int n,struct commit **twos) {struct commit_list *list =NULL ;struct commit_list *result =NULL ; +int i; -if (one == two) -/* We do not mark this even with RESULT so we do not -* have to clean it up. -*/ -return commit_list_insert (one, &result); +for (i =0 ; i < n; i++) { +if (one == twos[i]) +/* We do not mark this even with RESULT so we do not +* have to clean it up. +*/ +return commit_list_insert (one, &result); + }if (parse_commit (one))return NULL ; -if (parse_commit (two)) -return NULL ; +for (i =0 ; i < n; i++) { +if (parse_commit (twos[i])) +return NULL ; + } one->object.flags |=PARENT1 ; - two->object.flags |=PARENT2 ;insert_by_date (one, &list); -insert_by_date (two, &list); +for (i =0 ; i < n; i++) { + twos[i]->object.flags |=PARENT2 ; +insert_by_date (twos[i], &list); + }while (interesting (list)) {struct commit *commit;
@@ -598,6 +605,11 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)return result; } +static struct commit_list *merge_bases (struct commit *one,struct commit *two) +{ +return merge_bases_many (one,1 , &two); +} +struct commit_list *get_merge_bases (struct commit *one,struct commit *two,int cleanup) {
PATCH 1/1 Introduce get_octopus_merge_bases() in commit.c
This is like get_merge_bases() but it works for multiple heads. Internally it uses merge_bases_many() but it has the ability to clear commit marks like get_merge_bases().
---
Quoted 9 lines · Junio C Hamano
Something like this might be enough to get us started. Instead of finding
out the merge bases between two commits, "one" and "two", this would find
merge base between "one" and a commit that would be a merge across all the
"two"s. To apply it to the above example, you would give "C" as "one",
and "A" and "B" as "twos" to it, to compute "2".
Here is what I pushed to my working branch, on top of your patch.
BTW: How should I credit your patch? Add a Signed-off-by? If I make you the author I don't know how send-email handles the situation.
Thanks.
commit.c | 17 +++++++++++++++++ commit.h | 1 + 2 files changed, 18 insertions(+), 0 deletions(-)
commit.c +17 -0
@@ -618,6 +618,23 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)return merge_bases_many (one,1 , &two); } +struct commit_list *get_octopus_merge_bases (struct commit_list *in,int cleanup) +{ +struct commit *head = in->item; +int n =0 ; +struct commit **commits =xmalloc (n*sizeof (struct commit *)); +struct commit_list *ret, *i; + +for (i = in; i; i = i->next, n++) + commits[n] = i->item; + ret =merge_bases_many (head, n, commits); +free (commits); +if (cleanup) +for (i = in; i; i = i->next) +clear_commit_marks (i->item, all_flags); +return ret; +} +struct commit_list *get_merge_bases (struct commit *one,struct commit *two,int cleanup) {
commit.h +1 -0
@@ -121,6 +121,7 @@ int read_graft_file(const char *graft_file);struct commit_graft *lookup_commit_graft (const unsigned char *sha1);extern struct commit_list *get_merge_bases (struct commit *rev1,struct commit *rev2,int cleanup); +extern struct commit_list *get_octopus_merge_bases (struct commit_list *in,int cleanup);extern int register_shallow (const unsigned char *sha1);extern int unregister_shallow (const unsigned char *sha1);
Signature · 1 line
1.5.6.rc0.dirty
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
Hi,
commit.c static struct commit_list *merge_bases(struct commit *one, struct commit *two) · line 625
Quoted 13 lines · Miklos Vajna
commit.c +5
-0@@ -618,6 +618,23 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)return merge_bases_many (one,1 , &two); } +struct commit_list *get_octopus_merge_bases (struct commit_list *in,int cleanup) +{ +struct commit *head = in->item; +int n =0 ; +struct commit **commits =xmalloc (n*sizeof (struct commit *));
Here, n will be 0 and therefore commits will be xmalloc(0), right?
commit.c static struct commit_list *merge_bases(struct commit *one, struct commit *two) · line 629
Quoted 4 lines
commit.c +4
-0+struct commit_list *ret, *i; + +for (i = in; i; i = i->next, n++) + commits[n] = i->item;
And here, commits will never be realloc()ed.
commit.c static struct commit_list *merge_bases(struct commit *one, struct commit *two) · line 630
Quoted 1 line
commit.c +1
-0+ ret =merge_bases_many (head, n, commits);
If merge_bases_many took a commit_list (yes, as I suggested to Junio), this transformation would not be necessary.
IIRC nothing in merge_bases_many() needed a commit array.
Oh, and whose responsibility is it to free "in"? Caller or callee? (Because it is a non-const parameter, I would have expected the callee, but I think it makes more sense if the caller can do whatever she wants with the heads after calling octopus_merge_bases()).
Ciao,
Dscho
PATCH 1/1 Introduce get_octopus_merge_bases() in commit.c
This is like get_merge_bases() but it works for multiple heads. Internally it uses merge_bases_many() but it has the ability to clear commit marks like get_merge_bases().
---
Quoted 11 lines · Johannes Schindelin
Review
Quoted 2 lines
(quoted diff) +2
-0+ int n = 0; + struct commit **commits = xmalloc(n*sizeof(struct commit *));Here, n will be 0 and therefore commits will be xmalloc(0), right?
Review
Quoted 4 lines
(quoted diff) +4
-0+ struct commit_list *ret, *i; + + for(i = in; i; i = i->next, n++) + commits[n] = i->item;And here, commits will never be realloc()ed.
Ouch, yes.
Quoted 6 lines
Quoted 1 line
+ ret =
merge_bases_many(head, n, commits);If
merge_bases_manytook acommit_list(yes, as I suggested to Junio), this transformation would not be necessary.IIRC nothing in
merge_bases_many()needed a commit array.
Exactly. I modified merge_bases_many() to use a commit list.
Quoted 6 lines
Oh, and whose responsibility is it to free "in"? Caller or callee? (Because it is a non-const parameter, I would have expected the callee,
but I think it makes more sense if the caller can do whatever she wants
with the heads after callingoctopus_merge_bases()).
The previous. I think using const would be a mistake since we do modify the flags, but I still would prefer free the list in the caller, since the list is still usable. That's exactly why the cleanup flag is there:
in case the non-empty objects flags would cause any problem later in the caller.
commit.c | 11 +++++++++++ commit.h | 1 + 2 files changed, 12 insertions(+), 0 deletions(-)
commit.c +11 -0
@@ -615,6 +615,17 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)return merge_bases_many (one, list); } +struct commit_list *get_octopus_merge_bases (struct commit_list *in,int cleanup) +{ +struct commit_list *ret, *i; + + ret =merge_bases_many (in->item, in->next); +if (cleanup) +for (i = in; i; i = i->next) +clear_commit_marks (i->item, all_flags); +return ret; +} +struct commit_list *get_merge_bases (struct commit *one,struct commit *two,int cleanup) {
commit.h +1 -0
@@ -121,6 +121,7 @@ int read_graft_file(const char *graft_file);struct commit_graft *lookup_commit_graft (const unsigned char *sha1);extern struct commit_list *get_merge_bases (struct commit *rev1,struct commit *rev2,int cleanup); +extern struct commit_list *get_octopus_merge_bases (struct commit_list *in,int cleanup);extern int register_shallow (const unsigned char *sha1);extern int unregister_shallow (const unsigned char *sha1);
Signature · 1 line
1.5.6.rc2.dirty
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
commit.c static struct commit_list *merge_bases(struct commit *one, struct commit *two) · line 627
Quoted 10 lines · Miklos Vajna
commit.c +10
-0+struct commit_list *get_octopus_merge_bases (struct commit_list *in,int cleanup) +{ +struct commit_list *ret, *i; + + ret =merge_bases_many (in->item, in->next); +if (cleanup) +for (i = in; i; i = i->next) +clear_commit_marks (i->item, all_flags); +return ret; +}
I suspect either me or you are confused, but how is this exactly used?
The code for merge_bases_many(), at least the one I showed you a few days ago, is not a replacement for "show-branch --merge-base", and I do not think you would want to use it as such in the rewrite of git-merge, if you are trying to replace this part of git-merge.sh:
case "$#" in
1)
common=$(git merge-base --all $head "$@")
;;
*)
common=$(git show-branch --merge-base $head "$@")
;;
esacThe purpose of merge-base-many code was to improve this line in the git-merge-octopus.sh:
common=$(git merge-base --all $MRC $SHA1) || die "Unable to find common commit with $SHA1"
Instead of keeping a single MRC, we can compute the merge-base-many between the SHA1 (i.e. the one we are looking at right now -- it is fed as "one") and all the previous SHA1's we have already looked at (they become "two's"), like this:
common($git merge-base-many $SHA1 $SHA1_SO_FAR)
and you would have at the end of the loop:
SHA1_SO_FAR="$SHA1_SO_FAR$SHA1 "
or something.
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
Quoted 20 lines · Junio C Hamano
commit.c static struct commit_list *merge_bases(struct commit *one, struct commit *two) · line 627
Quoted 10 lines · Miklos Vajna
commit.c +10
-0+struct commit_list *get_octopus_merge_bases (struct commit_list *in,int cleanup) +{ +struct commit_list *ret, *i; + + ret =merge_bases_many (in->item, in->next); +if (cleanup) +for (i = in; i; i = i->next) +clear_commit_marks (i->item, all_flags); +return ret; +}I suspect either me or you are confused, but how is this exactly used?
The code for
merge_bases_many(), at least the one I showed you a few days ago, is not a replacement for "show-branch --merge-base", and I do not think you would want to use it as such in the rewrite of git-merge, if you are trying to replace this part ofgit-merge.sh:
Quoted 8 lines
case "$#" in
1)
common=$(git merge-base --all $head "$@")
;;
*)
common=$(git show-branch --merge-base $head "$@")
;;
esacThen I think I'm the one who is confused. My original get_octopus_merge_bases() - in commit e13c0c2 - was exactly to replace this part.
So, may I assume that the original get_octopus_merge_bases() is OK for this purpose?
Quoted 11 lines
The purpose of merge-base-many code was to improve this line in the
git-merge-octopus.sh:common=$(git merge-base --all $MRC $SHA1) || die "Unable to find common commit with $SHA1"Instead of keeping a single MRC, we can compute the merge-base-many between the SHA1 (i.e. the one we are looking at right now -- it is fed as "one") and all the previous SHA1's we have already looked at (they become "two's"), like this:
Quoted 7 lines
common($git merge-base-many $SHA1 $SHA1_SO_FAR)and you would have at the end of the loop:
SHA1_SO_FAR="$SHA1_SO_FAR$SHA1 "or something.
Actually I do not want to touch git-merge-octopus.sh before builtin-merge is not ready. (I try to "do one thing and do it well".)
Thanks.
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
In reply to Junio C HamanoHi,
Quoted 40 lines · Junio C Hamano
commit.c static struct commit_list *merge_bases(struct commit *one, struct commit *two) · line 627
Quoted 10 lines · Miklos Vajna
commit.c +10
-0+struct commit_list *get_octopus_merge_bases (struct commit_list *in,int cleanup) +{ +struct commit_list *ret, *i; + + ret =merge_bases_many (in->item, in->next); +if (cleanup) +for (i = in; i; i = i->next) +clear_commit_marks (i->item, all_flags); +return ret; +}I suspect either me or you are confused, but how is this exactly used?
The code for
merge_bases_many(), at least the one I showed you a few days ago, is not a replacement for "show-branch --merge-base", and I do not think you would want to use it as such in the rewrite of git-merge, if you are trying to replace this part ofgit-merge.sh:case "$#" in 1) common=$(git merge-base --all $head "$@") ;; *) common=$(git show-branch --merge-base $head "$@") ;; esacThe purpose of merge-base-many code was to improve this line in the
git-merge-octopus.sh:common=$(git merge-base --all $MRC $SHA1) || die "Unable to find common commit with $SHA1"Instead of keeping a single MRC, we can compute the merge-base-many between the SHA1 (i.e. the one we are looking at right now -- it is fed as "one") and all the previous SHA1's we have already looked at (they become "two's"), like this:
Quoted 7 lines
common($git merge-base-many $SHA1 $SHA1_SO_FAR)and you would have at the end of the loop:
SHA1_SO_FAR="$SHA1_SO_FAR$SHA1 "or something.
Ah! Even I thought that merge_bases_many() was meant for the show-branch --merge-base case.
However, your remark about optimizing for the two-head case got me
thinking: should we not rather use the simple algorithm Miklos proposed for octopus_merge_bases(), even if it is suboptimal?
Octopus is such a rare case that it is more important to have a robust, working code, than a fast one, right? Especially since Octopus will be exercized much less often, and therefore has a higher chance of hiding a bug.
Ciao,
Dscho
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
Quoted 3 lines · Johannes Schindelin
However, your remark about optimizing for the two-head case got me
thinking: should we not rather use the simple algorithm Miklos proposed foroctopus_merge_bases(), even if it is suboptimal?
Actually a quick glance at git-merge, a rather large case...esac after that "show-branch --merge-base" tells me that we do not really use the output from that operation and instead we check if we are fast-forward from all the other heads by iterating over them. merge-octupos would accept it as the base but never looks at it.
So I think with a proper refactoring of the codepath you probably would not even have to do the blanket "merge-base across all heads" at all.
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
Quoted 5 lines · Junio C Hamano
Actually a quick glance at git-merge, a rather large case...esac after that "show-branch --merge-base" tells me that we do not really use the output from that operation and instead we check if we are fast-forward from all the other heads by iterating over them. merge-octupos would accept it as the base but never looks at it.
I may be wrong but I think it would be still nice to pass a valid base to the backend, even if _currently_ the only octopus implementation ignores it.
Actually on "valid" I mean what my original get_octopus_merge_bases() returned. :-)
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
Hi,
Quoted 9 lines · Miklos Vajna
Quoted 5 lines · Junio C Hamano
Actually a quick glance at git-merge, a rather large case...esac after that "show-branch --merge-base" tells me that we do not really use the output from that operation and instead we check if we are fast-forward from all the other heads by iterating over them. merge-octupos would accept it as the base but never looks at it.
Quoted 6 lines
I may be wrong but I think it would be still nice to pass a valid base to the backend, even if _currently_ the only octopus implementation ignores it.
Actually on "valid" I mean what my original
get_octopus_merge_bases()returned. :-)
IMO that is one of the advantages of making git-merge a builtin: you need not work around the lack of performance of shell scripts, but you can actually compute what you need to compute.
IOW I agree that we could give the proper list of merge bases to whatever octopus backend we use. And since your original implementation is correct, if not super-optimal, I would like to stick to it.
Ciao,
Dscho
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
In reply to Miklos VajnaQuoted 1 line · Miklos Vajna
Exactly. I modified
merge_bases_many()to use a commit list.
I just looked at your f755fb6; from the point of view of merge_bases() which is far more often used, I think this is not an improvement but actively wrong thing to do. Most of the time callers compute merge base of two, and the codepath should be optimized for that case.
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
Hi,
Quoted 8 lines · Junio C Hamano
Quoted 1 line · Miklos Vajna
Exactly. I modified
merge_bases_many()to use a commit list.I just looked at your f755fb6; from the point of view of
merge_bases()which is far more often used, I think this is not an improvement but actively wrong thing to do. Most of the time callers compute merge base of two, and the codepath should be optimized for that case.
But how much work is
struct commit_list list = { two, NULL };
merge_bases_many(one, &list);? merge_bases_many() does not modify the list IIRC, and it looks conceptually clean while being pretty much in the same performance ballpark.
Ciao,
Dscho
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
Quoted 11 lines · Johannes Schindelin
Quoted 8 lines · Junio C Hamano
Quoted 1 line · Miklos Vajna
Exactly. I modified
merge_bases_many()to use a commit list.I just looked at your f755fb6; from the point of view of
merge_bases()which is far more often used, I think this is not an improvement but actively wrong thing to do. Most of the time callers compute merge base of two, and the codepath should be optimized for that case.
Quoted 5 lines
But how much work is
struct commit_list list = { two, NULL }; merge_bases_many(one, &list);
But (1) that is not what f755fb6 does; it does commit_list_insert() that has an allocation, and (2) how much work is to count, allocate and pass array for slow and less frequent codepath which octopus-merge-base is?
Re: [PATCH] Introduce get_octopus_merge_bases() in commit.c
Quoted 7 lines · Junio C Hamano
Quoted 3 lines
struct commit_list list = { two, NULL }; merge_bases_many(one, &list);But (1) that is not what f755fb6 does; it does
commit_list_insert()that has an allocation, and (2) how much work is to count, allocate and pass array for slow and less frequent codepath which octopus-merge-base is?
If (1) is the only problem, I can do it the way Dscho suggested, but I don't exactly see why using a commit_list* can slow merge_bases() down:
for (i = 0; i < n; i++) {
...
}is probably as fast as
for (i = list; i; i = i->next) {
...
}in case n = 0 / list->next is NULL.
Re: [PATCH 09/10] Introduce get_octopus_merge_bases() in commit.c
In reply to Junio C HamanoHi,
commit.c static struct commit *interesting(struct commit_list *list) · line 534
Quoted 10 lines · Junio C Hamano
commit.c +1
-1@@ -531,26 +531,33 @@ static struct commit *interesting(struct commit_list *list)return NULL ; } -static struct commit_list *merge_bases (struct commit *one,struct commit *two) +static struct commit_list *merge_bases_many (struct commit *one,int n,struct commit **twos)
Clever, but for performance reasons I think it might be better to use a commit list here. This can be a local commit_list in the helper that calls merge_bases_many() for the two-head case.
Quoted 16 lines
commit.c +7
-5{struct commit_list *list =NULL ;struct commit_list *result =NULL ; +int i; -if (one == two) -/* We do not mark this even with RESULT so we do not -* have to clean it up. -*/ -return commit_list_insert (one, &result); +for (i =0 ; i < n; i++) { +if (one == twos[i]) +/* We do not mark this even with RESULT so we do not +* have to clean it up. +*/ +return commit_list_insert (one, &result);
commit.c static struct commit *interesting(struct commit_list *list) · line 553
Quoted 10 lines
commit.c +5
-2+ }if (parse_commit (one))return NULL ; -if (parse_commit (two)) -return NULL ; +for (i =0 ; i < n; i++) { +if (parse_commit (twos[i])) +return NULL ; + }
Distracting curly brackets.
commit.c static struct commit *interesting(struct commit_list *list) · line 560
Quoted 8 lines
commit.c +4
-2one->object.flags |=PARENT1 ; - two->object.flags |=PARENT2 ;insert_by_date (one, &list); -insert_by_date (two, &list); +for (i =0 ; i < n; i++) { + twos[i]->object.flags |=PARENT2 ; +insert_by_date (twos[i], &list); + }
Ah, now that is clever: I thought we would get into a lot of problems because we would need a bit for every initial commit. But what you are basically doing is reusing PARENT2 for all the merge bases that have been found for the heads up to, but not including, the current one.
Maybe this should be documented.
But this is not yet enough, right? You still have to have a loop
merge_bases <- calculate merge bases from the first two heads
foreach head starting from the 3rd one
merge_bases <- calculate merge_bases_many(this_head, merge_bases)
No?
And these merge_bases are commit_lists, therefore, as mentioned above, the signature should not take an array, but the commit_lists.
Ciao,
Dscho
Re: [PATCH 09/10] Introduce get_octopus_merge_bases() in commit.c
Quoted 2 lines · Johannes Schindelin
Clever, but for performance reasons I think it might be better to use a commit list here.
Why? We do not iterate this that many times, and you do not gain much from extra allocations of commit_list structure you would need to do.
Quoted 11 lines
commit.c static struct commit *interesting(struct commit_list *list) · line 560
Quoted 8 lines
commit.c +4
-2one->object.flags |=PARENT1 ; - two->object.flags |=PARENT2 ;insert_by_date (one, &list); -insert_by_date (two, &list); +for (i =0 ; i < n; i++) { + twos[i]->object.flags |=PARENT2 ; +insert_by_date (twos[i], &list); + }Ah, now that is clever: I thought we would get into a lot of problems because we would need a bit for every initial commit.
That's not coming from being _clever_, but from solving a different problem than what you thought we are solving. This is _not_ computing the "base of all of them" as "git-show-branch --merge-base" computes.
Instead, this is "compute '2' given C and A+B", without actually having to have a commit T(A,B), for a merge to create T(A,B,C).
Quoted 5 lines
o---o---A
/ .
----1---2---B...T(A,B)
\ .
o---C...T(A,B,C)Instead of fabricating a commit T(A,B) and drop it as "two", you drop "A" and "B" both marked as PARENT2 into the list and have the logic traverse as before.
The purpose of this exercise is not doing the merge base computation for an octopus in one round. In fact, you do _not_ want to compute it upfront. Re-read the original message the above picture comes from to remind you of why (and the loop with a big comment at the bottom of git-merge-octopus). This is for a better computation of $MRC in each round there.
.
Re: [PATCH 09/10] Introduce get_octopus_merge_bases() in commit.c
In reply to Johannes SchindelinHi,
Quoted 5 lines · Johannes Schindelin
Quoted 5 lines
commit.c +1
-2one->object.flags |=PARENT1 ; - two->object.flags |=PARENT2 ;insert_by_date (one, &list); -insert_by_date (two, &list); +for (i =0 ; i < n; i++) {
Quoted 8 lines
commit.c static struct commit *interesting(struct commit_list *list) · line 560
Quoted 3 lines
commit.c +3
-0+ twos[i]->object.flags |=PARENT2 ; +insert_by_date (twos[i], &list); + }Ah, now that is clever: I thought we would get into a lot of problems because we would need a bit for every initial commit. But what you are basically doing is reusing PARENT2 for all the merge bases that have been found for the heads up to, but not including, the current one.
I spoke too soon. This is not going to work easily: after doing this for two heads, the PARENT1 flag must be reset to PARENT2 before traversing for a third head.
Or am I missing something obvious?
Ciao,
Dscho
Re: [PATCH 09/10] Introduce get_octopus_merge_bases() in commit.c
In reply to Junio C HamanoHi,
Quoted 11 lines · Junio C Hamano
Quoted 2 lines · Miklos Vajna
This is like
get_merge_bases()but it works for multiple heads, like show-branch --merge-base.In what sense is this "like show-branch --merge-base"?
The only similarlity I can spot is that it can take more than two heads, but what it computes and the way it computes it seem to be different. It certainly looks much less efficient as it does not walk the ancestry chain in one-go like show-branch does.
Ah, you are right. I thought that in the typical case (where the initial commits are independent), it will never traverse any commit twice. I did not really understand your example quickly enough, so I made up my own:
A - B - C - D - E
\ X
F - G - H I
\ X \
J \ K
\
- L
Now, let's get the merge bases with Miklos' algorithm for E, I, K and L.
First it will find the merge base for E and I, which is D. Then it calculates the merge base between that merge base and the third head, K, which leads us all the way back to A.
Now, calculating the merge base between that merge base and L will traverse the commits F, G and H _again_.
Ciao,
Dscho
Re: [PATCH 07/10] git-fmt-merge-msg: make it useable from other builtins
In reply to Miklos VajnaQuoted 8 lines · Miklos Vajna
Move all functionality (except config and option parsing) from
cmd_fmt_merge_msg()tofmt_merge_msg(), so that other builtins can use it without a child process.All functions have been changed to use strbufs, and now only
cmd_fmt_merge_msg()reads directly from a file / writes anything to stdout.
I have a very nearly identical patch in an ancient offshoot branch since my attempt to make git-pull a builtin (which was stupid as it doesn't do anything CPU-intensive apart from merging).
I likes it. For bonus points you could replace some strbuf_addf() calls with strbuf_addstr() ones, but I don't think anyone really cares.
So, fwiw,
Signature · 3 lines
Andreas Ericsson [email protected] OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231
PATCH 1/1 git-fmt-merge-msg: make it usable from other builtins
Move all functionality (except config and option parsing) from cmd_fmt_merge_msg() to fmt_merge_msg(), so that other builtins can use it without a child process.
All functions have been changed to use strbufs, and now only cmd_fmt_merge_msg() reads directly from a file / writes anything to stdout.
---
Quoted 3 lines · Andreas Ericsson
I likes it. For bonus points you could replace some
strbuf_addf()calls withstrbuf_addstr()ones, but I don't think anyone really cares.
Hopefully this one has no unnecessary strbuf_addf() calls.
Quoted 1 line
Liked-by Andreas Ericsson <[email protected]>
:-)
builtin-fmt-merge-msg.c | 157 +++++++++++++++++++++++++++-------------------- builtin.h | 3 + 2 files changed, 94 insertions(+), 66 deletions(-)
builtin-fmt-merge-msg.c +91 -66
@@ -159,23 +159,24 @@ static int handle_line(char *line) }static void print_joined (const char *singular,const char *plural, -struct list *list) +struct list *list,struct strbuf *out) {if (list->nr ==0 )return ;if (list->nr ==1 ) { -printf ("%s%s" , singular, list->list[0 ]); +strbuf_addf (out,"%s%s" , singular, list->list[0 ]); }else {int i; -printf ("%s" , plural); +strbuf_addstr (out, plural);for (i =0 ; i < list->nr -1 ; i++) -printf ("%s%s" , i >0 ?", " :"" , list->list[i]); -printf (" and %s" , list->list[list->nr -1 ]); +strbuf_addf (out,"%s%s" , i >0 ?", " :"" , list->list[i]); +strbuf_addf (out," and %s" , list->list[list->nr -1 ]); } }static void shortlog (const char *name,unsigned char *sha1, -struct commit *head,struct rev_info *rev,int limit) +struct commit *head,struct rev_info *rev,int limit, +struct strbuf *out) {int i, count =0 ;struct commit *commit;
@@ -232,15 +233,15 @@ static void shortlog(const char *name, unsigned char *sha1, }if (count > limit) -printf ("\n* %s: (%d commits)\n" , name, count); +strbuf_addf (out,"\n* %s: (%d commits)\n" , name, count);else -printf ("\n* %s:\n" , name); +strbuf_addf (out,"\n* %s:\n" , name);for (i =0 ; i < subjects.nr; i++)if (i >= limit) -printf (" ...\n" ); +strbuf_addf (out," ...\n" );else -printf (" %s\n" , subjects.list[i]); +strbuf_addf (out," %s\n" , subjects.list[i]);clear_commit_marks ((struct commit *)branch, flags);clear_commit_marks (head, flags);
@@ -251,43 +252,13 @@ static void shortlog(const char *name, unsigned char *sha1,free_list (&subjects); } -int cmd_fmt_merge_msg (int argc,const char **argv,const char *prefix) -{ -int limit =20 , i =0 ; +int fmt_merge_msg (int merge_summary,struct strbuf *in,struct strbuf *out) { +int limit =20 , i =0 , pos =0 ;char line[1024 ]; -FILE *in = stdin; -const char *sep ="" ; +char *p = line, *sep ="" ;unsigned char head_sha1[20 ];const char *current_branch; -git_config (fmt_merge_msg_config,NULL ); - -while (argc >1 ) { -if (!strcmp (argv[1 ],"--log" ) || !strcmp (argv[1 ],"--summary" )) - merge_summary =1 ; -else if (!strcmp (argv[1 ],"--no-log" ) - || !strcmp (argv[1 ],"--no-summary" )) - merge_summary =0 ; -else if (!strcmp (argv[1 ],"-F" ) || !strcmp (argv[1 ],"--file" )) { -if (argc <3 ) -die ("Which file?" ); -if (!strcmp (argv[2 ],"-" )) - in = stdin; -else { -fclose (in); - in =fopen (argv[2 ],"r" ); -if (!in) -die ("cannot open %s" , argv[2 ]); - } - argc--; argv++; - }else -break ; - argc--; argv++; - } - -if (argc >1 ) -usage (fmt_merge_msg_usage); -/* get current branch */ current_branch =resolve_ref ("HEAD" , head_sha1,1 ,NULL );if (!current_branch)
@@ -295,75 +266,129 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)if (!prefixcmp (current_branch,"refs/heads/" )) current_branch +=11 ; -while (fgets (line,sizeof (line), in)) { +/* get a line */ +for (;;) { +int len; +char *newline; + +if (pos >= in->len) +break ; + p = in->buf + pos; + newline =strchr (p,'\n' ); + len = newline ? newline - p :strlen (p); + pos += len + !!newline; i++; -if (line[0 ] ==0 ) -continue ; -if (handle_line (line)) -die ("Error in line %d: %s" , i, line); + p[len] =0 ; +if (handle_line (p)) +die ("Error in line %d: %.*s" , i, len, p); } -printf ("Merge " ); +strbuf_addstr (out,"Merge " );for (i =0 ; i < srcs.nr; i++) {struct src_data *src_data = srcs.payload[i];const char *subsep ="" ; -printf (sep); +strbuf_addstr (out, sep); sep ="; " ;if (src_data->head_status ==1 ) { -printf (srcs.list[i]); +strbuf_addstr (out, srcs.list[i]);continue ; }if (src_data->head_status ==3 ) { subsep =", " ; -printf ("HEAD" ); +strbuf_addstr (out,"HEAD" ); }if (src_data->branch.nr) { -printf (subsep); +strbuf_addstr (out, subsep); subsep =", " ; -print_joined ("branch " ,"branches " , &src_data->branch); +print_joined ("branch " ,"branches " , &src_data->branch, + out); }if (src_data->r_branch.nr) { -printf (subsep); +strbuf_addstr (out, subsep); subsep =", " ;print_joined ("remote branch " ,"remote branches " , - &src_data->r_branch); + &src_data->r_branch, out); }if (src_data->tag.nr) { -printf (subsep); +strbuf_addstr (out, subsep); subsep =", " ; -print_joined ("tag " ,"tags " , &src_data->tag); +print_joined ("tag " ,"tags " , &src_data->tag, out); }if (src_data->generic.nr) { -printf (subsep); -print_joined ("commit " ,"commits " , &src_data->generic); +strbuf_addstr (out, subsep); +print_joined ("commit " ,"commits " , &src_data->generic, + out); }if (strcmp ("." , srcs.list[i])) -printf (" of %s" , srcs.list[i]); +strbuf_addf (out," of %s" , srcs.list[i]); }if (!strcmp ("master" , current_branch)) -putchar ('\n' ); +strbuf_addch (out,'\n' );else -printf (" into %s\n" , current_branch); +strbuf_addf (out," into %s\n" , current_branch);if (merge_summary) {struct commit *head;struct rev_info rev; head =lookup_commit (head_sha1); -init_revisions (&rev, prefix); +init_revisions (&rev,NULL ); rev.commit_format =CMIT_FMT_ONELINE ; rev.ignore_merges =1 ; rev.limited =1 ;for (i =0 ; i < origins.nr; i++)shortlog (origins.list[i], origins.payload[i], - head, &rev, limit); + head, &rev, limit, out); } +return 0 ; +} + +int cmd_fmt_merge_msg (int argc,const char **argv,const char *prefix) +{ +FILE *in = stdin; +struct strbuf input, output; +int ret; + +git_config (fmt_merge_msg_config,NULL ); + +while (argc >1 ) { +if (!strcmp (argv[1 ],"--log" ) || !strcmp (argv[1 ],"--summary" )) + merge_summary =1 ; +else if (!strcmp (argv[1 ],"--no-log" ) + || !strcmp (argv[1 ],"--no-summary" )) + merge_summary =0 ; +else if (!strcmp (argv[1 ],"-F" ) || !strcmp (argv[1 ],"--file" )) { +if (argc <3 ) +die ("Which file?" ); +if (!strcmp (argv[2 ],"-" )) + in = stdin; +else { +fclose (in); + in =fopen (argv[2 ],"r" ); +if (!in) +die ("cannot open %s" , argv[2 ]); + } + argc--; argv++; + }else +break ; + argc--; argv++; + } + +if (argc >1 ) +usage (fmt_merge_msg_usage); -/* No cleanup yet; is standalone anyway */ +strbuf_init (&input,0 ); +if (strbuf_read (&input,fileno (in),0 ) <0 ) +die ("could not read input file %s" ,strerror (errno)); +strbuf_init (&output,0 ); + ret =fmt_merge_msg (merge_summary, &input, &output); +if (ret) +return ret; +printf ("%s" , output.buf);return 0 ; }
builtin.h +3 -0
@@ -2,6 +2,7 @@#define BUILTIN_H #include "git-compat-util.h" +#include "strbuf.h" extern const char git_version_string[];extern const char git_usage_string[];
@@ -11,6 +12,8 @@ extern void list_common_cmds_help(void);extern void help_unknown_cmd (const char *cmd);extern void prune_packed_objects (int );extern int read_line_with_nul (char *buf,int size,FILE *file); +extern int fmt_merge_msg (int merge_summary,struct strbuf *in, +struct strbuf *out);extern int cmd_add (int argc,const char **argv,const char *prefix);extern int cmd_annotate (int argc,const char **argv,const char *prefix);
Signature · 1 line
1.5.6.rc2.dirty
Re: [PATCH 06/10] Move read_cache_unmerged() to read-cache.c
In reply to Miklos VajnaQuoted 3 lines · Miklos Vajna
builtin-read-tree has a
read_cache_unmerged()which is useful for other builtins, for example builtin-merge uses it as well. Move it toread-cache.cto avoid code duplication.
Looks good, but as a public interface probably it needs a few lines of comment in front of the function's definition to describe what it is used for. Perhaps like...
/*
* Read the index file that is potentially unmerged into given
* index_state, dropping any unmerged entries. Returns true is
* the index is unmerged. Callers who want to refuse to work
* from an unmerged state can call this and check its return value,
* instead of calling read_cache().
*/
PATCH 1/1 Move read_cache_unmerged() to read-cache.c
builtin-read-tree has a read_cache_unmerged() which is useful for other builtins, for example builtin-merge uses it as well. Move it to read-cache.c to avoid code duplication.
---
Quoted 5 lines · Junio C Hamano
Looks good, but as a public interface probably it needs a few lines of comment in front of the function's definition to describe what it is used
for. Perhaps like...
Quoted 7 lines
/*
* Read the index file that is potentially unmerged into given
*index_state, dropping any unmerged entries. Returns true is
* the index is unmerged. Callers who want to refuse to work
* from an unmerged state can call this and check its return value,
* instead of callingread_cache().
*/
Added. I'm sending the patch I just pushed to my working branch.
builtin-read-tree.c | 24 ------------------------ cache.h | 2 ++ read-cache.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 24 deletions(-)
builtin-read-tree.c +0 -24
@@ -29,30 +29,6 @@ static int list_tree(unsigned char *sha1)return 0 ; } -static int read_cache_unmerged (void ) -{ -int i; -struct cache_entry **dst; -struct cache_entry *last =NULL ; - -read_cache (); - dst = active_cache; -for (i =0 ; i < active_nr; i++) { -struct cache_entry *ce = active_cache[i]; -if (ce_stage (ce)) { -remove_name_hash (ce); -if (last && !strcmp (ce->name, last->name)) -continue ; -cache_tree_invalidate_path (active_cache_tree, ce->name); - last = ce; -continue ; - } - *dst++ = ce; - } - active_nr = dst - active_cache; -return !!last; -} -static void prime_cache_tree_rec (struct cache_tree *it,struct tree *tree) {struct tree_desc desc;
cache.h +2 -0
@@ -254,6 +254,7 @@ static inline void remove_name_hash(struct cache_entry *ce)#define read_cache () read_index(&the_index)#define read_cache_from (path) read_index_from(&the_index, (path)) +#define read_cache_unmerged () read_index_unmerged(&the_index)#define write_cache (newfd, cache, entries) write_index(&the_index, (newfd))#define discard_cache () discard_index(&the_index)#define unmerged_cache () unmerged_index(&the_index)
@@ -357,6 +358,7 @@ extern int init_db(const char *template_dir, unsigned int flags);/* Initialize and use the cache information */ extern int read_index (struct index_state *);extern int read_index_from (struct index_state *,const char *path); +extern int read_index_unmerged (struct index_state *);extern int write_index (const struct index_state *,int newfd);extern int discard_index (struct index_state *);extern int unmerged_index (const struct index_state *);
read-cache.c +31 -0
@@ -1394,3 +1394,34 @@ int write_index(const struct index_state *istate, int newfd) }return ce_flush (&c, newfd); } + +/* +* Read the index file that is potentially unmerged into given +* index_state, dropping any unmerged entries. Returns true is +* the index is unmerged. Callers who want to refuse to work +* from an unmerged state can call this and check its return value, +* instead of calling read_cache(). +*/ +int read_index_unmerged (struct index_state *istate) +{ +int i; +struct cache_entry **dst; +struct cache_entry *last =NULL ; + +read_index (istate); + dst = istate->cache; +for (i =0 ; i < istate->cache_nr; i++) { +struct cache_entry *ce = istate->cache[i]; +if (ce_stage (ce)) { +remove_name_hash (ce); +if (last && !strcmp (ce->name, last->name)) +continue ; +cache_tree_invalidate_path (istate->cache_tree, ce->name); + last = ce; +continue ; + } + *dst++ = ce; + } + istate->cache_nr = dst - istate->cache; +return !!last; +}
Signature · 1 line
1.5.6.rc0.dirty
Re: [PATCH 04/10] Add new test to ensure git-merge handles pull.twohead and pull.octopus
In reply to Miklos VajnaQuoted 36 lines · Miklos Vajna
Test if the given strategies are used and test the case when multiple strategies are configured using a space separated list.
Signed-off-by Miklos Vajna <[email protected]>--- t/t7601-merge-pull-config.sh | 57 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 57 insertions(+), 0 deletions(-) create mode 100755 t/t7601-merge-pull-config.sht/t7601-merge-pull-config.sh new file +21
-0@@ -0,0 +1,57 @@ +#!/bin/sh + +test_description='git-merge + +Testing pull.* configuration parsing.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo c0 >c0.c && + git add c0.c && + git commit -m c0 && + git tag c0 && + echo c1 >c1.c && + git add c1.c && + git commit -m c1 && + git tag c1 && + git reset --hard c0 && + echo c2 >c2.c && + git add c2.c && + git commit -m c2 &&
t/t7601-merge-pull-config.sh line 31
Quoted 10 lines
t/t7601-merge-pull-config.sh new file +10
-0+ git tag c2 + git reset --hard c0 && + echo c3 >c3.c && + git add c3.c && + git commit -m c3 && + git tag c3 +' + +test_expect_success 'merge c1 with c2' ' + git reset --hard c1 &&
test that c0 and c1 do and c2 and c3 do not exist here, as it is cheap, and otherwise you may end up chasing wild-goose when somebody breaks git-reset. No need to do so in later tests in this script, but it is a cheap protection for yourself from others' mistakes ;-).
t/t7601-merge-pull-config.sh line 35
Quoted 4 lines
t/t7601-merge-pull-config.sh new file +4
-0+ git merge c2 && + test -e c1.c && + test -e c2.c +'
Nobody runs V7 that lacked "test -e" to run these test scripts, but you expect them to be regular files at this point of the test, so the correct way to spell these is with "test -f".
In general, you are better off training yourself to think if you can use "test -f" before blindly using "test -e".
t/t7601-merge-pull-config.sh line 48
Quoted 12 lines
t/t7601-merge-pull-config.sh new file +12
-0+test_expect_success 'merge c1 with c2 (ours in pull.twohead)' ' + git reset --hard c1 && + git config pull.twohead ours && + git merge c2 && + test -e c1.c && + ! test -e c2.c +' + +test_expect_success 'merge c1 with c2 and c3 (recursive in pull.octopus)' ' + git reset --hard c1 && + git config pull.octopus "recursive" && + ! git merge c2 c3
Is it because it should dump core, or is it because the command should decline to work, gracefully failing with an error message and non-zero exit status? Use "test_must_fail" to check for the latter.
Don't you want to check how it fails and in what shape the command leaves the work tree? I am assuming that recursive sees more than one "remote" head and declines to work without touching work tree nor the index, so if that is what you expect, you should check for that. Otherwise, a regression that loses local changes will go unnoticed.
t/t7601-merge-pull-config.sh line 54
Quoted 6 lines
t/t7601-merge-pull-config.sh new file +6
-0+' + +test_expect_success 'merge c1 with c2 and c3 (recursive and octopus in pull.octopus)' ' + git reset --hard c1 && + git config pull.octopus "recursive octopus" && + git merge c2 c3
Likewise, don't you want to check the result of the merge? Not just "merge exited with 0", but you would want to see that the HEAD has advanced, it has the expected parents, there is no unexpected local changes (because you did not have any when you started the merge), and it has the expected tree contents.
Quoted 5 lines
t/t7601-merge-pull-config.sh new file +3
-0+' + +test_doneSignature · 1 line
1.5.6.rc0.dirty
PATCH 1/1 Add new test to ensure git-merge handles pull.twohead and pull.octopus
Test if the given strategies are used and test the case when multiple strategies are configured using a space separated list.
---
Quoted 9 lines · Junio C Hamano
Review
Quoted 2 lines
(quoted diff) +2
-0+test_expect_success 'merge c1 with c2' ' + git reset --hard c1 &&test that c0 and c1 do and c2 and c3 do not exist here, as it is cheap,
and otherwise you may end up chasing wild-goose when somebody breaks git-reset. No need to do so in later tests in this script, but it is a
cheap protection for yourself from others' mistakes ;-).
Done.
Quoted 11 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ git merge c2 && + test -e c1.c && + test -e c2.c +'Nobody runs V7 that lacked "test -e" to run these test scripts, but you
expect them to be regular files at this point of the test, so the correct
way to spell these is with "test -f".
Quoted 3 lines
In general, you are better off training yourself to think if you can use
"test -f" before blindly using "test -e".
Sure, corrected.
Quoted 9 lines
Review
Quoted 5 lines
+test_expect_success 'merge c1 with c2 and c3 (recursive in pull.octopus)' '
(quoted diff) +3
-0+ git reset --hard c1 && + git config pull.octopus "recursive" && + ! git merge c2 c3Is it because it should dump core, or is it because the command should decline to work, gracefully failing with an error message and non-zero exit status? Use "
test_must_fail" to check for the latter.
Obviously the later, corrected.
Quoted 8 lines
Don't you want to check how it fails and in what shape the command leaves
the work tree? I am assuming that recursive sees more than one "remote"
head and declines to work without touching work tree nor the index, so if
that is what you expect, you should check for that. Otherwise, a regression that loses local changes will go unnoticed.
Hm yes. I added checks to ensure nothing happened.
Quoted 6 lines
Review
Quoted 5 lines
+test_expect_success 'merge c1 with c2 and c3 (recursive and octopus in pull.octopus)' '
(quoted diff) +3
-0+ git reset --hard c1 && + git config pull.octopus "recursive octopus" && + git merge c2 c3
Quoted 6 lines
Likewise, don't you want to check the result of the merge? Not just "merge exited with 0", but you would want to see that the HEAD has advanced, it has the expected parents, there is no unexpected local changes (because you did not have any when you started the merge), and it
has the expected tree contents.
Corrected.
I'm sending the version I just pushed to my working branch.
t/t7601-merge-pull-config.sh | 72 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 72 insertions(+), 0 deletions(-) create mode 100755 t/t7601-merge-pull-config.sh
t/t7601-merge-pull-config.sh new file +72 -0
@@ -0,0 +1,72 @@ +#!/bin/sh + +test_description='git-merge + +Testing pull.* configuration parsing.' + +. ./test-lib.sh + +test_expect_success 'setup' ' + echo c0 >c0.c && + git add c0.c && + git commit -m c0 && + git tag c0 && + echo c1 >c1.c && + git add c1.c && + git commit -m c1 && + git tag c1 && + git reset --hard c0 && + echo c2 >c2.c && + git add c2.c && + git commit -m c2 && + git tag c2 + git reset --hard c0 && + echo c3 >c3.c && + git add c3.c && + git commit -m c3 && + git tag c3 +' + +test_expect_success 'merge c1 with c2' ' + git reset --hard c1 && + test -f c0.c && + test -f c1.c && + test ! -f c2.c && + test ! -f c3.c && + git merge c2 && + test -f c1.c && + test -f c2.c +' + +test_expect_success 'merge c1 with c2 (ours in pull.twohead)' ' + git reset --hard c1 && + git config pull.twohead ours && + git merge c2 && + test -f c1.c && + ! test -f c2.c +' + +test_expect_success 'merge c1 with c2 and c3 (recursive in pull.octopus)' ' + git reset --hard c1 && + git config pull.octopus "recursive" && + test_must_fail git merge c2 c3 && + test "$(git rev-parse c1)" = "$(git rev-parse HEAD)" +' + +test_expect_success 'merge c1 with c2 and c3 (recursive and octopus in pull.octopus)' ' + git reset --hard c1 && + git config pull.octopus "recursive octopus" && + git merge c2 c3 && + test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" && + test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" && + test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" && + test "$(git rev-parse c3)" = "$(git rev-parse HEAD^3)" + test "$(git rev-parse c3)" = "$(git rev-parse HEAD^3)" && + git diff --exit-code && + test -f c0.c && + test -f c1.c && + test -f c2.c && + test -f c3.c +' + +test_done
Signature · 1 line
1.5.6.rc0.dirty
Re: [PATCH 03/10] Move builtin-remote's skip_prefix() to git-compat-util.h
In reply to Miklos VajnaQuoted 22 lines · Miklos Vajna
builtin-remote.c +0
-6@@ -29,12 +29,6 @@ static inline int postfixcmp(const char *string, const char *postfix)return strcmp (string + len1 - len2, postfix); } -static inline const char *skip_prefix (const char *name,const char *prefix) -{ -return !name ?"" : -prefixcmp (name, prefix) ? name : name +strlen (prefix); -} -static int opt_parse_track (const struct option *opt,const char *arg,int not) {struct path_list *list = opt->value;git-compat-util.h +0
-0
git-compat-util.h extern void set_warn_routine(void (*routine)(const char *warn, va_list params)); · line 135
Quoted 9 lines
git-compat-util.h +6
-0@@ -127,6 +127,12 @@ extern void set_warn_routine(void (*routine)(const char *warn, va_list params));extern int prefixcmp (const char *str,const char *prefix); +static inline const char *skip_prefix (const char *name,const char *prefix) +{ +return !name ?"" : +prefixcmp (name, prefix) ? name : name +strlen (prefix); +} +
Somehow you seemed to have picked the one whose semantics is defined much less clearly. For one thing, it takes more effort (and unnatural way to check) for the caller to detect the case where prefix did not match than the one that returns NULL. Worse, I think this one is less efficient, doing strlen(prefix) twice.
Quoted 18 lines
parse-options.c +0
-6@@ -22,12 +22,6 @@ static inline const char *get_arg(struct optparse_t *p)return *++p->argv; } -static inline const char *skip_prefix (const char *str,const char *prefix) -{ -size_t len =strlen (prefix); -return strncmp (str, prefix, len) ?NULL : str + len; -} -static int opterror (const struct option *opt,const char *reason,int flags) {if (flags &OPT_SHORT )
parse-options.c static int parse_long_opt(struct optparse_t *p, const char *arg, · line 158
Quoted 5 lines
parse-options.c +1
-1@@ -161,7 +155,7 @@ static int parse_long_opt(struct optparse_t *p, const char *arg, rest =skip_prefix (arg, options->long_name);if (options->type ==OPTION_ARGUMENT ) { -if (!rest) +if (!strcmp (rest, arg))
Ugh.
At least please have the courtesy of not making it more expensive than the original unnecessarily. Isn't (rest == arg) enough here?
Still, I think the one from builtin-remote.c you used is a much worse implementation of the same thing between the two. It was Ok while it was a local scope hack only for builtin-remote.c's use, but a special calling convention like "if name is NULL return empty string" should not be promoted to public utility library status without defending why it is always a good idea to do so.
PATCH 1/1 Move parse-options's skip_prefix() to git-compat-util.h
builtin-remote.c and parse-options.c both have a skip_prefix() function, for the same purpose. Move parse-options's one to git-compat-util.h and let builtin-remote use it as well.
---
Quoted 8 lines · Junio C Hamano
Somehow you seemed to have picked the one whose semantics is defined much
less clearly. For one thing, it takes more effort (and unnatural way to
check) for the caller to detect the case where prefix did not match than
the one that returns NULL. Worse, I think this one is less efficient, doing strlen(prefix) twice.
Here is what I pushed to my working branch.
builtin-remote.c | 39 ++++++++++++++++++++++++++------------- git-compat-util.h | 6 ++++++ parse-options.c | 6 ------ 3 files changed, 32 insertions(+), 19 deletions(-)
builtin-remote.c +26 -13
@@ -29,12 +29,6 @@ static inline int postfixcmp(const char *string, const char *postfix)return strcmp (string + len1 - len2, postfix); } -static inline const char *skip_prefix (const char *name,const char *prefix) -{ -return !name ?"" : -prefixcmp (name, prefix) ? name : name +strlen (prefix); -} -static int opt_parse_track (const struct option *opt,const char *arg,int not) {struct path_list *list = opt->value;
@@ -182,12 +176,18 @@ static int config_read_branches(const char *key, const char *value, void *cb) info->remote =xstrdup (value); }else {char *space =strchr (value,' ' ); - value =skip_prefix (value,"refs/heads/" ); +const char *ptr =skip_prefix (value,"refs/heads/" ); +if (ptr) + value = ptr;while (space) {char *merge; merge =xstrndup (value, space - value);path_list_append (merge, &info->merge); - value =skip_prefix (space +1 ,"refs/heads/" ); + ptr =skip_prefix (space +1 ,"refs/heads/" ); +if (ptr) + value = ptr; +else + value = space +1 ; space =strchr (value,' ' ); }path_list_append (xstrdup (value), &info->merge);
@@ -219,7 +219,12 @@ static int handle_one_branch(const char *refname, refspec.dst = (char *)refname; if (!remote_find_tracking(states->remote, &refspec)) { struct path_list_item *item; - const char *name = skip_prefix(refspec.src, "refs/heads/"); + const char *name, *ptr; + ptr = skip_prefix(refspec.src, "refs/heads/"); + if (ptr) + name = ptr; + else + name = refspec.src; /* symbolic refs pointing nowhere were handled already */ if ((flags & REF_ISSYMREF) || unsorted_path_list_has_path(&states->tracked,
@@ -248,6 +253,7 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)struct path_list *target = &states->tracked;unsigned char sha1[20 ];void *util =NULL ; +const char *ptr;if (!ref->peer_ref ||read_ref (ref->peer_ref->name, sha1)) target = &states->new;
@@ -256,8 +262,10 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)if (hashcmp (sha1, ref->new_sha1)) util = &states; } -path_list_append (skip_prefix (ref->name,"refs/heads/" ), - target)->util = util; + ptr =skip_prefix (ref->name,"refs/heads/" ); +if (!ptr) + ptr = ref->name; +path_list_append (ptr, target)->util = util; }free_refs (fetch_map);
@@ -504,10 +512,15 @@ static int show_or_prune(int argc, const char **argv, int prune) "es" : "");for (i =0 ; i < states.remote->push_refspec_nr; i++) {struct refspec *spec = states.remote->push + i; +const char *p ="" , *q ="" ; +if (spec->src) + p =skip_prefix (spec->src,"refs/heads/" ); +if (spec->dst) + q =skip_prefix (spec->dst,"refs/heads/" );printf (" %s%s%s%s" , spec->force ?"+" :"" , -skip_prefix (spec->src,"refs/heads/" ), + p ? p : spec->src, spec->dst ?":" :"" , -skip_prefix (spec->dst,"refs/heads/" )); + q ? q : spec->dst); }printf ("\n" ); }
git-compat-util.h +6 -0
@@ -127,6 +127,12 @@ extern void set_warn_routine(void (*routine)(const char *warn, va_list params));extern int prefixcmp (const char *str,const char *prefix); +static inline const char *skip_prefix (const char *str,const char *prefix) +{ +size_t len =strlen (prefix); +return strncmp (str, prefix, len) ?NULL : str + len; +} +#ifdef NO_MMAP #ifndef PROT_READ
parse-options.c +0 -6
@@ -22,12 +22,6 @@ static inline const char *get_arg(struct optparse_t *p)return *++p->argv; } -static inline const char *skip_prefix (const char *str,const char *prefix) -{ -size_t len =strlen (prefix); -return strncmp (str, prefix, len) ?NULL : str + len; -} -static int opterror (const struct option *opt,const char *reason,int flags) {if (flags &OPT_SHORT )
Signature · 1 line
1.5.6.rc0.dirty