[v3,03/24] merge-recursive: enforce opt->ancestor != NULL when calling merge_trees()
diff mbox series

Message ID 20190815214053.16594-4-newren@gmail.com
State New
Headers show
Series
  • Clean up merge API
Related show

Commit Message

Elijah Newren Aug. 15, 2019, 9:40 p.m. UTC
We always want our conflict hunks to be labelled so that users can know
where each came from.  The previous commit fixed the one caller in the
codebase which was not setting opt->ancestor (and thus not providing a
label for the "merge base" conflict hunk in diff3-style conflict
markers); add an assertion to prevent future codepaths from also
overlooking this requirement.

Enforcing this requirement also allows us to simplify the code for
labelling the conflict hunks by no longer checking if the ancestor label
is NULL.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Aug. 16, 2019, 9:05 p.m. UTC | #1
Elijah Newren <newren@gmail.com> writes:

> We always want our conflict hunks to be labelled so that users can know
> where each came from.  The previous commit fixed the one caller in the
> codebase which was not setting opt->ancestor (and thus not providing a
> label for the "merge base" conflict hunk in diff3-style conflict
> markers); add an assertion to prevent future codepaths from also
> overlooking this requirement.

Makes sense.

Patch
diff mbox series

diff --git a/merge-recursive.c b/merge-recursive.c
index 1d960fa64b..a67ea4957a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1019,7 +1019,7 @@  static int merge_3way(struct merge_options *opt,
 {
 	mmfile_t orig, src1, src2;
 	struct ll_merge_options ll_opts = {0};
-	char *base_name, *name1, *name2;
+	char *base, *name1, *name2;
 	int merge_status;
 
 	ll_opts.renormalize = opt->renormalize;
@@ -1043,16 +1043,13 @@  static int merge_3way(struct merge_options *opt,
 		}
 	}
 
-	assert(a->path && b->path && o->path);
-	if (strcmp(a->path, b->path) ||
-	    (opt->ancestor != NULL && strcmp(a->path, o->path) != 0)) {
-		base_name = opt->ancestor == NULL ? NULL :
-			mkpathdup("%s:%s", opt->ancestor, o->path);
+	assert(a->path && b->path && o->path && opt->ancestor);
+	if (strcmp(a->path, b->path) || strcmp(a->path, o->path) != 0) {
+		base  = mkpathdup("%s:%s", opt->ancestor, o->path);
 		name1 = mkpathdup("%s:%s", branch1, a->path);
 		name2 = mkpathdup("%s:%s", branch2, b->path);
 	} else {
-		base_name = opt->ancestor == NULL ? NULL :
-			mkpathdup("%s", opt->ancestor);
+		base  = mkpathdup("%s", opt->ancestor);
 		name1 = mkpathdup("%s", branch1);
 		name2 = mkpathdup("%s", branch2);
 	}
@@ -1061,11 +1058,11 @@  static int merge_3way(struct merge_options *opt,
 	read_mmblob(&src1, &a->oid);
 	read_mmblob(&src2, &b->oid);
 
-	merge_status = ll_merge(result_buf, a->path, &orig, base_name,
+	merge_status = ll_merge(result_buf, a->path, &orig, base,
 				&src1, name1, &src2, name2,
 				opt->repo->index, &ll_opts);
 
-	free(base_name);
+	free(base);
 	free(name1);
 	free(name2);
 	free(orig.ptr);
@@ -3390,6 +3387,8 @@  int merge_trees(struct merge_options *opt,
 	int code, clean;
 	struct strbuf sb = STRBUF_INIT;
 
+	assert(opt->ancestor != NULL);
+
 	if (!opt->call_depth && repo_index_has_changes(opt->repo, head, &sb)) {
 		err(opt, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
 		    sb.buf);