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

9 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:17

Miklos Vajna [off-list ref] writes:

On Tue, Sep 02, 2008 at 01:02:31PM -0700, Junio C Hamano [off-list ref] wrote:

quoted

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?

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

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:17

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 <redacted>
---

On Tue, Sep 02, 2008 at 03:39:33PM -0700, Junio C Hamano [off-list ref] 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(-)

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

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

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:17

Signed-off-by: Miklos Vajna <redacted>
---

On Tue, Sep 02, 2008 at 03:39:33PM -0700, Junio C Hamano [off-list ref] 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(-)

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

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

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:17

On Wed, Sep 03, 2008 at 02:39:09AM +0200, Miklos Vajna [off-list ref] wrote:

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(-)

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

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:17

Signed-off-by: Miklos Vajna <redacted>
---
 merge-recursive.c |   56 +++++++++++++++++++++++++++-------------------------
 merge-recursive.h |    4 +++
 2 files changed, 33 insertions(+), 27 deletions(-)

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

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

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:17

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 <redacted>
---
 merge-recursive.c |   24 ++++++++++++++----------
 merge-recursive.h |    1 +
 2 files changed, 15 insertions(+), 10 deletions(-)

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

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:17

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.

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:17

Miklos Vajna [off-list ref] 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.

[PATCH] merge-recursive: get rid of virtual_id

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:19

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 <redacted>
---

On Thu, Sep 04, 2008 at 12:03:08PM -0700, Junio C Hamano [off-list ref] 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(-)

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help