threads / patch / 15340

patch, 2 partsmerge-recursive: move call_depth to struct merge_options

Subject: [PATCH 1/2] merge-recursive: move call_depth to struct merge_options

## tl;dr

15 messages between Sep 2, 2008 and Sep 5, 2008. Diffs are folded; open one to read it.

replies: 14people: 2as markdown or json

Miklos Vajna· Sep 2, 2008, 22:05 UTC · lore

[PATCH 0/2] Move call_depth and index_only to struct merge_options

On Tue, Sep 02, 2008 at 01:02:31PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> I found it a bit disturbing that "index_only" and "call_depth" were
> not
> part of merge_options structure.
Here are two patches to do it, on top of current mv/merge-recursive.

Both patches are pretty trivial, I think, but I split them as the second one is larger.

Now in some cases we have function calls like
merge_file(o, one, a, b, o->branch1, o->branch2);

but I don't think we can avoid it, as the last two parameters are not always like this (two times are like this, and once conditionally swapped). I'm not sure if it worth to introduce a new "int swap" parameter, just to avoid the last two one.

Miklos Vajna (2):
  merge-recursive: move call_depth to struct merge_options
  merge-recursive: move index_only to struct merge_options
 merge-recursive.c |  164 ++++++++++++++++++++++++++---------------------------
 merge-recursive.h |   13 ++++
 2 files changed, 93 insertions(+), 84 deletions(-)
Miklos Vajna· Sep 2, 2008, 22:05 UTC · re: Miklos Vajna · lore
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 merge-recursive.c |   25 ++++++++++++-------------
 merge-recursive.h |    1 +
 2 files changed, 13 insertions(+), 13 deletions(-)
Show changes to 2 files +13 −13

merge-recursive.c, merge-recursive.h

diff --git a/merge-recursive.c b/merge-recursive.c
index 457ad84..5bb20aa 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -80,12 +80,11 @@ struct stage_data
 static struct string_list current_file_set = {NULL, 0, 0, 1};
 static struct string_list current_directory_set = {NULL, 0, 0, 1};
 
-static int call_depth = 0;
 static struct strbuf obuf = STRBUF_INIT;
 
 static int show(struct merge_options *o, int v)
 {
-	return (!call_depth && o->verbosity >= v) || o->verbosity >= 5;
+	return (!o->call_depth && o->verbosity >= v) || o->verbosity >= 5;
 }
 
 static void flush_output(void)
@@ -104,9 +103,9 @@ static void output(struct merge_options *o, int v, const char *fmt, ...)
 	if (!show(o, v))
 		return;
 
-	strbuf_grow(&obuf, call_depth * 2 + 2);
-	memset(obuf.buf + obuf.len, ' ', call_depth * 2);
-	strbuf_setlen(&obuf, obuf.len + call_depth * 2);
+	strbuf_grow(&obuf, o->call_depth * 2 + 2);
+	memset(obuf.buf + obuf.len, ' ', o->call_depth * 2);
+	strbuf_setlen(&obuf, obuf.len + o->call_depth * 2);
 
 	va_start(ap, fmt);
 	len = vsnprintf(obuf.buf + obuf.len, strbuf_avail(&obuf), fmt, ap);
@@ -129,11 +128,11 @@ static void output(struct merge_options *o, int v, const char *fmt, ...)
 		flush_output();
 }
 
-static void output_commit_title(struct commit *commit)
+static void output_commit_title(struct merge_options *o, struct commit *commit)
 {
 	int i;
 	flush_output();
-	for (i = call_depth; i--;)
+	for (i = o->call_depth; i--;)
 		fputs("  ", stdout);
 	if (commit->util)
 		printf("virtual %s\n", (char *)commit->util);
@@ -1230,8 +1229,8 @@ int merge_recursive(struct merge_options *o,
 
 	if (show(o, 4)) {
 		output(o, 4, "Merging:");
-		output_commit_title(h1);
-		output_commit_title(h2);
+		output_commit_title(o, h1);
+		output_commit_title(o, h2);
 	}
 
 	if (!ca) {
@@ -1242,7 +1241,7 @@ int merge_recursive(struct merge_options *o,
 	if (show(o, 5)) {
 		output(o, 5, "found %u common ancestor(s):", commit_list_count(ca));
 		for (iter = ca; iter; iter = iter->next)
-			output_commit_title(iter->item);
+			output_commit_title(o, iter->item);
 	}
 
 	merged_common_ancestors = pop_commit(&ca);
@@ -1258,7 +1257,7 @@ int merge_recursive(struct merge_options *o,
 
 	for (iter = ca; iter; iter = iter->next) {
 		const char *saved_b1, *saved_b2;
-		call_depth++;
+		o->call_depth++;
 		/*
 		 * When the merge fails, the result contains files
 		 * with conflict markers. The cleanness flag is
@@ -1275,14 +1274,14 @@ int merge_recursive(struct merge_options *o,
 				NULL, &merged_common_ancestors);
 		o->branch1 = saved_b1;
 		o->branch2 = saved_b2;
-		call_depth--;
+		o->call_depth--;
 
 		if (!merged_common_ancestors)
 			die("merge returned no commit");
 	}
 
 	discard_cache();
-	if (!call_depth) {
+	if (!o->call_depth) {
 		read_cache();
 		index_only = 0;
 	} else
diff --git a/merge-recursive.h b/merge-recursive.h
index 72f0a28..4f55374 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -9,6 +9,7 @@ struct merge_options {
 	int verbosity;
 	int diff_rename_limit;
 	int merge_rename_limit;
+	int call_depth;
 };
 
 /* merge_trees() but with recursive ancestor consolidation */
-- 
1.6.0.1
Miklos Vajna· Sep 2, 2008, 22:05 UTC · re: Miklos Vajna · lore

[PATCH 2/2] merge-recursive: move index_only to struct merge_options

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 merge-recursive.c |  139 ++++++++++++++++++++++++++---------------------------
 merge-recursive.h |   12 +++++
 2 files changed, 80 insertions(+), 71 deletions(-)
Show changes to 2 files +80 −71

merge-recursive.c, merge-recursive.h

diff --git a/merge-recursive.c b/merge-recursive.c
index 5bb20aa..4af657c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -165,17 +165,6 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 	return add_cache_entry(ce, options);
 }
 
-/*
- * This is a global variable which is used in a number of places but
- * only written to in the 'merge' function.
- *
- * index_only == 1    => Don't leave any non-stage 0 entries in the cache and
- *                       don't update the working directory.
- *               0    => Leave unmerged entries in the cache and update
- *                       the working directory.
- */
-static int index_only = 0;
-
 static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
 {
 	parse_tree(tree);
@@ -428,10 +417,11 @@ static int remove_path(const char *name)
 	return ret;
 }
 
-static int remove_file(int clean, const char *path, int no_wd)
+static int remove_file(struct merge_options *o, int clean,
+		       const char *path, int no_wd)
 {
-	int update_cache = index_only || clean;
-	int update_working_directory = !index_only && !no_wd;
+	int update_cache = o->index_only || clean;
+	int update_working_directory = !o->index_only && !no_wd;
 
 	if (update_cache) {
 		if (remove_file_from_cache(path))
@@ -509,13 +499,14 @@ static int make_room_for_path(const char *path)
 	return error(msg, path, ": perhaps a D/F conflict?");
 }
 
-static void update_file_flags(const unsigned char *sha,
+static void update_file_flags(struct merge_options *o,
+			      const unsigned char *sha,
 			      unsigned mode,
 			      const char *path,
 			      int update_cache,
 			      int update_wd)
 {
-	if (index_only)
+	if (o->index_only)
 		update_wd = 0;
 
 	if (update_wd) {
@@ -574,12 +565,13 @@ static void update_file_flags(const unsigned char *sha,
 		add_cacheinfo(mode, sha, path, 0, update_wd, ADD_CACHE_OK_TO_ADD);
 }
 
-static void update_file(int clean,
+static void update_file(struct merge_options *o,
+			int clean,
 			const unsigned char *sha,
 			unsigned mode,
 			const char *path)
 {
-	update_file_flags(sha, mode, path, index_only || clean, !index_only);
+	update_file_flags(o, sha, mode, path, o->index_only || clean, !o->index_only);
 }
 
 /* Low level file merging, update and removal */
@@ -609,8 +601,9 @@ static void fill_mm(const unsigned char *sha1, mmfile_t *mm)
 	mm->size = size;
 }
 
-static int merge_3way(mmbuffer_t *result_buf,
-		      struct diff_filespec *o,
+static int merge_3way(struct merge_options *o,
+		      mmbuffer_t *result_buf,
+		      struct diff_filespec *one,
 		      struct diff_filespec *a,
 		      struct diff_filespec *b,
 		      const char *branch1,
@@ -623,13 +616,13 @@ static int merge_3way(mmbuffer_t *result_buf,
 	name1 = xstrdup(mkpath("%s:%s", branch1, a->path));
 	name2 = xstrdup(mkpath("%s:%s", branch2, b->path));
 
-	fill_mm(o->sha1, &orig);
+	fill_mm(one->sha1, &orig);
 	fill_mm(a->sha1, &src1);
 	fill_mm(b->sha1, &src2);
 
 	merge_status = ll_merge(result_buf, a->path, &orig,
 				&src1, name1, &src2, name2,
-				index_only);
+				o->index_only);
 
 	free(name1);
 	free(name2);
@@ -639,9 +632,12 @@ static int merge_3way(mmbuffer_t *result_buf,
 	return merge_status;
 }
 
-static struct merge_file_info merge_file(struct diff_filespec *o,
-		struct diff_filespec *a, struct diff_filespec *b,
-		const char *branch1, const char *branch2)
+static struct merge_file_info merge_file(struct merge_options *o,
+				         struct diff_filespec *one,
+					 struct diff_filespec *a,
+					 struct diff_filespec *b,
+					 const char *branch1,
+					 const char *branch2)
 {
 	struct merge_file_info result;
 	result.merge = 0;
@@ -657,31 +653,31 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
 			hashcpy(result.sha, b->sha1);
 		}
 	} else {
-		if (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))
+		if (!sha_eq(a->sha1, one->sha1) && !sha_eq(b->sha1, one->sha1))
 			result.merge = 1;
 
 		/*
 		 * Merge modes
 		 */
-		if (a->mode == b->mode || a->mode == o->mode)
+		if (a->mode == b->mode || a->mode == one->mode)
 			result.mode = b->mode;
 		else {
 			result.mode = a->mode;
-			if (b->mode != o->mode) {
+			if (b->mode != one->mode) {
 				result.clean = 0;
 				result.merge = 1;
 			}
 		}
 
-		if (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, o->sha1))
+		if (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, one->sha1))
 			hashcpy(result.sha, b->sha1);
-		else if (sha_eq(b->sha1, o->sha1))
+		else if (sha_eq(b->sha1, one->sha1))
 			hashcpy(result.sha, a->sha1);
 		else if (S_ISREG(a->mode)) {
 			mmbuffer_t result_buf;
 			int merge_status;
 
-			merge_status = merge_3way(&result_buf, o, a, b,
+			merge_status = merge_3way(o, &result_buf, one, a, b,
 						  branch1, branch2);
 
 			if ((merge_status < 0) || !result_buf.ptr)
@@ -726,22 +722,22 @@ static void conflict_rename_rename(struct merge_options *o,
 		dst_name1 = del[delp++] = unique_path(ren1_dst, branch1);
 		output(o, 1, "%s is a directory in %s adding as %s instead",
 		       ren1_dst, branch2, dst_name1);
-		remove_file(0, ren1_dst, 0);
+		remove_file(o, 0, ren1_dst, 0);
 	}
 	if (string_list_has_string(&current_directory_set, ren2_dst)) {
 		dst_name2 = del[delp++] = unique_path(ren2_dst, branch2);
 		output(o, 1, "%s is a directory in %s adding as %s instead",
 		       ren2_dst, branch1, dst_name2);
-		remove_file(0, ren2_dst, 0);
+		remove_file(o, 0, ren2_dst, 0);
 	}
-	if (index_only) {
+	if (o->index_only) {
 		remove_file_from_cache(dst_name1);
 		remove_file_from_cache(dst_name2);
 		/*
 		 * Uncomment to leave the conflicting names in the resulting tree
 		 *
-		 * update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, dst_name1);
-		 * update_file(0, ren2->pair->two->sha1, ren2->pair->two->mode, dst_name2);
+		 * update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode, dst_name1);
+		 * update_file(o, 0, ren2->pair->two->sha1, ren2->pair->two->mode, dst_name2);
 		 */
 	} else {
 		update_stages(dst_name1, NULL, ren1->pair->two, NULL, 1);
@@ -757,8 +753,8 @@ static void conflict_rename_dir(struct merge_options *o,
 {
 	char *new_path = unique_path(ren1->pair->two->path, branch1);
 	output(o, 1, "Renaming %s to %s instead", ren1->pair->one->path, new_path);
-	remove_file(0, ren1->pair->two->path, 0);
-	update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path);
+	remove_file(o, 0, ren1->pair->two->path, 0);
+	update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path);
 	free(new_path);
 }
 
@@ -773,9 +769,9 @@ static void conflict_rename_rename_2(struct merge_options *o,
 	output(o, 1, "Renaming %s to %s and %s to %s instead",
 	       ren1->pair->one->path, new_path1,
 	       ren2->pair->one->path, new_path2);
-	remove_file(0, ren1->pair->two->path, 0);
-	update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path1);
-	update_file(0, ren2->pair->two->sha1, ren2->pair->two->mode, new_path2);
+	remove_file(o, 0, ren1->pair->two->path, 0);
+	update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path1);
+	update_file(o, 0, ren2->pair->two->sha1, ren2->pair->two->mode, new_path2);
 	free(new_path2);
 	free(new_path1);
 }
@@ -867,17 +863,18 @@ static int process_renames(struct merge_options *o,
 				       "rename \"%s\"->\"%s\" in \"%s\"%s",
 				       src, ren1_dst, branch1,
 				       src, ren2_dst, branch2,
-				       index_only ? " (left unresolved)": "");
-				if (index_only) {
+				       o->index_only ? " (left unresolved)": "");
+				if (o->index_only) {
 					remove_file_from_cache(src);
-					update_file(0, ren1->pair->one->sha1,
+					update_file(o, 0, ren1->pair->one->sha1,
 						    ren1->pair->one->mode, src);
 				}
 				conflict_rename_rename(o, ren1, branch1, ren2, branch2);
 			} else {
 				struct merge_file_info mfi;
-				remove_file(1, ren1_src, 1);
-				mfi = merge_file(ren1->pair->one,
+				remove_file(o, 1, ren1_src, 1);
+				mfi = merge_file(o,
+						 ren1->pair->one,
 						 ren1->pair->two,
 						 ren2->pair->two,
 						 branch1,
@@ -893,14 +890,14 @@ static int process_renames(struct merge_options *o,
 					       ren1_dst);
 					clean_merge = 0;
 
-					if (!index_only)
+					if (!o->index_only)
 						update_stages(ren1_dst,
 							      ren1->pair->one,
 							      ren1->pair->two,
 							      ren2->pair->two,
 							      1 /* clear */);
 				}
-				update_file(mfi.clean, mfi.sha, mfi.mode, ren1_dst);
+				update_file(o, mfi.clean, mfi.sha, mfi.mode, ren1_dst);
 			}
 		} else {
 			/* Renamed in 1, maybe changed in 2 */
@@ -909,7 +906,7 @@ static int process_renames(struct merge_options *o,
 			struct diff_filespec src_other, dst_other;
 			int try_merge, stage = a_renames == renames1 ? 3: 2;
 
-			remove_file(1, ren1_src, index_only || stage == 3);
+			remove_file(o, 1, ren1_src, o->index_only || stage == 3);
 
 			hashcpy(src_other.sha1, ren1->src_entry->stages[stage].sha);
 			src_other.mode = ren1->src_entry->stages[stage].mode;
@@ -931,7 +928,7 @@ static int process_renames(struct merge_options *o,
 				       "and deleted in %s",
 				       ren1_src, ren1_dst, branch1,
 				       branch2);
-				update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, ren1_dst);
+				update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode, ren1_dst);
 			} else if (!sha_eq(dst_other.sha1, null_sha1)) {
 				const char *new_path;
 				clean_merge = 0;
@@ -942,7 +939,7 @@ static int process_renames(struct merge_options *o,
 				       ren1_dst, branch2);
 				new_path = unique_path(ren1_dst, branch2);
 				output(o, 1, "Adding as %s instead", new_path);
-				update_file(0, dst_other.sha1, dst_other.mode, new_path);
+				update_file(o, 0, dst_other.sha1, dst_other.mode, new_path);
 			} else if ((item = string_list_lookup(ren1_dst, renames2Dst))) {
 				ren2 = item->util;
 				clean_merge = 0;
@@ -969,7 +966,7 @@ static int process_renames(struct merge_options *o,
 					b = ren1->pair->two;
 					a = &src_other;
 				}
-				mfi = merge_file(one, a, b,
+				mfi = merge_file(o, one, a, b,
 						o->branch1, o->branch2);
 
 				if (mfi.clean &&
@@ -991,11 +988,11 @@ static int process_renames(struct merge_options *o,
 						       ren1_dst);
 						clean_merge = 0;
 
-						if (!index_only)
+						if (!o->index_only)
 							update_stages(ren1_dst,
 								      one, a, b, 1);
 					}
-					update_file(mfi.clean, mfi.sha, mfi.mode, ren1_dst);
+					update_file(o, mfi.clean, mfi.sha, mfi.mode, ren1_dst);
 				}
 			}
 		}
@@ -1037,7 +1034,7 @@ static int process_entry(struct merge_options *o,
 			if (a_sha)
 				output(o, 2, "Removing %s", path);
 			/* do not touch working file if it did not exist */
-			remove_file(1, path, !a_sha);
+			remove_file(o, 1, path, !a_sha);
 		} else {
 			/* Deleted in one and changed in the other */
 			clean_merge = 0;
@@ -1046,13 +1043,13 @@ static int process_entry(struct merge_options *o,
 				       "and modified in %s. Version %s of %s left in tree.",
 				       path, o->branch1,
 				       o->branch2, o->branch2, path);
-				update_file(0, b_sha, b_mode, path);
+				update_file(o, 0, b_sha, b_mode, path);
 			} else {
 				output(o, 1, "CONFLICT (delete/modify): %s deleted in %s "
 				       "and modified in %s. Version %s of %s left in tree.",
 				       path, o->branch2,
 				       o->branch1, o->branch1, path);
-				update_file(0, a_sha, a_mode, path);
+				update_file(o, 0, a_sha, a_mode, path);
 			}
 		}
 
@@ -1084,11 +1081,11 @@ static int process_entry(struct merge_options *o,
 			output(o, 1, "CONFLICT (%s): There is a directory with name %s in %s. "
 			       "Adding %s as %s",
 			       conf, path, other_branch, path, new_path);
-			remove_file(0, path, 0);
-			update_file(0, sha, mode, new_path);
+			remove_file(o, 0, path, 0);
+			update_file(o, 0, sha, mode, new_path);
 		} else {
 			output(o, 2, "Adding %s", path);
-			update_file(1, sha, mode, path);
+			update_file(o, 1, sha, mode, path);
 		}
 	} else if (a_sha && b_sha) {
 		/* Case C: Added in both (check for same permissions) and */
@@ -1110,12 +1107,12 @@ static int process_entry(struct merge_options *o,
 		hashcpy(b.sha1, b_sha);
 		b.mode = b_mode;
 
-		mfi = merge_file(&one, &a, &b,
+		mfi = merge_file(o, &one, &a, &b,
 				 o->branch1, o->branch2);
 
 		clean_merge = mfi.clean;
 		if (mfi.clean)
-			update_file(1, mfi.sha, mfi.mode, path);
+			update_file(o, 1, mfi.sha, mfi.mode, path);
 		else if (S_ISGITLINK(mfi.mode))
 			output(o, 1, "CONFLICT (submodule): Merge conflict in %s "
 			       "- needs %s", path, sha1_to_hex(b.sha1));
@@ -1123,10 +1120,10 @@ static int process_entry(struct merge_options *o,
 			output(o, 1, "CONFLICT (%s): Merge conflict in %s",
 					reason, path);
 
-			if (index_only)
-				update_file(0, mfi.sha, mfi.mode, path);
+			if (o->index_only)
+				update_file(o, 0, mfi.sha, mfi.mode, path);
 			else
-				update_file_flags(mfi.sha, mfi.mode, path,
+				update_file_flags(o, mfi.sha, mfi.mode, path,
 					      0 /* update_cache */, 1 /* update_working_directory */);
 		}
 	} else if (!o_sha && !a_sha && !b_sha) {
@@ -1134,7 +1131,7 @@ static int process_entry(struct merge_options *o,
 		 * this entry was deleted altogether. a_mode == 0 means
 		 * we had that path and want to actively remove it.
 		 */
-		remove_file(1, path, !a_mode);
+		remove_file(o, 1, path, !a_mode);
 	} else
 		die("Fatal merge failure, shouldn't happen.");
 
@@ -1160,7 +1157,7 @@ int merge_trees(struct merge_options *o,
 		return 1;
 	}
 
-	code = git_merge_trees(index_only, common, head, merge);
+	code = git_merge_trees(o->index_only, common, head, merge);
 
 	if (code != 0)
 		die("merging of trees %s and %s failed",
@@ -1195,7 +1192,7 @@ int merge_trees(struct merge_options *o,
 	else
 		clean = 1;
 
-	if (index_only)
+	if (o->index_only)
 		*result = write_tree_from_memory(o);
 
 	return clean;
@@ -1283,14 +1280,14 @@ int merge_recursive(struct merge_options *o,
 	discard_cache();
 	if (!o->call_depth) {
 		read_cache();
-		index_only = 0;
+		o->index_only = 0;
 	} else
-		index_only = 1;
+		o->index_only = 1;
 
 	clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,
 			    &mrtree);
 
-	if (index_only) {
+	if (o->index_only) {
 		*result = make_virtual_commit(mrtree, "merged tree");
 		commit_list_insert(h1, &(*result)->parents);
 		commit_list_insert(h2, &(*result)->parents->next);
diff --git a/merge-recursive.h b/merge-recursive.h
index 4f55374..6622c3e 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -10,6 +10,18 @@ struct merge_options {
 	int diff_rename_limit;
 	int merge_rename_limit;
 	int call_depth;
+	/*
+	 * This variable is used in a number of places but only written
+	 * to in the 'merge_recursive' function.
+	 *
+	 * index_only == 1    => Don't leave any non-stage 0 entries in
+	 *                       the cache and don't update the working
+	 *                       directory.
+	 *               0    => Leave unmerged entries in the cache and
+	 *                       update the working directory.
+	 */
+	int index_only;
+
 };
 
 /* merge_trees() but with recursive ancestor consolidation */
-- 
1.6.0.1
Miklos Vajna· Sep 2, 2008, 22:13 UTC · re: Miklos Vajna · lore

[PATCH] Makefile: add merge_recursive.h to LIB_H

When modifying merge-recursive.h, for example builtin-merge-recursive.c have to be recompiled which was not true till now, causing various runtime errors using an incremental build.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
On Wed, Sep 03, 2008 at 12:05:32AM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> Subject: Re: [PATCH 1/2] merge-recursive: move call_depth to struct
>       merge_options

While testing this one, I got some weird errors, finally they gone after a 'touch builtin-merge-recursive.c', I guess this is the right fix.

 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to Makefile +1 −0
diff --git a/Makefile b/Makefile
index f697618..bec6a84 100644
--- a/Makefile
+++ b/Makefile
@@ -362,6 +362,7 @@ LIB_H += list-objects.h
 LIB_H += ll-merge.h
 LIB_H += log-tree.h
 LIB_H += mailmap.h
+LIB_H += merge_recursive.h
 LIB_H += object.h
 LIB_H += pack.h
 LIB_H += pack-refs.h
-- 
1.6.0.1
Junio C Hamano· Sep 2, 2008, 22:39 UTC · re: Miklos Vajna · lore

Re: [PATCH] Makefile: add merge_recursive.h to LIB_H

Miklos Vajna <vmiklos@frugalware.org> writes:
Show 13 quoted lines
> When modifying merge-recursive.h, for example builtin-merge-recursive.c
> have to be recompiled which was not true till now, causing various
> runtime errors using an incremental build.
>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
>
> On Wed, Sep 03, 2008 at 12:05:32AM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
>> Subject: Re: [PATCH 1/2] merge-recursive: move call_depth to struct
>>       merge_options
>
> While testing this one, I got some weird errors, finally they gone after
> a 'touch builtin-merge-recursive.c', I guess this is the right fix.
Thanks.
Miklos Vajna· Sep 2, 2008, 23:49 UTC · re: Junio C Hamano · lore

[PATCH] Makefile: add merge_recursive.h to LIB_H

When modifying merge-recursive.h, for example builtin-merge-recursive.c have to be recompiled which was not true till now, causing various runtime errors using an incremental build.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
n Tue, Sep 02, 2008 at 03:39:57PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> > While testing this one, I got some weird errors, finally they gone
> > after
> > a 'touch builtin-merge-recursive.c', I guess this is the right fix.
>
> Thanks.
Ugh, typo. It is called 'merge-recursive.h'.
Sorry.
 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to Makefile +1 −0
diff --git a/Makefile b/Makefile
index f697618..1647831 100644
--- a/Makefile
+++ b/Makefile
@@ -362,6 +362,7 @@ LIB_H += list-objects.h
 LIB_H += ll-merge.h
 LIB_H += log-tree.h
 LIB_H += mailmap.h
+LIB_H += merge-recursive.h
 LIB_H += object.h
 LIB_H += pack.h
 LIB_H += pack-refs.h
-- 
1.6.0.1
Junio C Hamano· Sep 2, 2008, 22:39 UTC · re: Miklos Vajna · lore

Re: [PATCH 0/2] Move call_depth and index_only to struct merge_options

Miklos Vajna <vmiklos@frugalware.org> writes:
Show 6 quoted lines
> On Tue, Sep 02, 2008 at 01:02:31PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
>> I found it a bit disturbing that "index_only" and "call_depth" were
>> not
>> part of merge_options structure.
>
> Here are two patches to do it, on top of current mv/merge-recursive.
I suspected that it always holds that "index_only === !!call_depth".

Shouldn't strbuf obuf be part of the merge_options structure that describes the current call status?

Miklos Vajna· Sep 3, 2008, 00:16 UTC · re: Junio C Hamano · lore

[PATCH] merge-recursive: get rid of the index_only global variable

struct merge_options as already a call_depth member, where index_only == !!call_depth. We always use index_only as a condition, so we can just use call_depth instead of index_only.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
On Tue, Sep 02, 2008 at 03:39:33PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> I suspected that it always holds that "index_only === !!call_depth".

Uhm, yes. Here is an updated version that just removes the global index_only and does not touch the header.

The patch is still large, as I still needed to introduce struct merge_options as the first parameter in several functions.

 merge-recursive.c |  140 +++++++++++++++++++++++++---------------------------
 1 files changed, 67 insertions(+), 73 deletions(-)
Show changes to merge-recursive.c +67 −73
diff --git a/merge-recursive.c b/merge-recursive.c
index 5bb20aa..c426589 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -165,17 +165,6 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 	return add_cache_entry(ce, options);
 }
 
-/*
- * This is a global variable which is used in a number of places but
- * only written to in the 'merge' function.
- *
- * index_only == 1    => Don't leave any non-stage 0 entries in the cache and
- *                       don't update the working directory.
- *               0    => Leave unmerged entries in the cache and update
- *                       the working directory.
- */
-static int index_only = 0;
-
 static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
 {
 	parse_tree(tree);
@@ -428,10 +417,11 @@ static int remove_path(const char *name)
 	return ret;
 }
 
-static int remove_file(int clean, const char *path, int no_wd)
+static int remove_file(struct merge_options *o, int clean,
+		       const char *path, int no_wd)
 {
-	int update_cache = index_only || clean;
-	int update_working_directory = !index_only && !no_wd;
+	int update_cache = o->call_depth || clean;
+	int update_working_directory = !o->call_depth && !no_wd;
 
 	if (update_cache) {
 		if (remove_file_from_cache(path))
@@ -509,13 +499,14 @@ static int make_room_for_path(const char *path)
 	return error(msg, path, ": perhaps a D/F conflict?");
 }
 
-static void update_file_flags(const unsigned char *sha,
+static void update_file_flags(struct merge_options *o,
+			      const unsigned char *sha,
 			      unsigned mode,
 			      const char *path,
 			      int update_cache,
 			      int update_wd)
 {
-	if (index_only)
+	if (o->call_depth)
 		update_wd = 0;
 
 	if (update_wd) {
@@ -574,12 +565,13 @@ static void update_file_flags(const unsigned char *sha,
 		add_cacheinfo(mode, sha, path, 0, update_wd, ADD_CACHE_OK_TO_ADD);
 }
 
-static void update_file(int clean,
+static void update_file(struct merge_options *o,
+			int clean,
 			const unsigned char *sha,
 			unsigned mode,
 			const char *path)
 {
-	update_file_flags(sha, mode, path, index_only || clean, !index_only);
+	update_file_flags(o, sha, mode, path, o->call_depth || clean, !o->call_depth);
 }
 
 /* Low level file merging, update and removal */
@@ -609,8 +601,9 @@ static void fill_mm(const unsigned char *sha1, mmfile_t *mm)
 	mm->size = size;
 }
 
-static int merge_3way(mmbuffer_t *result_buf,
-		      struct diff_filespec *o,
+static int merge_3way(struct merge_options *o,
+		      mmbuffer_t *result_buf,
+		      struct diff_filespec *one,
 		      struct diff_filespec *a,
 		      struct diff_filespec *b,
 		      const char *branch1,
@@ -623,13 +616,13 @@ static int merge_3way(mmbuffer_t *result_buf,
 	name1 = xstrdup(mkpath("%s:%s", branch1, a->path));
 	name2 = xstrdup(mkpath("%s:%s", branch2, b->path));
 
-	fill_mm(o->sha1, &orig);
+	fill_mm(one->sha1, &orig);
 	fill_mm(a->sha1, &src1);
 	fill_mm(b->sha1, &src2);
 
 	merge_status = ll_merge(result_buf, a->path, &orig,
 				&src1, name1, &src2, name2,
-				index_only);
+				o->call_depth);
 
 	free(name1);
 	free(name2);
@@ -639,9 +632,12 @@ static int merge_3way(mmbuffer_t *result_buf,
 	return merge_status;
 }
 
-static struct merge_file_info merge_file(struct diff_filespec *o,
-		struct diff_filespec *a, struct diff_filespec *b,
-		const char *branch1, const char *branch2)
+static struct merge_file_info merge_file(struct merge_options *o,
+				         struct diff_filespec *one,
+					 struct diff_filespec *a,
+					 struct diff_filespec *b,
+					 const char *branch1,
+					 const char *branch2)
 {
 	struct merge_file_info result;
 	result.merge = 0;
@@ -657,31 +653,31 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
 			hashcpy(result.sha, b->sha1);
 		}
 	} else {
-		if (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))
+		if (!sha_eq(a->sha1, one->sha1) && !sha_eq(b->sha1, one->sha1))
 			result.merge = 1;
 
 		/*
 		 * Merge modes
 		 */
-		if (a->mode == b->mode || a->mode == o->mode)
+		if (a->mode == b->mode || a->mode == one->mode)
 			result.mode = b->mode;
 		else {
 			result.mode = a->mode;
-			if (b->mode != o->mode) {
+			if (b->mode != one->mode) {
 				result.clean = 0;
 				result.merge = 1;
 			}
 		}
 
-		if (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, o->sha1))
+		if (sha_eq(a->sha1, b->sha1) || sha_eq(a->sha1, one->sha1))
 			hashcpy(result.sha, b->sha1);
-		else if (sha_eq(b->sha1, o->sha1))
+		else if (sha_eq(b->sha1, one->sha1))
 			hashcpy(result.sha, a->sha1);
 		else if (S_ISREG(a->mode)) {
 			mmbuffer_t result_buf;
 			int merge_status;
 
-			merge_status = merge_3way(&result_buf, o, a, b,
+			merge_status = merge_3way(o, &result_buf, one, a, b,
 						  branch1, branch2);
 
 			if ((merge_status < 0) || !result_buf.ptr)
@@ -726,22 +722,22 @@ static void conflict_rename_rename(struct merge_options *o,
 		dst_name1 = del[delp++] = unique_path(ren1_dst, branch1);
 		output(o, 1, "%s is a directory in %s adding as %s instead",
 		       ren1_dst, branch2, dst_name1);
-		remove_file(0, ren1_dst, 0);
+		remove_file(o, 0, ren1_dst, 0);
 	}
 	if (string_list_has_string(&current_directory_set, ren2_dst)) {
 		dst_name2 = del[delp++] = unique_path(ren2_dst, branch2);
 		output(o, 1, "%s is a directory in %s adding as %s instead",
 		       ren2_dst, branch1, dst_name2);
-		remove_file(0, ren2_dst, 0);
+		remove_file(o, 0, ren2_dst, 0);
 	}
-	if (index_only) {
+	if (o->call_depth) {
 		remove_file_from_cache(dst_name1);
 		remove_file_from_cache(dst_name2);
 		/*
 		 * Uncomment to leave the conflicting names in the resulting tree
 		 *
-		 * update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, dst_name1);
-		 * update_file(0, ren2->pair->two->sha1, ren2->pair->two->mode, dst_name2);
+		 * update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode, dst_name1);
+		 * update_file(o, 0, ren2->pair->two->sha1, ren2->pair->two->mode, dst_name2);
 		 */
 	} else {
 		update_stages(dst_name1, NULL, ren1->pair->two, NULL, 1);
@@ -757,8 +753,8 @@ static void conflict_rename_dir(struct merge_options *o,
 {
 	char *new_path = unique_path(ren1->pair->two->path, branch1);
 	output(o, 1, "Renaming %s to %s instead", ren1->pair->one->path, new_path);
-	remove_file(0, ren1->pair->two->path, 0);
-	update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path);
+	remove_file(o, 0, ren1->pair->two->path, 0);
+	update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path);
 	free(new_path);
 }
 
@@ -773,9 +769,9 @@ static void conflict_rename_rename_2(struct merge_options *o,
 	output(o, 1, "Renaming %s to %s and %s to %s instead",
 	       ren1->pair->one->path, new_path1,
 	       ren2->pair->one->path, new_path2);
-	remove_file(0, ren1->pair->two->path, 0);
-	update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path1);
-	update_file(0, ren2->pair->two->sha1, ren2->pair->two->mode, new_path2);
+	remove_file(o, 0, ren1->pair->two->path, 0);
+	update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path1);
+	update_file(o, 0, ren2->pair->two->sha1, ren2->pair->two->mode, new_path2);
 	free(new_path2);
 	free(new_path1);
 }
@@ -867,17 +863,18 @@ static int process_renames(struct merge_options *o,
 				       "rename \"%s\"->\"%s\" in \"%s\"%s",
 				       src, ren1_dst, branch1,
 				       src, ren2_dst, branch2,
-				       index_only ? " (left unresolved)": "");
-				if (index_only) {
+				       o->call_depth ? " (left unresolved)": "");
+				if (o->call_depth) {
 					remove_file_from_cache(src);
-					update_file(0, ren1->pair->one->sha1,
+					update_file(o, 0, ren1->pair->one->sha1,
 						    ren1->pair->one->mode, src);
 				}
 				conflict_rename_rename(o, ren1, branch1, ren2, branch2);
 			} else {
 				struct merge_file_info mfi;
-				remove_file(1, ren1_src, 1);
-				mfi = merge_file(ren1->pair->one,
+				remove_file(o, 1, ren1_src, 1);
+				mfi = merge_file(o,
+						 ren1->pair->one,
 						 ren1->pair->two,
 						 ren2->pair->two,
 						 branch1,
@@ -893,14 +890,14 @@ static int process_renames(struct merge_options *o,
 					       ren1_dst);
 					clean_merge = 0;
 
-					if (!index_only)
+					if (!o->call_depth)
 						update_stages(ren1_dst,
 							      ren1->pair->one,
 							      ren1->pair->two,
 							      ren2->pair->two,
 							      1 /* clear */);
 				}
-				update_file(mfi.clean, mfi.sha, mfi.mode, ren1_dst);
+				update_file(o, mfi.clean, mfi.sha, mfi.mode, ren1_dst);
 			}
 		} else {
 			/* Renamed in 1, maybe changed in 2 */
@@ -909,7 +906,7 @@ static int process_renames(struct merge_options *o,
 			struct diff_filespec src_other, dst_other;
 			int try_merge, stage = a_renames == renames1 ? 3: 2;
 
-			remove_file(1, ren1_src, index_only || stage == 3);
+			remove_file(o, 1, ren1_src, o->call_depth || stage == 3);
 
 			hashcpy(src_other.sha1, ren1->src_entry->stages[stage].sha);
 			src_other.mode = ren1->src_entry->stages[stage].mode;
@@ -931,7 +928,7 @@ static int process_renames(struct merge_options *o,
 				       "and deleted in %s",
 				       ren1_src, ren1_dst, branch1,
 				       branch2);
-				update_file(0, ren1->pair->two->sha1, ren1->pair->two->mode, ren1_dst);
+				update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode, ren1_dst);
 			} else if (!sha_eq(dst_other.sha1, null_sha1)) {
 				const char *new_path;
 				clean_merge = 0;
@@ -942,7 +939,7 @@ static int process_renames(struct merge_options *o,
 				       ren1_dst, branch2);
 				new_path = unique_path(ren1_dst, branch2);
 				output(o, 1, "Adding as %s instead", new_path);
-				update_file(0, dst_other.sha1, dst_other.mode, new_path);
+				update_file(o, 0, dst_other.sha1, dst_other.mode, new_path);
 			} else if ((item = string_list_lookup(ren1_dst, renames2Dst))) {
 				ren2 = item->util;
 				clean_merge = 0;
@@ -969,7 +966,7 @@ static int process_renames(struct merge_options *o,
 					b = ren1->pair->two;
 					a = &src_other;
 				}
-				mfi = merge_file(one, a, b,
+				mfi = merge_file(o, one, a, b,
 						o->branch1, o->branch2);
 
 				if (mfi.clean &&
@@ -991,11 +988,11 @@ static int process_renames(struct merge_options *o,
 						       ren1_dst);
 						clean_merge = 0;
 
-						if (!index_only)
+						if (!o->call_depth)
 							update_stages(ren1_dst,
 								      one, a, b, 1);
 					}
-					update_file(mfi.clean, mfi.sha, mfi.mode, ren1_dst);
+					update_file(o, mfi.clean, mfi.sha, mfi.mode, ren1_dst);
 				}
 			}
 		}
@@ -1037,7 +1034,7 @@ static int process_entry(struct merge_options *o,
 			if (a_sha)
 				output(o, 2, "Removing %s", path);
 			/* do not touch working file if it did not exist */
-			remove_file(1, path, !a_sha);
+			remove_file(o, 1, path, !a_sha);
 		} else {
 			/* Deleted in one and changed in the other */
 			clean_merge = 0;
@@ -1046,13 +1043,13 @@ static int process_entry(struct merge_options *o,
 				       "and modified in %s. Version %s of %s left in tree.",
 				       path, o->branch1,
 				       o->branch2, o->branch2, path);
-				update_file(0, b_sha, b_mode, path);
+				update_file(o, 0, b_sha, b_mode, path);
 			} else {
 				output(o, 1, "CONFLICT (delete/modify): %s deleted in %s "
 				       "and modified in %s. Version %s of %s left in tree.",
 				       path, o->branch2,
 				       o->branch1, o->branch1, path);
-				update_file(0, a_sha, a_mode, path);
+				update_file(o, 0, a_sha, a_mode, path);
 			}
 		}
 
@@ -1084,11 +1081,11 @@ static int process_entry(struct merge_options *o,
 			output(o, 1, "CONFLICT (%s): There is a directory with name %s in %s. "
 			       "Adding %s as %s",
 			       conf, path, other_branch, path, new_path);
-			remove_file(0, path, 0);
-			update_file(0, sha, mode, new_path);
+			remove_file(o, 0, path, 0);
+			update_file(o, 0, sha, mode, new_path);
 		} else {
 			output(o, 2, "Adding %s", path);
-			update_file(1, sha, mode, path);
+			update_file(o, 1, sha, mode, path);
 		}
 	} else if (a_sha && b_sha) {
 		/* Case C: Added in both (check for same permissions) and */
@@ -1110,12 +1107,12 @@ static int process_entry(struct merge_options *o,
 		hashcpy(b.sha1, b_sha);
 		b.mode = b_mode;
 
-		mfi = merge_file(&one, &a, &b,
+		mfi = merge_file(o, &one, &a, &b,
 				 o->branch1, o->branch2);
 
 		clean_merge = mfi.clean;
 		if (mfi.clean)
-			update_file(1, mfi.sha, mfi.mode, path);
+			update_file(o, 1, mfi.sha, mfi.mode, path);
 		else if (S_ISGITLINK(mfi.mode))
 			output(o, 1, "CONFLICT (submodule): Merge conflict in %s "
 			       "- needs %s", path, sha1_to_hex(b.sha1));
@@ -1123,10 +1120,10 @@ static int process_entry(struct merge_options *o,
 			output(o, 1, "CONFLICT (%s): Merge conflict in %s",
 					reason, path);
 
-			if (index_only)
-				update_file(0, mfi.sha, mfi.mode, path);
+			if (o->call_depth)
+				update_file(o, 0, mfi.sha, mfi.mode, path);
 			else
-				update_file_flags(mfi.sha, mfi.mode, path,
+				update_file_flags(o, mfi.sha, mfi.mode, path,
 					      0 /* update_cache */, 1 /* update_working_directory */);
 		}
 	} else if (!o_sha && !a_sha && !b_sha) {
@@ -1134,7 +1131,7 @@ static int process_entry(struct merge_options *o,
 		 * this entry was deleted altogether. a_mode == 0 means
 		 * we had that path and want to actively remove it.
 		 */
-		remove_file(1, path, !a_mode);
+		remove_file(o, 1, path, !a_mode);
 	} else
 		die("Fatal merge failure, shouldn't happen.");
 
@@ -1160,7 +1157,7 @@ int merge_trees(struct merge_options *o,
 		return 1;
 	}
 
-	code = git_merge_trees(index_only, common, head, merge);
+	code = git_merge_trees(o->call_depth, common, head, merge);
 
 	if (code != 0)
 		die("merging of trees %s and %s failed",
@@ -1195,7 +1192,7 @@ int merge_trees(struct merge_options *o,
 	else
 		clean = 1;
 
-	if (index_only)
+	if (o->call_depth)
 		*result = write_tree_from_memory(o);
 
 	return clean;
@@ -1281,16 +1278,13 @@ int merge_recursive(struct merge_options *o,
 	}
 
 	discard_cache();
-	if (!o->call_depth) {
+	if (!o->call_depth)
 		read_cache();
-		index_only = 0;
-	} else
-		index_only = 1;
 
 	clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,
 			    &mrtree);
 
-	if (index_only) {
+	if (o->call_depth) {
 		*result = make_virtual_commit(mrtree, "merged tree");
 		commit_list_insert(h1, &(*result)->parents);
 		commit_list_insert(h2, &(*result)->parents->next);
-- 
1.6.0.1
Miklos Vajna· Sep 3, 2008, 00:39 UTC · re: Junio C Hamano · lore

[PATCH] merge-recursive: move the global obuf to struct merge_options

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
On Tue, Sep 02, 2008 at 03:39:33PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Shouldn't strbuf obuf be part of the merge_options structure that
> describes the current call status?
It can be done, see below. :-)

BTW, there are 3 leftovers: make_virtual_commit()'s virtual_id and the global current_file_set/current_directory_set. I guess all of them could be moved to merge_options as well. (I don't have more time today to do so, but I can do it tomorrow, unless you see some fundamental problem with it.)

 merge-recursive.c |   37 ++++++++++++++++++-------------------
 merge-recursive.h |    1 +
 2 files changed, 19 insertions(+), 19 deletions(-)
Show changes to 2 files +19 −19

merge-recursive.c, merge-recursive.h

diff --git a/merge-recursive.c b/merge-recursive.c
index c426589..d4f12d0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -80,18 +80,16 @@ struct stage_data
 static struct string_list current_file_set = {NULL, 0, 0, 1};
 static struct string_list current_directory_set = {NULL, 0, 0, 1};
 
-static struct strbuf obuf = STRBUF_INIT;
-
 static int show(struct merge_options *o, int v)
 {
 	return (!o->call_depth && o->verbosity >= v) || o->verbosity >= 5;
 }
 
-static void flush_output(void)
+static void flush_output(struct merge_options *o)
 {
-	if (obuf.len) {
-		fputs(obuf.buf, stdout);
-		strbuf_reset(&obuf);
+	if (o->obuf.len) {
+		fputs(o->obuf.buf, stdout);
+		strbuf_reset(&o->obuf);
 	}
 }
 
@@ -103,35 +101,35 @@ static void output(struct merge_options *o, int v, const char *fmt, ...)
 	if (!show(o, v))
 		return;
 
-	strbuf_grow(&obuf, o->call_depth * 2 + 2);
-	memset(obuf.buf + obuf.len, ' ', o->call_depth * 2);
-	strbuf_setlen(&obuf, obuf.len + o->call_depth * 2);
+	strbuf_grow(&o->obuf, o->call_depth * 2 + 2);
+	memset(o->obuf.buf + o->obuf.len, ' ', o->call_depth * 2);
+	strbuf_setlen(&o->obuf, o->obuf.len + o->call_depth * 2);
 
 	va_start(ap, fmt);
-	len = vsnprintf(obuf.buf + obuf.len, strbuf_avail(&obuf), fmt, ap);
+	len = vsnprintf(o->obuf.buf + o->obuf.len, strbuf_avail(&o->obuf), fmt, ap);
 	va_end(ap);
 
 	if (len < 0)
 		len = 0;
-	if (len >= strbuf_avail(&obuf)) {
-		strbuf_grow(&obuf, len + 2);
+	if (len >= strbuf_avail(&o->obuf)) {
+		strbuf_grow(&o->obuf, len + 2);
 		va_start(ap, fmt);
-		len = vsnprintf(obuf.buf + obuf.len, strbuf_avail(&obuf), fmt, ap);
+		len = vsnprintf(o->obuf.buf + o->obuf.len, strbuf_avail(&o->obuf), fmt, ap);
 		va_end(ap);
-		if (len >= strbuf_avail(&obuf)) {
+		if (len >= strbuf_avail(&o->obuf)) {
 			die("this should not happen, your snprintf is broken");
 		}
 	}
-	strbuf_setlen(&obuf, obuf.len + len);
-	strbuf_add(&obuf, "\n", 1);
+	strbuf_setlen(&o->obuf, o->obuf.len + len);
+	strbuf_add(&o->obuf, "\n", 1);
 	if (!o->buffer_output)
-		flush_output();
+		flush_output(o);
 }
 
 static void output_commit_title(struct merge_options *o, struct commit *commit)
 {
 	int i;
-	flush_output();
+	flush_output(o);
 	for (i = o->call_depth; i--;)
 		fputs("  ", stdout);
 	if (commit->util)
@@ -1289,7 +1287,7 @@ int merge_recursive(struct merge_options *o,
 		commit_list_insert(h1, &(*result)->parents);
 		commit_list_insert(h2, &(*result)->parents->next);
 	}
-	flush_output();
+	flush_output(o);
 	return clean;
 }
 
@@ -1375,4 +1373,5 @@ void init_merge_options(struct merge_options *o)
 			strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10);
 	if (o->verbosity >= 5)
 		o->buffer_output = 0;
+	strbuf_init(&o->obuf, 0);
 }
diff --git a/merge-recursive.h b/merge-recursive.h
index 4f55374..be84d9b 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -10,6 +10,7 @@ struct merge_options {
 	int diff_rename_limit;
 	int merge_rename_limit;
 	int call_depth;
+	struct strbuf obuf;
 };
 
 /* merge_trees() but with recursive ancestor consolidation */
-- 
1.6.0.1
Miklos Vajna· Sep 3, 2008, 17:34 UTC · re: Miklos Vajna · lore

Re: [PATCH] merge-recursive: move the global obuf to struct merge_options

On Wed, Sep 03, 2008 at 02:39:09AM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
Show 5 quoted lines
> BTW, there are 3 leftovers: make_virtual_commit()'s virtual_id and the
> global current_file_set/current_directory_set. I guess all of them
> could be moved to merge_options as well. (I don't have more time today
> to do so, but I can do it tomorrow, unless you see some fundamental
> problem with it.)
Here are the two patches, that do this.

These, and the 4 other related patches which are not yet in git.git at the moment are also available in the 'merge-recursive' branch of git://repo.or.cz/git/vmiklos.git.

Miklos Vajna (2):
  merge-recursive: move current_{file,directory}_set to struct
    merge_options
  merge-recursive: move make_virtual_commit()'s virtual_id to
    merge_options
 merge-recursive.c |   80 ++++++++++++++++++++++++++++------------------------
 merge-recursive.h |    5 +++
 2 files changed, 48 insertions(+), 37 deletions(-)
Miklos Vajna· Sep 3, 2008, 17:34 UTC · re: Miklos Vajna · lore

[PATCH 1/2] merge-recursive: move current_{file,directory}_set to struct merge_options

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 merge-recursive.c |   56 +++++++++++++++++++++++++++-------------------------
 merge-recursive.h |    4 +++
 2 files changed, 33 insertions(+), 27 deletions(-)
Show changes to 2 files +33 −27

merge-recursive.c, merge-recursive.h

diff --git a/merge-recursive.c b/merge-recursive.c
index d4f12d0..964b8f3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -77,9 +77,6 @@ struct stage_data
 	unsigned processed:1;
 };
 
-static struct string_list current_file_set = {NULL, 0, 0, 1};
-static struct string_list current_directory_set = {NULL, 0, 0, 1};
-
 static int show(struct merge_options *o, int v)
 {
 	return (!o->call_depth && o->verbosity >= v) || o->verbosity >= 5;
@@ -235,22 +232,23 @@ static int save_files_dirs(const unsigned char *sha1,
 	memcpy(newpath, base, baselen);
 	memcpy(newpath + baselen, path, len);
 	newpath[baselen + len] = '\0';
+	struct merge_options *o = context;
 
 	if (S_ISDIR(mode))
-		string_list_insert(newpath, &current_directory_set);
+		string_list_insert(newpath, &o->current_directory_set);
 	else
-		string_list_insert(newpath, &current_file_set);
+		string_list_insert(newpath, &o->current_file_set);
 	free(newpath);
 
 	return READ_TREE_RECURSIVE;
 }
 
-static int get_files_dirs(struct tree *tree)
+static int get_files_dirs(struct merge_options *o, struct tree *tree)
 {
 	int n;
-	if (read_tree_recursive(tree, "", 0, 0, NULL, save_files_dirs, NULL))
+	if (read_tree_recursive(tree, "", 0, 0, NULL, save_files_dirs, o))
 		return 0;
-	n = current_file_set.nr + current_directory_set.nr;
+	n = o->current_file_set.nr + o->current_directory_set.nr;
 	return n;
 }
 
@@ -434,7 +432,7 @@ static int remove_file(struct merge_options *o, int clean,
 	return 0;
 }
 
-static char *unique_path(const char *path, const char *branch)
+static char *unique_path(struct merge_options *o, const char *path, const char *branch)
 {
 	char *newpath = xmalloc(strlen(path) + 1 + strlen(branch) + 8 + 1);
 	int suffix = 0;
@@ -446,12 +444,12 @@ static char *unique_path(const char *path, const char *branch)
 	for (; *p; ++p)
 		if ('/' == *p)
 			*p = '_';
-	while (string_list_has_string(&current_file_set, newpath) ||
-	       string_list_has_string(&current_directory_set, newpath) ||
+	while (string_list_has_string(&o->current_file_set, newpath) ||
+	       string_list_has_string(&o->current_directory_set, newpath) ||
 	       lstat(newpath, &st) == 0)
 		sprintf(p, "_%d", suffix++);
 
-	string_list_insert(newpath, &current_file_set);
+	string_list_insert(newpath, &o->current_file_set);
 	return newpath;
 }
 
@@ -716,14 +714,14 @@ static void conflict_rename_rename(struct merge_options *o,
 	const char *ren2_dst = ren2->pair->two->path;
 	const char *dst_name1 = ren1_dst;
 	const char *dst_name2 = ren2_dst;
-	if (string_list_has_string(&current_directory_set, ren1_dst)) {
-		dst_name1 = del[delp++] = unique_path(ren1_dst, branch1);
+	if (string_list_has_string(&o->current_directory_set, ren1_dst)) {
+		dst_name1 = del[delp++] = unique_path(o, ren1_dst, branch1);
 		output(o, 1, "%s is a directory in %s adding as %s instead",
 		       ren1_dst, branch2, dst_name1);
 		remove_file(o, 0, ren1_dst, 0);
 	}
-	if (string_list_has_string(&current_directory_set, ren2_dst)) {
-		dst_name2 = del[delp++] = unique_path(ren2_dst, branch2);
+	if (string_list_has_string(&o->current_directory_set, ren2_dst)) {
+		dst_name2 = del[delp++] = unique_path(o, ren2_dst, branch2);
 		output(o, 1, "%s is a directory in %s adding as %s instead",
 		       ren2_dst, branch1, dst_name2);
 		remove_file(o, 0, ren2_dst, 0);
@@ -749,7 +747,7 @@ static void conflict_rename_dir(struct merge_options *o,
 				struct rename *ren1,
 				const char *branch1)
 {
-	char *new_path = unique_path(ren1->pair->two->path, branch1);
+	char *new_path = unique_path(o, ren1->pair->two->path, branch1);
 	output(o, 1, "Renaming %s to %s instead", ren1->pair->one->path, new_path);
 	remove_file(o, 0, ren1->pair->two->path, 0);
 	update_file(o, 0, ren1->pair->two->sha1, ren1->pair->two->mode, new_path);
@@ -762,8 +760,8 @@ static void conflict_rename_rename_2(struct merge_options *o,
 				     struct rename *ren2,
 				     const char *branch2)
 {
-	char *new_path1 = unique_path(ren1->pair->two->path, branch1);
-	char *new_path2 = unique_path(ren2->pair->two->path, branch2);
+	char *new_path1 = unique_path(o, ren1->pair->two->path, branch1);
+	char *new_path2 = unique_path(o, ren2->pair->two->path, branch2);
 	output(o, 1, "Renaming %s to %s and %s to %s instead",
 	       ren1->pair->one->path, new_path1,
 	       ren2->pair->one->path, new_path2);
@@ -913,7 +911,7 @@ static int process_renames(struct merge_options *o,
 
 			try_merge = 0;
 
-			if (string_list_has_string(&current_directory_set, ren1_dst)) {
+			if (string_list_has_string(&o->current_directory_set, ren1_dst)) {
 				clean_merge = 0;
 				output(o, 1, "CONFLICT (rename/directory): Rename %s->%s in %s "
 				       " directory %s added in %s",
@@ -935,7 +933,7 @@ static int process_renames(struct merge_options *o,
 				       "%s added in %s",
 				       ren1_src, ren1_dst, branch1,
 				       ren1_dst, branch2);
-				new_path = unique_path(ren1_dst, branch2);
+				new_path = unique_path(o, ren1_dst, branch2);
 				output(o, 1, "Adding as %s instead", new_path);
 				update_file(o, 0, dst_other.sha1, dst_other.mode, new_path);
 			} else if ((item = string_list_lookup(ren1_dst, renames2Dst))) {
@@ -1073,8 +1071,8 @@ static int process_entry(struct merge_options *o,
 			sha = b_sha;
 			conf = "directory/file";
 		}
-		if (string_list_has_string(&current_directory_set, path)) {
-			const char *new_path = unique_path(path, add_branch);
+		if (string_list_has_string(&o->current_directory_set, path)) {
+			const char *new_path = unique_path(o, path, add_branch);
 			clean_merge = 0;
 			output(o, 1, "CONFLICT (%s): There is a directory with name %s in %s. "
 			       "Adding %s as %s",
@@ -1165,10 +1163,10 @@ int merge_trees(struct merge_options *o,
 	if (unmerged_cache()) {
 		struct string_list *entries, *re_head, *re_merge;
 		int i;
-		string_list_clear(&current_file_set, 1);
-		string_list_clear(&current_directory_set, 1);
-		get_files_dirs(head);
-		get_files_dirs(merge);
+		string_list_clear(&o->current_file_set, 1);
+		string_list_clear(&o->current_directory_set, 1);
+		get_files_dirs(o, head);
+		get_files_dirs(o, merge);
 
 		entries = get_unmerged();
 		re_head  = get_renames(o, head, common, head, merge, entries);
@@ -1374,4 +1372,8 @@ void init_merge_options(struct merge_options *o)
 	if (o->verbosity >= 5)
 		o->buffer_output = 0;
 	strbuf_init(&o->obuf, 0);
+	memset(&o->current_file_set, 0, sizeof(struct string_list));
+	o->current_file_set.strdup_strings = 1;
+	memset(&o->current_directory_set, 0, sizeof(struct string_list));
+	o->current_directory_set.strdup_strings = 1;
 }
diff --git a/merge-recursive.h b/merge-recursive.h
index be84d9b..fd138ca 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -1,6 +1,8 @@
 #ifndef MERGE_RECURSIVE_H
 #define MERGE_RECURSIVE_H
 
+#include "string-list.h"
+
 struct merge_options {
 	const char *branch1;
 	const char *branch2;
@@ -11,6 +13,8 @@ struct merge_options {
 	int merge_rename_limit;
 	int call_depth;
 	struct strbuf obuf;
+	struct string_list current_file_set;
+	struct string_list current_directory_set;
 };
 
 /* merge_trees() but with recursive ancestor consolidation */
-- 
1.6.0.1
Miklos Vajna· Sep 3, 2008, 17:34 UTC · re: Miklos Vajna · lore

[PATCH 2/2] merge-recursive: move make_virtual_commit()'s virtual_id to merge_options

This is the last patch in this series, now all static variables in merge-recursive.c are moved to struct merge_options.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 merge-recursive.c |   24 ++++++++++++++----------
 merge-recursive.h |    1 +
 2 files changed, 15 insertions(+), 10 deletions(-)
Show changes to 2 files +15 −10

merge-recursive.c, merge-recursive.h

diff --git a/merge-recursive.c b/merge-recursive.c
index 964b8f3..4fa3308 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -40,13 +40,14 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two)
  * - *(int *)commit->object.sha1 set to the virtual id.
  */
 
-struct commit *make_virtual_commit(struct tree *tree, const char *comment)
+struct commit *make_virtual_commit(struct merge_options *o,
+				   struct tree *tree,
+				   const char *comment)
 {
 	struct commit *commit = xcalloc(1, sizeof(struct commit));
-	static unsigned virtual_id = 1;
 	commit->tree = tree;
 	commit->util = (void*)comment;
-	*(int*)commit->object.sha1 = virtual_id++;
+	*(int*)commit->object.sha1 = o->virtual_id++;
 	/* avoid warnings */
 	commit->object.parsed = 1;
 	return commit;
@@ -1245,7 +1246,7 @@ int merge_recursive(struct merge_options *o,
 		tree->object.parsed = 1;
 		tree->object.type = OBJ_TREE;
 		pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
-		merged_common_ancestors = make_virtual_commit(tree, "ancestor");
+		merged_common_ancestors = make_virtual_commit(o, tree, "ancestor");
 	}
 
 	for (iter = ca; iter; iter = iter->next) {
@@ -1281,7 +1282,7 @@ int merge_recursive(struct merge_options *o,
 			    &mrtree);
 
 	if (o->call_depth) {
-		*result = make_virtual_commit(mrtree, "merged tree");
+		*result = make_virtual_commit(o, mrtree, "merged tree");
 		commit_list_insert(h1, &(*result)->parents);
 		commit_list_insert(h2, &(*result)->parents->next);
 	}
@@ -1289,7 +1290,9 @@ int merge_recursive(struct merge_options *o,
 	return clean;
 }
 
-static struct commit *get_ref(const unsigned char *sha1, const char *name)
+static struct commit *get_ref(struct merge_options *o,
+			      const unsigned char *sha1,
+			      const char *name)
 {
 	struct object *object;
 
@@ -1297,7 +1300,7 @@ static struct commit *get_ref(const unsigned char *sha1, const char *name)
 	if (!object)
 		return NULL;
 	if (object->type == OBJ_TREE)
-		return make_virtual_commit((struct tree*)object, name);
+		return make_virtual_commit(o, (struct tree*)object, name);
 	if (object->type != OBJ_COMMIT)
 		return NULL;
 	if (parse_commit((struct commit *)object))
@@ -1314,15 +1317,15 @@ int merge_recursive_generic(struct merge_options *o,
 {
 	int clean, index_fd;
 	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
-	struct commit *head_commit = get_ref(head, o->branch1);
-	struct commit *next_commit = get_ref(merge, o->branch2);
+	struct commit *head_commit = get_ref(o, head, o->branch1);
+	struct commit *next_commit = get_ref(o, merge, o->branch2);
 	struct commit_list *ca = NULL;
 
 	if (base_list) {
 		int i;
 		for (i = 0; i < num_base_list; ++i) {
 			struct commit *base;
-			if (!(base = get_ref(base_list[i], sha1_to_hex(base_list[i]))))
+			if (!(base = get_ref(o, base_list[i], sha1_to_hex(base_list[i]))))
 				return error("Could not parse object '%s'",
 					sha1_to_hex(base_list[i]));
 			commit_list_insert(base, &ca);
@@ -1376,4 +1379,5 @@ void init_merge_options(struct merge_options *o)
 	o->current_file_set.strdup_strings = 1;
 	memset(&o->current_directory_set, 0, sizeof(struct string_list));
 	o->current_directory_set.strdup_strings = 1;
+	o->virtual_id = 1;
 }
diff --git a/merge-recursive.h b/merge-recursive.h
index fd138ca..bc23fe0 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -15,6 +15,7 @@ struct merge_options {
 	struct strbuf obuf;
 	struct string_list current_file_set;
 	struct string_list current_directory_set;
+	unsigned virtual_id;
 };
 
 /* merge_trees() but with recursive ancestor consolidation */
-- 
1.6.0.1
Junio C Hamano· Sep 4, 2008, 19:03 UTC · re: Miklos Vajna · lore

Re: [PATCH 2/2] merge-recursive: move make_virtual_commit()'s virtual_id to merge_options

I do not think this one is a good idea.

What does it mean for a two "virtual commits" that are returned from separate calls to make_virtual_commit() to have the same virtual_id?

I do not know offhand if the existing code does something like:
	commit = lookup_commit(commit->object.sha1)
or
	if (hashcmp(commit1->object.sha1, commit2->object.sha1))
        	...

but if there is such a code, I think this change makes the problem "virtual id" has even worse. With the current single function local static "virtual_id", at least during a program's lifetime it is guaranteed that there won't be any two instances of virtual commits created for different purposes that share the same (fake) sha1 value. If you call merge_recursive more than once in your process, using a fresh "struct merge_options" each time, that guarantee is lost with your change.

The only purpose of a "virtual commit", as I understand it, is to allow you to pass a tree resulting from an internal merge to a function that expects you to call with three commit objects to come up with a new tree that is the result of the merge. The code stores a "virtual_id" as a phoney sha1 value in the object, but I do not think the actual value is used for anything but debugging purposes (Alex, Dscho, please correct me as necessary).

Does it hurt if we get rid of virtual_id and always leave the object->sha1 field of virtual commits 0{40} as it is initialized?

I further suspect we _could_ fix the API that requires you to pass three commits to accept three trees instead and get rid of virtual commits altogether, but then we would lose an easy access to the message of commits that are being merged, and we would need to pass these strings as separate parameters (or part of merge_options) --- which might be a good clean-up in a longer run, but I do not think it is absolutely necessary during this round.

Miklos Vajna· Sep 5, 2008, 17:26 UTC · re: Junio C Hamano · lore

[PATCH] merge-recursive: get rid of virtual_id

We now just leave the object->sha1 field of virtual commits 0{40} as it is initialized, as a unique hash is not necessary in case of virtual commits.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
On Thu, Sep 04, 2008 at 12:03:08PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Does it hurt if we get rid of virtual_id and always leave the
> object->sha1 field of virtual commits 0{40} as it is initialized?
I don't think so. Here is a patch that does it.
 merge-recursive.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)
Show changes to merge-recursive.c +1 −5
diff --git a/merge-recursive.c b/merge-recursive.c
index 1c24c31..dbdb9ac 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -35,18 +35,14 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two)
 }
 
 /*
- * A virtual commit has
- * - (const char *)commit->util set to the name, and
- * - *(int *)commit->object.sha1 set to the virtual id.
+ * A virtual commit has (const char *)commit->util set to the name.
  */
 
 struct commit *make_virtual_commit(struct tree *tree, const char *comment)
 {
 	struct commit *commit = xcalloc(1, sizeof(struct commit));
-	static unsigned virtual_id = 1;
 	commit->tree = tree;
 	commit->util = (void*)comment;
-	*(int*)commit->object.sha1 = virtual_id++;
 	/* avoid warnings */
 	commit->object.parsed = 1;
 	return commit;
-- 
1.6.0.1
Junio C Hamano· Sep 4, 2008, 19:05 UTC · re: Miklos Vajna · lore

Re: [PATCH] merge-recursive: move the global obuf to struct merge_options

Miklos Vajna <vmiklos@frugalware.org> writes:
> These, and the 4 other related patches which are not yet in git.git at
> the moment are also available in the 'merge-recursive' branch of
> git://repo.or.cz/git/vmiklos.git.
Thanks.
I already expressed my doubts about "move make_virtual_commit()".

"add merge-recursive to LIB_H" should go to 'maint' as a build-fix, so it does not have to be on this topic. If this is meant to be a pullable topic branch, I'd prefer if you didn't have that commit.

I cherry picked call-depth, index-only removal, obuf and current_{f/d} but haven't pushed the results out.

← back to recent threads