merge-recursive code cleanups

PATCH Superseded Elijah Newren 3 participants 27 messages May 19, 2018 – Jun 10, 2018

Share & tools

Elijah Newren

author

2018-05-19 02:07 UTC #0 lore raw

cover merge-recursive code cleanups

This patch series contains several small code cleanups for merge-recursive. I have removed a couple small cleanup chunks in order to avoid conflicts with any other in-flight topics in pu (namely, nd/commit-util-to-slab and sb/submodule-merge-in-merge-recursive). I may resend those later separately.

The series was made on top of next (specifically commit c95db04db ("Merge branch 'sb/object-store-replace' into next")); it will not apply to master.

Elijah Newren (5):
merge-recursive: fix miscellaneous grammar error in comment
merge-recursive: fix numerous argument alignment issues
merge-recursive: clarify the rename_dir/RENAME_DIR meaning
merge-recursive: rename conflict_rename_*() family of functions
merge-recursive: simplify handle_change_delete

 merge-recursive.c | 173 ++++++++++++++++++++++------------------------
 1 file changed, 83 insertions(+), 90 deletions(-)

Signature · 1 line

2.17.0.847.g20b8963732

Elijah Newren

author

2018-05-19 02:07 UTC #1 lore raw

PATCH 1/5 merge-recursive: fix miscellaneous grammar error in comment

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

merge-recursive.c +1 -1

index 35df695fa4..c430fd72bc 100644
@@ -531,7 +531,7 @@ static void record_df_conflict_files(struct merge_options *o,
 				     struct string_list *entries)
 {
 	/* If there is a D/F conflict and the file for such a conflict
-	 * currently exist in the working tree, we want to allow it to be
+	 * currently exists in the working tree, we want to allow it to be
 	 * removed to make room for the corresponding directory if needed.
 	 * The files underneath the directories of such D/F conflicts will
 	 * be processed before the corresponding file involved in the D/F

Signature · 1 line

2.17.0.847.g20b8963732

Elijah Newren

author

2018-05-19 02:07 UTC #2 lore raw

PATCH 2/5 merge-recursive: fix numerous argument alignment issues

In reply to Elijah Newren

Various refactorings throughout the code have left lots of alignment issues that were driving me crazy; fix them.

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

merge-recursive.c +24 ~~-23~~

index c430fd72bc..01306c87eb 100644
@@ -308,8 +308,8 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)
 }
 
 static int add_cacheinfo(struct merge_options *o,
-		unsigned int mode, const struct object_id *oid,
-		const char *path, int stage, int refresh, int options)
+			 unsigned int mode, const struct object_id *oid,
+			 const char *path, int stage, int refresh, int options)
 {
 	struct cache_entry *ce;
 	int ret;

@@ -409,8 +409,8 @@ struct tree *write_tree_from_memory(struct merge_options *o)
 }
 
 static int save_files_dirs(const struct object_id *oid,
-		struct strbuf *base, const char *path,
-		unsigned int mode, int stage, void *context)
+			   struct strbuf *base, const char *path,
+			   unsigned int mode, int stage, void *context)
 {
 	struct path_hashmap_entry *entry;
 	int baselen = base->len;

@@ -1257,13 +1257,13 @@ static int conflict_rename_dir(struct merge_options *o,
 }
 
 static int handle_change_delete(struct merge_options *o,
-				 const char *path, const char *old_path,
-				 const struct object_id *o_oid, int o_mode,
-				 const struct object_id *changed_oid,
-				 int changed_mode,
-				 const char *change_branch,
-				 const char *delete_branch,
-				 const char *change, const char *change_past)
+				const char *path, const char *old_path,
+				const struct object_id *o_oid, int o_mode,
+				const struct object_id *changed_oid,
+				int changed_mode,
+				const char *change_branch,
+				const char *delete_branch,
+				const char *change, const char *change_past)
 {
 	char *alt_path = NULL;
 	const char *update_path = path;

@@ -1324,9 +1324,9 @@ static int handle_change_delete(struct merge_options *o,
 }
 
 static int conflict_rename_delete(struct merge_options *o,
-				   struct diff_filepair *pair,
-				   const char *rename_branch,
-				   const char *delete_branch)
+				  struct diff_filepair *pair,
+				  const char *rename_branch,
+				  const char *delete_branch)
 {
 	const struct diff_filespec *orig = pair->one;
 	const struct diff_filespec *dest = pair->two;

@@ -1429,7 +1429,7 @@ static int handle_file(struct merge_options *o,
 }
 
 static int conflict_rename_rename_1to2(struct merge_options *o,
-					struct rename_conflict_info *ci)
+				       struct rename_conflict_info *ci)
 {
 	/* One file was renamed in both branches, but to different names. */
 	struct diff_filespec *one = ci->pair1->one;

@@ -1491,7 +1491,7 @@ static int conflict_rename_rename_1to2(struct merge_options *o,
 }
 
 static int conflict_rename_rename_2to1(struct merge_options *o,
-					struct rename_conflict_info *ci)
+				       struct rename_conflict_info *ci)
 {
 	/* Two files, a & b, were renamed to the same thing, c. */
 	struct diff_filespec *a = ci->pair1->one;

@@ -2710,7 +2710,8 @@ static struct object_id *stage_oid(const struct object_id *oid, unsigned mode)
 }
 
 static int read_oid_strbuf(struct merge_options *o,
-	const struct object_id *oid, struct strbuf *dst)
+			   const struct object_id *oid,
+			   struct strbuf *dst)
 {
 	void *buf;
 	enum object_type type;

@@ -2763,10 +2764,10 @@ static int blob_unchanged(struct merge_options *opt,
 }
 
 static int handle_modify_delete(struct merge_options *o,
-				 const char *path,
-				 struct object_id *o_oid, int o_mode,
-				 struct object_id *a_oid, int a_mode,
-				 struct object_id *b_oid, int b_mode)
+				const char *path,
+				struct object_id *o_oid, int o_mode,
+				struct object_id *a_oid, int a_mode,
+				struct object_id *b_oid, int b_mode)
 {
 	const char *modify_branch, *delete_branch;
 	struct object_id *changed_oid;

@@ -3303,14 +3304,14 @@ int merge_recursive_generic(struct merge_options *o,
 			struct commit *base;
 			if (!(base = get_ref(base_list[i], oid_to_hex(base_list[i]))))
 				return err(o, _("Could not parse object '%s'"),
-					oid_to_hex(base_list[i]));
+					   oid_to_hex(base_list[i]));
 			commit_list_insert(base, &ca);
 		}
 	}
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	clean = merge_recursive(o, head_commit, next_commit, ca,
-			result);
+				result);
 	if (clean < 0) {
 		rollback_lock_file(&lock);
 		return clean;

Signature · 1 line

2.17.0.847.g20b8963732

Johannes Schindelin

2018-05-21 13:42 UTC #3 lore raw

Re: [PATCH 2/5] merge-recursive: fix numerous argument alignment issues

Hi Elijah,

Quoted 2 lines · Elijah Newren

Various refactorings throughout the code have left lots of alignment issues that were driving me crazy; fix them.

I hope you did not do that manually. What is your code formatting tool of choice?

The patch looks obviously good to me.

Ciao,
Dscho

Elijah Newren

author

2018-05-21 16:48 UTC #4 lore raw

Re: [PATCH 2/5] merge-recursive: fix numerous argument alignment issues

Hi Dscho,

Quoted 9 lines · Johannes Schindelin

Hi Elijah,
Quoted 2 lines · Elijah Newren
Various refactorings throughout the code have left lots of alignment issues that were driving me crazy; fix them.
I hope you did not do that manually. What is your code formatting tool of choice?

Sorry to disappoint but it was manual. I noticed and fixed one of them many months ago, tossing it into a 'misc' branch. Then ran across another and added it. When I hit the third, I was annoyed and cleaned them all up -- and combined them with other changes into this series.

However, it's hard to call this formatting entirely manual. A quick regex found the relevant sites pretty easily, and 'M-x indent-region' (emacs) fixes the indentation for a block of lines all at once. I guess if I had taken the time to fix a few other emacs formatting rules, I could have highlighted the whole file and ran C-M-\ (a.k.a. indent-region), but didn't.

Quoted 1 line

The patch looks obviously good to me.

Thanks for taking a look!

Elijah Newren

author

2018-05-19 02:07 UTC #5 lore raw

PATCH 3/5 merge-recursive: clarify the rename_dir/RENAME_DIR meaning

In reply to Elijah Newren

We had an enum of rename types which included RENAME_DIR; this name felt misleading since it was not about an entire directory but was a status for each individual file add that occurred within a renamed directory. Since this type is for signifying that the files in question were being renamed due to directory rename detection, rename this enum value to RENAME_VIA_DIR. Make a similar change to the conflict_rename_dir() function, and add a comment to the top of that function explaining its purpose (it may not be quite as obvious as for the other conflict_rename_*() functions).

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

merge-recursive.c +17 ~~-11~~

index 01306c87eb..d30085d9c7 100644
@@ -180,7 +180,7 @@ static int oid_eq(const struct object_id *a, const struct object_id *b)
 
 enum rename_type {
 	RENAME_NORMAL = 0,
-	RENAME_DIR,
+	RENAME_VIA_DIR,
 	RENAME_DELETE,
 	RENAME_ONE_FILE_TO_ONE,
 	RENAME_ONE_FILE_TO_TWO,

@@ -1224,11 +1224,17 @@ static int merge_file_one(struct merge_options *o,
 	return merge_file_1(o, &one, &a, &b, path, branch1, branch2, mfi);
 }
 
-static int conflict_rename_dir(struct merge_options *o,
-			       struct diff_filepair *pair,
-			       const char *rename_branch,
-			       const char *other_branch)
+static int conflict_rename_via_dir(struct merge_options *o,
+				   struct diff_filepair *pair,
+				   const char *rename_branch,
+				   const char *other_branch)
 {
+	/*
+	 * Handle file adds that need to be renamed due to directory rename
+	 * detection.  This differs from handle_rename_normal, because
+	 * there is no content merge to do; just move the path into the
+	 * desired final location.
+	 */
 	const struct diff_filespec *dest = pair->two;
 
 	if (!o->call_depth && would_lose_untracked(dest->path)) {

@@ -2498,7 +2504,7 @@ static int process_renames(struct merge_options *o,
 
 			if (oid_eq(&src_other.oid, &null_oid) &&
 			    ren1->add_turned_into_rename) {
-				setup_rename_conflict_info(RENAME_DIR,
+				setup_rename_conflict_info(RENAME_VIA_DIR,
 							   ren1->pair,
 							   NULL,
 							   branch1,

@@ -2944,12 +2950,12 @@ static int process_entry(struct merge_options *o,
 							     b_oid, b_mode,
 							     conflict_info);
 			break;
-		case RENAME_DIR:
+		case RENAME_VIA_DIR:
 			clean_merge = 1;
-			if (conflict_rename_dir(o,
-						conflict_info->pair1,
-						conflict_info->branch1,
-						conflict_info->branch2))
+			if (conflict_rename_via_dir(o,
+						    conflict_info->pair1,
+						    conflict_info->branch1,
+						    conflict_info->branch2))
 				clean_merge = -1;
 			break;
 		case RENAME_DELETE:

Signature · 1 line

2.17.0.847.g20b8963732

Johannes Schindelin

2018-05-21 14:28 UTC #6 lore raw

Re: [PATCH 3/5] merge-recursive: clarify the rename_dir/RENAME_DIR meaning

Hi Elijah,

Quoted 10 lines · Elijah Newren

We had an enum of rename types which included RENAME_DIR; this name felt misleading since it was not about an entire directory but was a status for each individual file add that occurred within a renamed directory. Since this type is for signifying that the files in question were being renamed due to directory rename detection, rename this enum value to RENAME_VIA_DIR. Make a similar change to the conflict_rename_dir() function, and add a comment to the top of that function explaining its purpose (it may not be quite as obvious as for the other conflict_rename_*() functions).

Quoted 2 lines

Signed-off-by Elijah Newren <[email protected]>
---

Make sense. I think the reading flow could be improved a little bit by splitting this paragraph into three.

Quoted 7 lines

merge-recursive.c +0 -3

index 01306c87eb..d30085d9c7 100644
-static int conflict_rename_dir(struct merge_options *o,
-			       struct diff_filepair *pair,
-			       const char *rename_branch,

merge-recursive.c static int merge_file_one(struct merge_options *o, · line 1235

Quoted 10 lines

merge-recursive.c +8 -1

-			       const char *other_branch)
+static int conflict_rename_via_dir(struct merge_options *o,
+				   struct diff_filepair *pair,
+				   const char *rename_branch,
+				   const char *other_branch)
 {
+	/*
+	 * Handle file adds that need to be renamed due to directory rename
+	 * detection.  This differs from handle_rename_normal, because
+	 * there is no content merge to do; just move the path into the

Technically, we do not move the path, but the file.

The rest of the diff looks obviously correct to me.

Thanks,
Dscho

Elijah Newren

author

2018-05-19 02:07 UTC #7 lore raw

PATCH 4/5 merge-recursive: rename conflict_rename_*() family of functions

In reply to Elijah Newren

These functions were added because processing of these conflicts needed to be deferred until process_entry() in order to get D/F conflicts and such right. The number of these has grown over time, and now include some whose name is misleading:
* conflict_rename_normal() is for handling normal file renames; a typical rename may need content merging, but we expect conflicts from that to be more the exception than the rule.
* conflict_rename_via_dir() will not be a conflict; it was just an add that turned into a move due to directory rename detection. (If there was a file in the way of the move, that would have been detected and reported earlier.)
* conflict_rename_rename_2to1 and conflict_rename_add (the latter of which doesn't exist yet but has been submitted before and I intend to resend) technically might not be conflicts if the colliding paths happen to match exactly.
Rename this family of functions to handle_rename_*().

Also rename handle_renames() to detect_and_process_renames() both to make it clearer what it does, and to differentiate it as a pre-processing step from all the handle_rename_*() functions which are called from process_entry().

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 86 +++++++++++++++++++++++------------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

merge-recursive.c +43 ~~-43~~

index d30085d9c7..273ee79afa 100644
@@ -1224,10 +1224,10 @@ static int merge_file_one(struct merge_options *o,
 	return merge_file_1(o, &one, &a, &b, path, branch1, branch2, mfi);
 }
 
-static int conflict_rename_via_dir(struct merge_options *o,
-				   struct diff_filepair *pair,
-				   const char *rename_branch,
-				   const char *other_branch)
+static int handle_rename_via_dir(struct merge_options *o,
+				 struct diff_filepair *pair,
+				 const char *rename_branch,
+				 const char *other_branch)
 {
 	/*
 	 * Handle file adds that need to be renamed due to directory rename

@@ -1329,10 +1329,10 @@ static int handle_change_delete(struct merge_options *o,
 	return ret;
 }
 
-static int conflict_rename_delete(struct merge_options *o,
-				  struct diff_filepair *pair,
-				  const char *rename_branch,
-				  const char *delete_branch)
+static int handle_rename_delete(struct merge_options *o,
+				struct diff_filepair *pair,
+				const char *rename_branch,
+				const char *delete_branch)
 {
 	const struct diff_filespec *orig = pair->one;
 	const struct diff_filespec *dest = pair->two;

@@ -1434,8 +1434,8 @@ static int handle_file(struct merge_options *o,
 	return ret;
 }
 
-static int conflict_rename_rename_1to2(struct merge_options *o,
-				       struct rename_conflict_info *ci)
+static int handle_rename_rename_1to2(struct merge_options *o,
+				     struct rename_conflict_info *ci)
 {
 	/* One file was renamed in both branches, but to different names. */
 	struct diff_filespec *one = ci->pair1->one;

@@ -1496,8 +1496,8 @@ static int conflict_rename_rename_1to2(struct merge_options *o,
 	return 0;
 }
 
-static int conflict_rename_rename_2to1(struct merge_options *o,
-				       struct rename_conflict_info *ci)
+static int handle_rename_rename_2to1(struct merge_options *o,
+				     struct rename_conflict_info *ci)
 {
 	/* Two files, a & b, were renamed to the same thing, c. */
 	struct diff_filespec *a = ci->pair1->one;

@@ -2231,7 +2231,7 @@ static void apply_directory_rename_modifications(struct merge_options *o,
 	 * "NOTE" in update_stages(), doing so will modify the current
 	 * in-memory index which will break calls to would_lose_untracked()
 	 * that we need to make.  Instead, we need to just make sure that
-	 * the various conflict_rename_*() functions update the index
+	 * the various handle_rename_*() functions update the index
 	 * explicitly rather than relying on unpack_trees() to have done it.
 	 */
 	get_tree_entry(&tree->object.oid,

@@ -2635,12 +2635,12 @@ static void initial_cleanup_rename(struct diff_queue_struct *pairs,
 	free(pairs);
 }
 
-static int handle_renames(struct merge_options *o,
-			  struct tree *common,
-			  struct tree *head,
-			  struct tree *merge,
-			  struct string_list *entries,
-			  struct rename_info *ri)
+static int detect_and_process_renames(struct merge_options *o,
+				      struct tree *common,
+				      struct tree *head,
+				      struct tree *merge,
+				      struct string_list *entries,
+				      struct rename_info *ri)
 {
 	struct diff_queue_struct *head_pairs, *merge_pairs;
 	struct hashmap *dir_re_head, *dir_re_merge;

@@ -2911,12 +2911,12 @@ static int merge_content(struct merge_options *o,
 	return !is_dirty && mfi.clean;
 }
 
-static int conflict_rename_normal(struct merge_options *o,
-				  const char *path,
-				  struct object_id *o_oid, unsigned int o_mode,
-				  struct object_id *a_oid, unsigned int a_mode,
-				  struct object_id *b_oid, unsigned int b_mode,
-				  struct rename_conflict_info *ci)
+static int handle_rename_normal(struct merge_options *o,
+				const char *path,
+				struct object_id *o_oid, unsigned int o_mode,
+				struct object_id *a_oid, unsigned int a_mode,
+				struct object_id *b_oid, unsigned int b_mode,
+				struct rename_conflict_info *ci)
 {
 	/* Merge the content and write it out */
 	return merge_content(o, path, was_dirty(o, path),

@@ -2943,37 +2943,37 @@ static int process_entry(struct merge_options *o,
 		switch (conflict_info->rename_type) {
 		case RENAME_NORMAL:
 		case RENAME_ONE_FILE_TO_ONE:
-			clean_merge = conflict_rename_normal(o,
-							     path,
-							     o_oid, o_mode,
-							     a_oid, a_mode,
-							     b_oid, b_mode,
-							     conflict_info);
+			clean_merge = handle_rename_normal(o,
+							   path,
+							   o_oid, o_mode,
+							   a_oid, a_mode,
+							   b_oid, b_mode,
+							   conflict_info);
 			break;
 		case RENAME_VIA_DIR:
 			clean_merge = 1;
-			if (conflict_rename_via_dir(o,
-						    conflict_info->pair1,
-						    conflict_info->branch1,
-						    conflict_info->branch2))
+			if (handle_rename_via_dir(o,
+						  conflict_info->pair1,
+						  conflict_info->branch1,
+						  conflict_info->branch2))
 				clean_merge = -1;
 			break;
 		case RENAME_DELETE:
 			clean_merge = 0;
-			if (conflict_rename_delete(o,
-						   conflict_info->pair1,
-						   conflict_info->branch1,
-						   conflict_info->branch2))
+			if (handle_rename_delete(o,
+						 conflict_info->pair1,
+						 conflict_info->branch1,
+						 conflict_info->branch2))
 				clean_merge = -1;
 			break;
 		case RENAME_ONE_FILE_TO_TWO:
 			clean_merge = 0;
-			if (conflict_rename_rename_1to2(o, conflict_info))
+			if (handle_rename_rename_1to2(o, conflict_info))
 				clean_merge = -1;
 			break;
 		case RENAME_TWO_FILES_TO_ONE:
 			clean_merge = 0;
-			if (conflict_rename_rename_2to1(o, conflict_info))
+			if (handle_rename_rename_2to1(o, conflict_info))
 				clean_merge = -1;
 			break;
 		default:

@@ -3112,8 +3112,8 @@ int merge_trees(struct merge_options *o,
 		get_files_dirs(o, merge);
 
 		entries = get_unmerged();
-		clean = handle_renames(o, common, head, merge, entries,
-				       &re_info);
+		clean = detect_and_process_renames(o, common, head, merge,
+						   entries, &re_info);
 		record_df_conflict_files(o, entries);
 		if (clean < 0)
 			goto cleanup;

Signature · 1 line

2.17.0.847.g20b8963732

Johannes Schindelin

2018-05-21 14:30 UTC #8 lore raw

Re: [PATCH 4/5] merge-recursive: rename conflict_rename_*() family of functions

Hi Elijah,

Quoted 17 lines · Elijah Newren

These functions were added because processing of these conflicts needed to be deferred until process_entry() in order to get D/F conflicts and such right. The number of these has grown over time, and now include some whose name is misleading:
* conflict_rename_normal() is for handling normal file renames; a typical rename may need content merging, but we expect conflicts from that to be more the exception than the rule.
* conflict_rename_via_dir() will not be a conflict; it was just an add that turned into a move due to directory rename detection. (If there was a file in the way of the move, that would have been detected and reported earlier.)
* conflict_rename_rename_2to1 and conflict_rename_add (the latter of which doesn't exist yet but has been submitted before and I intend to resend) technically might not be conflicts if the colliding paths happen to match exactly.
Rename this family of functions to handle_rename_*().

Quoted 6 lines

Also rename handle_renames() to detect_and_process_renames() both to make it clearer what it does, and to differentiate it as a pre-processing step from all the handle_rename_*() functions which are called from process_entry().
Signed-off-by Elijah Newren <[email protected]>

Makes sense, and the patch looks obviously correct to me.

Thanks,
Dscho

Elijah Newren

author

2018-05-19 02:07 UTC #9 lore raw

PATCH 5/5 merge-recursive: simplify handle_change_delete

In reply to Elijah Newren

There is really no need for four branches of nearly identical messages when we can store the differences into small variables before printing. It does require a few allocations this way, but makes the code much easier to parse for human readers.

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

merge-recursive.c +11 ~~-25~~

index 273ee79afa..3bd727995b 100644
@@ -1290,31 +1290,17 @@ static int handle_change_delete(struct merge_options *o,
 		if (!ret)
 			ret = update_file(o, 0, o_oid, o_mode, update_path);
 	} else {
-		if (!alt_path) {
-			if (!old_path) {
-				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
-				       "and %s in %s. Version %s of %s left in tree."),
-				       change, path, delete_branch, change_past,
-				       change_branch, change_branch, path);
-			} else {
-				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
-				       "and %s to %s in %s. Version %s of %s left in tree."),
-				       change, old_path, delete_branch, change_past, path,
-				       change_branch, change_branch, path);
-			}
-		} else {
-			if (!old_path) {
-				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
-				       "and %s in %s. Version %s of %s left in tree at %s."),
-				       change, path, delete_branch, change_past,
-				       change_branch, change_branch, path, alt_path);
-			} else {
-				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
-				       "and %s to %s in %s. Version %s of %s left in tree at %s."),
-				       change, old_path, delete_branch, change_past, path,
-				       change_branch, change_branch, path, alt_path);
-			}
-		}
+		const char *deleted_path = old_path ? old_path : path;
+		char *supp1 = xstrfmt(old_path ? " to %s" : "", path);
+		char *supp2 = xstrfmt(alt_path ? " at %s" : "", alt_path);
+
+		output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
+		       "and %s%s in %s. Version %s of %s left in tree%s."),
+		       change, deleted_path, delete_branch, change_past,
+		       supp1, change_branch, change_branch, path, supp2);
+		free(supp1);
+		free(supp2);
+
 		/*
 		 * No need to call update_file() on path when change_branch ==
 		 * o->branch1 && !alt_path, since that would needlessly touch

Signature · 1 line

2.17.0.847.g20b8963732

Johannes Sixt

2018-05-19 07:32 UTC #10 lore raw

Re: [PATCH 5/5] merge-recursive: simplify handle_change_delete

Am 19.05.2018 um 04:07 schrieb Elijah Newren:

Quoted 2 lines

There is really no need for four branches of nearly identical messages when we can store the differences into small variables before printing.

Oh, there is a reason for the repeated message text: translations! Please do not play sentence Lego with translated strings. The original code is preferable.

Quoted 56 lines

It does require a few allocations this way, but makes the code much easier to parse for human readers.

Signed-off-by Elijah Newren <[email protected]>

---
  merge-recursive.c | 36 +++++++++++-------------------------
  1 file changed, 11 insertions(+), 25 deletions(-)

merge-recursive.c +11 ~~-25~~

index 273ee79afa..3bd727995b 100644
@@ -1290,31 +1290,17 @@ static int handle_change_delete(struct merge_options *o,
  		if (!ret)
  			ret = update_file(o, 0, o_oid, o_mode, update_path);
  	} else {
-		if (!alt_path) {
-			if (!old_path) {
-				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
-				       "and %s in %s. Version %s of %s left in tree."),
-				       change, path, delete_branch, change_past,
-				       change_branch, change_branch, path);
-			} else {
-				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
-				       "and %s to %s in %s. Version %s of %s left in tree."),
-				       change, old_path, delete_branch, change_past, path,
-				       change_branch, change_branch, path);
-			}
-		} else {
-			if (!old_path) {
-				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
-				       "and %s in %s. Version %s of %s left in tree at %s."),
-				       change, path, delete_branch, change_past,
-				       change_branch, change_branch, path, alt_path);
-			} else {
-				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
-				       "and %s to %s in %s. Version %s of %s left in tree at %s."),
-				       change, old_path, delete_branch, change_past, path,
-				       change_branch, change_branch, path, alt_path);
-			}
-		}
+		const char *deleted_path = old_path ? old_path : path;
+		char *supp1 = xstrfmt(old_path ? " to %s" : "", path);
+		char *supp2 = xstrfmt(alt_path ? " at %s" : "", alt_path);
+
+		output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
+		       "and %s%s in %s. Version %s of %s left in tree%s."),
+		       change, deleted_path, delete_branch, change_past,
+		       supp1, change_branch, change_branch, path, supp2);
+		free(supp1);
+		free(supp2);
+
  		/*
  		 * No need to call update_file() on path when change_branch ==
  		 * o->branch1 && !alt_path, since that would needlessly touch

Elijah Newren

author

2018-05-19 15:39 UTC #11 lore raw

Re: [PATCH 5/5] merge-recursive: simplify handle_change_delete

Quoted 8 lines · Johannes Sixt

Am 19.05.2018 um 04:07 schrieb Elijah Newren:
Quoted 3 lines
There is really no need for four branches of nearly identical messages when we can store the differences into small variables before printing.
Oh, there is a reason for the repeated message text: translations! Please do not play sentence Lego with translated strings. The original code is preferable.

Ah, translations; that makes sense now. I'm still annoyed by the code, but I retract patch 5 then.

Johannes Schindelin

2018-05-21 13:41 UTC #12 lore raw

Re: [PATCH 5/5] merge-recursive: simplify handle_change_delete

Hi Elijah,

Quoted 6 lines · Elijah Newren

Quoted 5 lines · Johannes Sixt
Am 19.05.2018 um 04:07 schrieb Elijah Newren:
Quoted 3 lines
There is really no need for four branches of nearly identical messages when we can store the differences into small variables before printing.

Quoted 6 lines

Quoted 3 lines
Oh, there is a reason for the repeated message text: translations! Please do not play sentence Lego with translated strings. The original code is preferable.
Ah, translations; that makes sense now. I'm still annoyed by the code, but I retract patch 5 then.

Maybe you can remove the temptation for others, too, my replacing your patch 5 with one that adds the code comment "No sentence Lego! Think of our poor translators and refrain from making their life miserable!" or some more appropriate one?

Ciao,
Dscho

Elijah Newren

author

2018-05-21 17:22 UTC #13 lore raw

Re: [PATCH 5/5] merge-recursive: simplify handle_change_delete

Hi Dscho,

Quoted 8 lines · Johannes Schindelin

Quoted 7 lines · Elijah Newren
Quoted 4 lines · Johannes Sixt
Oh, there is a reason for the repeated message text: translations! Please do not play sentence Lego with translated strings. The original code is preferable.

Quoted 7 lines

Quoted 2 lines
Ah, translations; that makes sense now. I'm still annoyed by the code, but I retract patch 5 then.
Maybe you can remove the temptation for others, too, my replacing your patch 5 with one that adds the code comment "No sentence Lego! Think of our poor translators and refrain from making their life miserable!" or some more appropriate one?

That sounds reasonable. I'll make that change (plus your other suggested changes on other patches; thanks for all the reviews) in the next round.

I'm also considering adopting a setup_unpack_trees_porcelain() style of handling (from unpack-trees.c) for all these messages in merge-recursive.c, but that's a bigger change that I'd probably put in a separate series if I decide to go with it.

Elijah

Elijah Newren

author

2018-05-22 00:43 UTC #14 lore raw

v2 cover merge-recursive code cleanups

In reply to Elijah Newren

The series was made on top of next (specifically commit 4fb28f7b1639 ("Merge branch 'sb/object-store-replace' into next")); it will not apply to master.

Changes since v1:
* As suggested by Dscho, replaced patch 5 with a pointer about the fact that the code is that way to help translators
* Small rewordings or paragraph splittings suggested by Dscho
* Added Dscho's Acked-by on relevant patches (2-4)

 merge-recursive.c | 152 ++++++++++++++++++++++++++--------------------
 1 file changed, 87 insertions(+), 65 deletions(-)

branch-diff:
1: 1af1c7df17 = 1: 96225eddbb merge-recursive: fix miscellaneous grammar error in comment 2: 8ed7c8b588 ! 2: c63f72e96d merge-recursive: fix numerous argument alignment issues
@@ -5,6 +5,7 @@
Various refactorings throughout the code have left lots of alignment issues that were driving me crazy; fix them.

+ Acked-by: Johannes Schindelin <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

diff --git a/merge-recursive.c b/merge-recursive.c
3: e96b2cd9ae ! 3: 3d95e8b756 merge-recursive: clarify the rename_dir/RENAME_DIR meaning
@@ -4,14 +4,17 @@

We had an enum of rename types which included RENAME_DIR; this name felt misleading since it was not about an entire directory but was a status for
- each individual file add that occurred within a renamed directory. Since
- this type is for signifying that the files in question were being renamed
- due to directory rename detection, rename this enum value to
- RENAME_VIA_DIR. Make a similar change to the conflict_rename_dir()
- function, and add a comment to the top of that function explaining its
- purpose (it may not be quite as obvious as for the other
- conflict_rename_*() functions).
+ each individual file add that occurred within a renamed directory.

+ Since this type is for signifying that the files in question were being
+ renamed due to directory rename detection, rename this enum value to
+ RENAME_VIA_DIR.
+
+ Make a similar change to the conflict_rename_dir() function, and add a
+ comment to the top of that function explaining its purpose (it may not be
+ quite as obvious as for the other conflict_rename_*() functions).
+
+ Acked-by: Johannes Schindelin <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

diff --git a/merge-recursive.c b/merge-recursive.c
@@ -42,7 +45,7 @@
+ /*
+ * Handle file adds that need to be renamed due to directory rename
+ * detection. This differs from handle_rename_normal, because
-+ * there is no content merge to do; just move the path into the
++ * there is no content merge to do; just move the file into the
+ * desired final location.
+ */
const struct diff_filespec *dest = pair->two;
4: 4155613a6b ! 4: dfd6ab22db merge-recursive: rename conflict_rename_*() family of functions
@@ -24,6 +24,7 @@
from all the handle_rename_*() functions which are called from process_entry().

+ Acked-by: Johannes Schindelin <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

diff --git a/merge-recursive.c b/merge-recursive.c
5: 20b8963732 < --: ---------- merge-recursive: simplify handle_change_delete
--: ---------- > 5: 122629ef7a merge-recursive: add pointer about unduly complex looking code

Signature · 1 line

2.17.0.847.g20b8963732

Elijah Newren

author

2018-05-22 00:43 UTC #15 lore raw

PATCH v2 1/5 merge-recursive: fix miscellaneous grammar error in comment

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

merge-recursive.c +1 -1

index 35df695fa4..c430fd72bc 100644
@@ -531,7 +531,7 @@ static void record_df_conflict_files(struct merge_options *o,
 				     struct string_list *entries)
 {
 	/* If there is a D/F conflict and the file for such a conflict
-	 * currently exist in the working tree, we want to allow it to be
+	 * currently exists in the working tree, we want to allow it to be
 	 * removed to make room for the corresponding directory if needed.
 	 * The files underneath the directories of such D/F conflicts will
 	 * be processed before the corresponding file involved in the D/F

Signature · 1 line

2.17.0.847.g20b8963732

Elijah Newren

author

2018-05-22 00:43 UTC #16 lore raw

PATCH v2 2/5 merge-recursive: fix numerous argument alignment issues

In reply to Elijah Newren

Various refactorings throughout the code have left lots of alignment issues that were driving me crazy; fix them.

Acked-by Johannes Schindelin <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

merge-recursive.c +24 ~~-23~~

index c430fd72bc..01306c87eb 100644
@@ -308,8 +308,8 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)
 }
 
 static int add_cacheinfo(struct merge_options *o,
-		unsigned int mode, const struct object_id *oid,
-		const char *path, int stage, int refresh, int options)
+			 unsigned int mode, const struct object_id *oid,
+			 const char *path, int stage, int refresh, int options)
 {
 	struct cache_entry *ce;
 	int ret;

@@ -409,8 +409,8 @@ struct tree *write_tree_from_memory(struct merge_options *o)
 }
 
 static int save_files_dirs(const struct object_id *oid,
-		struct strbuf *base, const char *path,
-		unsigned int mode, int stage, void *context)
+			   struct strbuf *base, const char *path,
+			   unsigned int mode, int stage, void *context)
 {
 	struct path_hashmap_entry *entry;
 	int baselen = base->len;

@@ -1257,13 +1257,13 @@ static int conflict_rename_dir(struct merge_options *o,
 }
 
 static int handle_change_delete(struct merge_options *o,
-				 const char *path, const char *old_path,
-				 const struct object_id *o_oid, int o_mode,
-				 const struct object_id *changed_oid,
-				 int changed_mode,
-				 const char *change_branch,
-				 const char *delete_branch,
-				 const char *change, const char *change_past)
+				const char *path, const char *old_path,
+				const struct object_id *o_oid, int o_mode,
+				const struct object_id *changed_oid,
+				int changed_mode,
+				const char *change_branch,
+				const char *delete_branch,
+				const char *change, const char *change_past)
 {
 	char *alt_path = NULL;
 	const char *update_path = path;

@@ -1324,9 +1324,9 @@ static int handle_change_delete(struct merge_options *o,
 }
 
 static int conflict_rename_delete(struct merge_options *o,
-				   struct diff_filepair *pair,
-				   const char *rename_branch,
-				   const char *delete_branch)
+				  struct diff_filepair *pair,
+				  const char *rename_branch,
+				  const char *delete_branch)
 {
 	const struct diff_filespec *orig = pair->one;
 	const struct diff_filespec *dest = pair->two;

@@ -1429,7 +1429,7 @@ static int handle_file(struct merge_options *o,
 }
 
 static int conflict_rename_rename_1to2(struct merge_options *o,
-					struct rename_conflict_info *ci)
+				       struct rename_conflict_info *ci)
 {
 	/* One file was renamed in both branches, but to different names. */
 	struct diff_filespec *one = ci->pair1->one;

@@ -1491,7 +1491,7 @@ static int conflict_rename_rename_1to2(struct merge_options *o,
 }
 
 static int conflict_rename_rename_2to1(struct merge_options *o,
-					struct rename_conflict_info *ci)
+				       struct rename_conflict_info *ci)
 {
 	/* Two files, a & b, were renamed to the same thing, c. */
 	struct diff_filespec *a = ci->pair1->one;

@@ -2710,7 +2710,8 @@ static struct object_id *stage_oid(const struct object_id *oid, unsigned mode)
 }
 
 static int read_oid_strbuf(struct merge_options *o,
-	const struct object_id *oid, struct strbuf *dst)
+			   const struct object_id *oid,
+			   struct strbuf *dst)
 {
 	void *buf;
 	enum object_type type;

@@ -2763,10 +2764,10 @@ static int blob_unchanged(struct merge_options *opt,
 }
 
 static int handle_modify_delete(struct merge_options *o,
-				 const char *path,
-				 struct object_id *o_oid, int o_mode,
-				 struct object_id *a_oid, int a_mode,
-				 struct object_id *b_oid, int b_mode)
+				const char *path,
+				struct object_id *o_oid, int o_mode,
+				struct object_id *a_oid, int a_mode,
+				struct object_id *b_oid, int b_mode)
 {
 	const char *modify_branch, *delete_branch;
 	struct object_id *changed_oid;

@@ -3303,14 +3304,14 @@ int merge_recursive_generic(struct merge_options *o,
 			struct commit *base;
 			if (!(base = get_ref(base_list[i], oid_to_hex(base_list[i]))))
 				return err(o, _("Could not parse object '%s'"),
-					oid_to_hex(base_list[i]));
+					   oid_to_hex(base_list[i]));
 			commit_list_insert(base, &ca);
 		}
 	}
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	clean = merge_recursive(o, head_commit, next_commit, ca,
-			result);
+				result);
 	if (clean < 0) {
 		rollback_lock_file(&lock);
 		return clean;

Signature · 1 line

2.17.0.847.g20b8963732

Elijah Newren

author

2018-05-22 00:43 UTC #17 lore raw

PATCH v2 3/5 merge-recursive: clarify the rename_dir/RENAME_DIR meaning

In reply to Elijah Newren

Since this type is for signifying that the files in question were being renamed due to directory rename detection, rename this enum value to RENAME_VIA_DIR.

Make a similar change to the conflict_rename_dir() function, and add a comment to the top of that function explaining its purpose (it may not be quite as obvious as for the other conflict_rename_*() functions).

Acked-by Johannes Schindelin <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

merge-recursive.c +17 ~~-11~~

index 01306c87eb..e2b0db0645 100644
@@ -180,7 +180,7 @@ static int oid_eq(const struct object_id *a, const struct object_id *b)
 
 enum rename_type {
 	RENAME_NORMAL = 0,
-	RENAME_DIR,
+	RENAME_VIA_DIR,
 	RENAME_DELETE,
 	RENAME_ONE_FILE_TO_ONE,
 	RENAME_ONE_FILE_TO_TWO,

@@ -1224,11 +1224,17 @@ static int merge_file_one(struct merge_options *o,
 	return merge_file_1(o, &one, &a, &b, path, branch1, branch2, mfi);
 }
 
-static int conflict_rename_dir(struct merge_options *o,
-			       struct diff_filepair *pair,
-			       const char *rename_branch,
-			       const char *other_branch)
+static int conflict_rename_via_dir(struct merge_options *o,
+				   struct diff_filepair *pair,
+				   const char *rename_branch,
+				   const char *other_branch)
 {
+	/*
+	 * Handle file adds that need to be renamed due to directory rename
+	 * detection.  This differs from handle_rename_normal, because
+	 * there is no content merge to do; just move the file into the
+	 * desired final location.
+	 */
 	const struct diff_filespec *dest = pair->two;
 
 	if (!o->call_depth && would_lose_untracked(dest->path)) {

@@ -2498,7 +2504,7 @@ static int process_renames(struct merge_options *o,
 
 			if (oid_eq(&src_other.oid, &null_oid) &&
 			    ren1->add_turned_into_rename) {
-				setup_rename_conflict_info(RENAME_DIR,
+				setup_rename_conflict_info(RENAME_VIA_DIR,
 							   ren1->pair,
 							   NULL,
 							   branch1,

@@ -2944,12 +2950,12 @@ static int process_entry(struct merge_options *o,
 							     b_oid, b_mode,
 							     conflict_info);
 			break;
-		case RENAME_DIR:
+		case RENAME_VIA_DIR:
 			clean_merge = 1;
-			if (conflict_rename_dir(o,
-						conflict_info->pair1,
-						conflict_info->branch1,
-						conflict_info->branch2))
+			if (conflict_rename_via_dir(o,
+						    conflict_info->pair1,
+						    conflict_info->branch1,
+						    conflict_info->branch2))
 				clean_merge = -1;
 			break;
 		case RENAME_DELETE:

Signature · 1 line

2.17.0.847.g20b8963732

Elijah Newren

author

2018-05-22 00:43 UTC #18 lore raw

PATCH v2 4/5 merge-recursive: rename conflict_rename_*() family of functions

In reply to Elijah Newren

Acked-by Johannes Schindelin <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 86 +++++++++++++++++++++++------------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

merge-recursive.c +43 ~~-43~~

index e2b0db0645..270a4d2aad 100644
@@ -1224,10 +1224,10 @@ static int merge_file_one(struct merge_options *o,
 	return merge_file_1(o, &one, &a, &b, path, branch1, branch2, mfi);
 }
 
-static int conflict_rename_via_dir(struct merge_options *o,
-				   struct diff_filepair *pair,
-				   const char *rename_branch,
-				   const char *other_branch)
+static int handle_rename_via_dir(struct merge_options *o,
+				 struct diff_filepair *pair,
+				 const char *rename_branch,
+				 const char *other_branch)
 {
 	/*
 	 * Handle file adds that need to be renamed due to directory rename

@@ -1329,10 +1329,10 @@ static int handle_change_delete(struct merge_options *o,
 	return ret;
 }
 
-static int conflict_rename_delete(struct merge_options *o,
-				  struct diff_filepair *pair,
-				  const char *rename_branch,
-				  const char *delete_branch)
+static int handle_rename_delete(struct merge_options *o,
+				struct diff_filepair *pair,
+				const char *rename_branch,
+				const char *delete_branch)
 {
 	const struct diff_filespec *orig = pair->one;
 	const struct diff_filespec *dest = pair->two;

@@ -1434,8 +1434,8 @@ static int handle_file(struct merge_options *o,
 	return ret;
 }
 
-static int conflict_rename_rename_1to2(struct merge_options *o,
-				       struct rename_conflict_info *ci)
+static int handle_rename_rename_1to2(struct merge_options *o,
+				     struct rename_conflict_info *ci)
 {
 	/* One file was renamed in both branches, but to different names. */
 	struct diff_filespec *one = ci->pair1->one;

@@ -1496,8 +1496,8 @@ static int conflict_rename_rename_1to2(struct merge_options *o,
 	return 0;
 }
 
-static int conflict_rename_rename_2to1(struct merge_options *o,
-				       struct rename_conflict_info *ci)
+static int handle_rename_rename_2to1(struct merge_options *o,
+				     struct rename_conflict_info *ci)
 {
 	/* Two files, a & b, were renamed to the same thing, c. */
 	struct diff_filespec *a = ci->pair1->one;

@@ -2231,7 +2231,7 @@ static void apply_directory_rename_modifications(struct merge_options *o,
 	 * "NOTE" in update_stages(), doing so will modify the current
 	 * in-memory index which will break calls to would_lose_untracked()
 	 * that we need to make.  Instead, we need to just make sure that
-	 * the various conflict_rename_*() functions update the index
+	 * the various handle_rename_*() functions update the index
 	 * explicitly rather than relying on unpack_trees() to have done it.
 	 */
 	get_tree_entry(&tree->object.oid,

@@ -2635,12 +2635,12 @@ static void initial_cleanup_rename(struct diff_queue_struct *pairs,
 	free(pairs);
 }
 
-static int handle_renames(struct merge_options *o,
-			  struct tree *common,
-			  struct tree *head,
-			  struct tree *merge,
-			  struct string_list *entries,
-			  struct rename_info *ri)
+static int detect_and_process_renames(struct merge_options *o,
+				      struct tree *common,
+				      struct tree *head,
+				      struct tree *merge,
+				      struct string_list *entries,
+				      struct rename_info *ri)
 {
 	struct diff_queue_struct *head_pairs, *merge_pairs;
 	struct hashmap *dir_re_head, *dir_re_merge;

@@ -2911,12 +2911,12 @@ static int merge_content(struct merge_options *o,
 	return !is_dirty && mfi.clean;
 }
 
-static int conflict_rename_normal(struct merge_options *o,
-				  const char *path,
-				  struct object_id *o_oid, unsigned int o_mode,
-				  struct object_id *a_oid, unsigned int a_mode,
-				  struct object_id *b_oid, unsigned int b_mode,
-				  struct rename_conflict_info *ci)
+static int handle_rename_normal(struct merge_options *o,
+				const char *path,
+				struct object_id *o_oid, unsigned int o_mode,
+				struct object_id *a_oid, unsigned int a_mode,
+				struct object_id *b_oid, unsigned int b_mode,
+				struct rename_conflict_info *ci)
 {
 	/* Merge the content and write it out */
 	return merge_content(o, path, was_dirty(o, path),

@@ -2943,37 +2943,37 @@ static int process_entry(struct merge_options *o,
 		switch (conflict_info->rename_type) {
 		case RENAME_NORMAL:
 		case RENAME_ONE_FILE_TO_ONE:
-			clean_merge = conflict_rename_normal(o,
-							     path,
-							     o_oid, o_mode,
-							     a_oid, a_mode,
-							     b_oid, b_mode,
-							     conflict_info);
+			clean_merge = handle_rename_normal(o,
+							   path,
+							   o_oid, o_mode,
+							   a_oid, a_mode,
+							   b_oid, b_mode,
+							   conflict_info);
 			break;
 		case RENAME_VIA_DIR:
 			clean_merge = 1;
-			if (conflict_rename_via_dir(o,
-						    conflict_info->pair1,
-						    conflict_info->branch1,
-						    conflict_info->branch2))
+			if (handle_rename_via_dir(o,
+						  conflict_info->pair1,
+						  conflict_info->branch1,
+						  conflict_info->branch2))
 				clean_merge = -1;
 			break;
 		case RENAME_DELETE:
 			clean_merge = 0;
-			if (conflict_rename_delete(o,
-						   conflict_info->pair1,
-						   conflict_info->branch1,
-						   conflict_info->branch2))
+			if (handle_rename_delete(o,
+						 conflict_info->pair1,
+						 conflict_info->branch1,
+						 conflict_info->branch2))
 				clean_merge = -1;
 			break;
 		case RENAME_ONE_FILE_TO_TWO:
 			clean_merge = 0;
-			if (conflict_rename_rename_1to2(o, conflict_info))
+			if (handle_rename_rename_1to2(o, conflict_info))
 				clean_merge = -1;
 			break;
 		case RENAME_TWO_FILES_TO_ONE:
 			clean_merge = 0;
-			if (conflict_rename_rename_2to1(o, conflict_info))
+			if (handle_rename_rename_2to1(o, conflict_info))
 				clean_merge = -1;
 			break;
 		default:

@@ -3112,8 +3112,8 @@ int merge_trees(struct merge_options *o,
 		get_files_dirs(o, merge);
 
 		entries = get_unmerged();
-		clean = handle_renames(o, common, head, merge, entries,
-				       &re_info);
+		clean = detect_and_process_renames(o, common, head, merge,
+						   entries, &re_info);
 		record_df_conflict_files(o, entries);
 		if (clean < 0)
 			goto cleanup;

Signature · 1 line

2.17.0.847.g20b8963732

Elijah Newren

author

2018-05-22 00:43 UTC #19 lore raw

PATCH v2 5/5 merge-recursive: add pointer about unduly complex looking code

In reply to Elijah Newren

handle_change_delete() has a block of code displaying one of four nearly identical messages. Each contains about half a dozen variable interpolations, which use nearly identical variables as well. Someone trying to parse this may be slowed down trying to parse the differences and why they are here; help them out by adding a comment explaining the differences.

Further, point out that this code structure isn't collapsed into something more concise and readable for the programmer, because we want to keep full messages intact in order to make translators' jobs much easier.

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

merge-recursive.c +15 -0

index 270a4d2aad..28f39b3e9f 100644
@@ -1290,6 +1290,21 @@ static int handle_change_delete(struct merge_options *o,
 		if (!ret)
 			ret = update_file(o, 0, o_oid, o_mode, update_path);
 	} else {
+		/*
+		 * Despite the four nearly duplicate messages and argument
+		 * lists below and the ugliness of the nested if-statements,
+		 * having complete messages makes the job easier for
+		 * translators.
+		 *
+		 * The slight variance among the cases is due to the fact
+		 * that:
+		 *   1) directory/file conflicts (in effect if
+		 *      !alt_path) could cause us to need to write the
+		 *      file to a different path.
+		 *   2) renames (in effect if !old_path) could mean that
+		 *      there are two names for the path that the user
+		 *      may know the file by.
+		 */
 		if (!alt_path) {
 			if (!old_path) {
 				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "

Signature · 1 line

2.17.0.847.g20b8963732

Elijah Newren

author

2018-06-10 04:16 UTC #20 lore raw

v3 cover merge-recursive code cleanups

In reply to Elijah Newren

This patch series contains several small code cleanups for merge-recursive.

Changes since v2:
* Now built on master (the topics this depended on have graduated); merges cleanly with next and pu.
* Patch #2 has a few additional argument alignment fixes; some in code added by topics that have since graduated, and a couple that were overlooked previously.
* Patch #3 is entirely new.
* Re-wrapped the commit message of the final patch. (range-diff below)

Elijah Newren (6):
merge-recursive: fix miscellaneous grammar error in comment
merge-recursive: fix numerous argument alignment issues
merge-recursive: align labels with their respective code blocks
merge-recursive: clarify the rename_dir/RENAME_DIR meaning
merge-recursive: rename conflict_rename_*() family of functions
merge-recursive: add pointer about unduly complex looking code

 merge-recursive.c | 186 ++++++++++++++++++++++++++--------------------
 1 file changed, 104 insertions(+), 82 deletions(-)

range-diff against v2:

1: 4222f174de = 1: 4222f174de merge-recursive: fix miscellaneous grammar error in comment 2: 4ebf93822d ! 2: 284a5fee3d merge-recursive: fix numerous argument alignment issues
@@ -33,6 +33,76 @@
{
struct path_hashmap_entry *entry;
int baselen = base->len;
+@@
+ */
+ if (would_lose_untracked(path))
+ return err(o, _("refusing to lose untracked file at '%s'"),
+- path);
++ path);
+
+ /* Successful unlink is good.. */
+ if (!unlink(path))
+@@
+ unlink(path);
+ if (symlink(lnk, path))
+ ret = err(o, _("failed to symlink '%s': %s"),
+- path, strerror(errno));
++ path, strerror(errno));
+ free(lnk);
+ } else
+ ret = err(o,
+@@
+ }
+
+ static int find_first_merges(struct object_array *result, const char *path,
+- struct commit *a, struct commit *b)
++ struct commit *a, struct commit *b)
+ {
+ int i, j;
+ struct object_array merges = OBJECT_ARRAY_INIT;
+@@
+
+ /* get all revisions that merge commit a */
+ xsnprintf(merged_revision, sizeof(merged_revision), "^%s",
+- oid_to_hex(&a->object.oid));
++ oid_to_hex(&a->object.oid));
+ init_revisions(&revs, NULL);
+ rev_opts.submodule = path;
+ /* FIXME: can't handle linked worktrees in submodules yet */
+@@
+ output(o, 2, _("Found a possible merge resolution for the submodule:\n"));
+ print_commit((struct commit *) merges.objects[0].item);
+ output(o, 2, _(
+- "If this is correct simply add it to the index "
+- "for example\n"
+- "by using:\n\n"
+- " git update-index --cacheinfo 160000 %s \"%s\"\n\n"
+- "which will accept this suggestion.\n"),
+- oid_to_hex(&merges.objects[0].item->oid), path);
++ "If this is correct simply add it to the index "
++ "for example\n"
++ "by using:\n\n"
++ " git update-index --cacheinfo 160000 %s \"%s\"\n\n"
++ "which will accept this suggestion.\n"),
++ oid_to_hex(&merges.objects[0].item->oid), path);
+ break;
+
+ default:
+@@
+ result->clean = (merge_status == 0);
+ } else if (S_ISGITLINK(a->mode)) {
+ result->clean = merge_submodule(o, &result->oid,
+- one->path,
+- &one->oid,
+- &a->oid,
+- &b->oid);
++ one->path,
++ &one->oid,
++ &a->oid,
++ &b->oid);
+ } else if (S_ISLNK(a->mode)) {
+ switch (o->recursive_variant) {
+ case MERGE_RECURSIVE_NORMAL:
@@
}

--: ---------- >  3: 6bae2a267f merge-recursive: align labels with their respective code blocks
 3: 585759f07a =  4: aecf1267d8 merge-recursive: clarify the rename_dir/RENAME_DIR meaning
 4: 3cfb8b01b8 =  5: f7637bef12 merge-recursive: rename conflict_rename_*() family of functions
 5: d2a24f5b38 !  6: ffeb3ef585 merge-recursive: add pointer about unduly complex looking code

@@ -3,10 +3,11 @@
merge-recursive: add pointer about unduly complex looking code

handle_change_delete() has a block of code displaying one of four nearly
- identical messages. Each contains about half a dozen variable interpolations,
- which use nearly identical variables as well. Someone trying to parse this
- may be slowed down trying to parse the differences and why they are here; help
- them out by adding a comment explaining the differences.
+ identical messages. Each contains about half a dozen variable
+ interpolations, which use nearly identical variables as well. Someone
+ trying to parse this may be slowed down trying to parse the differences
+ and why they are here; help them out by adding a comment explaining the
+ differences.

Further, point out that this code structure isn't collapsed into something more concise and readable for the programmer, because we want to keep full

Signature · 1 line

2.18.0.rc1.6.gffeb3ef585

Elijah Newren

author

2018-06-10 04:16 UTC #21 lore raw

PATCH v3 1/6 merge-recursive: fix miscellaneous grammar error in comment

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

merge-recursive.c +1 -1

index ac27abbd4c..921f8e2d2d 100644
@@ -539,7 +539,7 @@ static void record_df_conflict_files(struct merge_options *o,
 				     struct string_list *entries)
 {
 	/* If there is a D/F conflict and the file for such a conflict
-	 * currently exist in the working tree, we want to allow it to be
+	 * currently exists in the working tree, we want to allow it to be
 	 * removed to make room for the corresponding directory if needed.
 	 * The files underneath the directories of such D/F conflicts will
 	 * be processed before the corresponding file involved in the D/F

Signature · 1 line

2.18.0.rc1.6.gffeb3ef585

Elijah Newren

author

2018-06-10 04:16 UTC #22 lore raw

PATCH v3 2/6 merge-recursive: fix numerous argument alignment issues

In reply to Elijah Newren

Various refactorings throughout the code have left lots of alignment issues that were driving me crazy; fix them.

Acked-by Johannes Schindelin <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 75 ++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 37 deletions(-)

merge-recursive.c +38 ~~-37~~

index 921f8e2d2d..09c249b93e 100644
@@ -309,8 +309,8 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)
 }
 
 static int add_cacheinfo(struct merge_options *o,
-		unsigned int mode, const struct object_id *oid,
-		const char *path, int stage, int refresh, int options)
+			 unsigned int mode, const struct object_id *oid,
+			 const char *path, int stage, int refresh, int options)
 {
 	struct cache_entry *ce;
 	int ret;

@@ -417,8 +417,8 @@ struct tree *write_tree_from_memory(struct merge_options *o)
 }
 
 static int save_files_dirs(const struct object_id *oid,
-		struct strbuf *base, const char *path,
-		unsigned int mode, int stage, void *context)
+			   struct strbuf *base, const char *path,
+			   unsigned int mode, int stage, void *context)
 {
 	struct path_hashmap_entry *entry;
 	int baselen = base->len;

@@ -913,7 +913,7 @@ static int make_room_for_path(struct merge_options *o, const char *path)
 	 */
 	if (would_lose_untracked(path))
 		return err(o, _("refusing to lose untracked file at '%s'"),
-			     path);
+			   path);
 
 	/* Successful unlink is good.. */
 	if (!unlink(path))

@@ -992,7 +992,7 @@ static int update_file_flags(struct merge_options *o,
 			unlink(path);
 			if (symlink(lnk, path))
 				ret = err(o, _("failed to symlink '%s': %s"),
-					path, strerror(errno));
+					  path, strerror(errno));
 			free(lnk);
 		} else
 			ret = err(o,

@@ -1090,7 +1090,7 @@ static int merge_3way(struct merge_options *o,
 }
 
 static int find_first_merges(struct object_array *result, const char *path,
-		struct commit *a, struct commit *b)
+			     struct commit *a, struct commit *b)
 {
 	int i, j;
 	struct object_array merges = OBJECT_ARRAY_INIT;

@@ -1108,7 +1108,7 @@ static int find_first_merges(struct object_array *result, const char *path,
 
 	/* get all revisions that merge commit a */
 	xsnprintf(merged_revision, sizeof(merged_revision), "^%s",
-			oid_to_hex(&a->object.oid));
+		  oid_to_hex(&a->object.oid));
 	init_revisions(&revs, NULL);
 	rev_opts.submodule = path;
 	/* FIXME: can't handle linked worktrees in submodules yet */

@@ -1250,12 +1250,12 @@ static int merge_submodule(struct merge_options *o,
 		output(o, 2, _("Found a possible merge resolution for the submodule:\n"));
 		print_commit((struct commit *) merges.objects[0].item);
 		output(o, 2, _(
-			"If this is correct simply add it to the index "
-			"for example\n"
-			"by using:\n\n"
-			"  git update-index --cacheinfo 160000 %s \"%s\"\n\n"
-			"which will accept this suggestion.\n"),
-			oid_to_hex(&merges.objects[0].item->oid), path);
+		       "If this is correct simply add it to the index "
+		       "for example\n"
+		       "by using:\n\n"
+		       "  git update-index --cacheinfo 160000 %s \"%s\"\n\n"
+		       "which will accept this suggestion.\n"),
+		       oid_to_hex(&merges.objects[0].item->oid), path);
 		break;
 
 	default:

@@ -1332,10 +1332,10 @@ static int merge_file_1(struct merge_options *o,
 			result->clean = (merge_status == 0);
 		} else if (S_ISGITLINK(a->mode)) {
 			result->clean = merge_submodule(o, &result->oid,
-						       one->path,
-						       &one->oid,
-						       &a->oid,
-						       &b->oid);
+							one->path,
+							&one->oid,
+							&a->oid,
+							&b->oid);
 		} else if (S_ISLNK(a->mode)) {
 			switch (o->recursive_variant) {
 			case MERGE_RECURSIVE_NORMAL:

@@ -1443,13 +1443,13 @@ static int conflict_rename_dir(struct merge_options *o,
 }
 
 static int handle_change_delete(struct merge_options *o,
-				 const char *path, const char *old_path,
-				 const struct object_id *o_oid, int o_mode,
-				 const struct object_id *changed_oid,
-				 int changed_mode,
-				 const char *change_branch,
-				 const char *delete_branch,
-				 const char *change, const char *change_past)
+				const char *path, const char *old_path,
+				const struct object_id *o_oid, int o_mode,
+				const struct object_id *changed_oid,
+				int changed_mode,
+				const char *change_branch,
+				const char *delete_branch,
+				const char *change, const char *change_past)
 {
 	char *alt_path = NULL;
 	const char *update_path = path;

@@ -1510,9 +1510,9 @@ static int handle_change_delete(struct merge_options *o,
 }
 
 static int conflict_rename_delete(struct merge_options *o,
-				   struct diff_filepair *pair,
-				   const char *rename_branch,
-				   const char *delete_branch)
+				  struct diff_filepair *pair,
+				  const char *rename_branch,
+				  const char *delete_branch)
 {
 	const struct diff_filespec *orig = pair->one;
 	const struct diff_filespec *dest = pair->two;

@@ -1615,7 +1615,7 @@ static int handle_file(struct merge_options *o,
 }
 
 static int conflict_rename_rename_1to2(struct merge_options *o,
-					struct rename_conflict_info *ci)
+				       struct rename_conflict_info *ci)
 {
 	/* One file was renamed in both branches, but to different names. */
 	struct diff_filespec *one = ci->pair1->one;

@@ -1677,7 +1677,7 @@ static int conflict_rename_rename_1to2(struct merge_options *o,
 }
 
 static int conflict_rename_rename_2to1(struct merge_options *o,
-					struct rename_conflict_info *ci)
+				       struct rename_conflict_info *ci)
 {
 	/* Two files, a & b, were renamed to the same thing, c. */
 	struct diff_filespec *a = ci->pair1->one;

@@ -2904,7 +2904,8 @@ static struct object_id *stage_oid(const struct object_id *oid, unsigned mode)
 }
 
 static int read_oid_strbuf(struct merge_options *o,
-	const struct object_id *oid, struct strbuf *dst)
+			   const struct object_id *oid,
+			   struct strbuf *dst)
 {
 	void *buf;
 	enum object_type type;

@@ -2957,10 +2958,10 @@ static int blob_unchanged(struct merge_options *opt,
 }
 
 static int handle_modify_delete(struct merge_options *o,
-				 const char *path,
-				 struct object_id *o_oid, int o_mode,
-				 struct object_id *a_oid, int a_mode,
-				 struct object_id *b_oid, int b_mode)
+				const char *path,
+				struct object_id *o_oid, int o_mode,
+				struct object_id *a_oid, int a_mode,
+				struct object_id *b_oid, int b_mode)
 {
 	const char *modify_branch, *delete_branch;
 	struct object_id *changed_oid;

@@ -3493,14 +3494,14 @@ int merge_recursive_generic(struct merge_options *o,
 			struct commit *base;
 			if (!(base = get_ref(base_list[i], oid_to_hex(base_list[i]))))
 				return err(o, _("Could not parse object '%s'"),
-					oid_to_hex(base_list[i]));
+					   oid_to_hex(base_list[i]));
 			commit_list_insert(base, &ca);
 		}
 	}
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	clean = merge_recursive(o, head_commit, next_commit, ca,
-			result);
+				result);
 	if (clean < 0) {
 		rollback_lock_file(&lock);
 		return clean;

Signature · 1 line

2.18.0.rc1.6.gffeb3ef585

Elijah Newren

author

2018-06-10 04:16 UTC #23 lore raw

PATCH v3 3/6 merge-recursive: align labels with their respective code blocks

In reply to Elijah Newren

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

merge-recursive.c +3 -3

index 09c249b93e..5538038178 100644
@@ -998,10 +998,10 @@ static int update_file_flags(struct merge_options *o,
 			ret = err(o,
 				  _("do not know what to do with %06o %s '%s'"),
 				  mode, oid_to_hex(oid), path);
- free_buf:
+	free_buf:
 		free(buf);
 	}
- update_index:
+update_index:
 	if (!ret && update_cache)
 		if (add_cacheinfo(o, mode, oid, path, 0, update_wd,
 				  ADD_CACHE_OK_TO_ADD))

@@ -3326,7 +3326,7 @@ int merge_trees(struct merge_options *o,
 				    entries->items[i].string);
 		}
 
-cleanup:
+	cleanup:
 		final_cleanup_renames(&re_info);
 
 		string_list_clear(entries, 1);

Signature · 1 line

2.18.0.rc1.6.gffeb3ef585

Elijah Newren

author

2018-06-10 04:16 UTC #24 lore raw

PATCH v3 4/6 merge-recursive: clarify the rename_dir/RENAME_DIR meaning

In reply to Elijah Newren

Since this type is for signifying that the files in question were being renamed due to directory rename detection, rename this enum value to RENAME_VIA_DIR.

Acked-by Johannes Schindelin <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

merge-recursive.c +17 ~~-11~~

index 5538038178..910f0b70f0 100644
@@ -181,7 +181,7 @@ static int oid_eq(const struct object_id *a, const struct object_id *b)
 
 enum rename_type {
 	RENAME_NORMAL = 0,
-	RENAME_DIR,
+	RENAME_VIA_DIR,
 	RENAME_DELETE,
 	RENAME_ONE_FILE_TO_ONE,
 	RENAME_ONE_FILE_TO_TWO,

@@ -1410,11 +1410,17 @@ static int merge_file_one(struct merge_options *o,
 	return merge_file_1(o, &one, &a, &b, path, branch1, branch2, mfi);
 }
 
-static int conflict_rename_dir(struct merge_options *o,
-			       struct diff_filepair *pair,
-			       const char *rename_branch,
-			       const char *other_branch)
+static int conflict_rename_via_dir(struct merge_options *o,
+				   struct diff_filepair *pair,
+				   const char *rename_branch,
+				   const char *other_branch)
 {
+	/*
+	 * Handle file adds that need to be renamed due to directory rename
+	 * detection.  This differs from handle_rename_normal, because
+	 * there is no content merge to do; just move the file into the
+	 * desired final location.
+	 */
 	const struct diff_filespec *dest = pair->two;
 
 	if (!o->call_depth && would_lose_untracked(dest->path)) {

@@ -2692,7 +2698,7 @@ static int process_renames(struct merge_options *o,
 
 			if (oid_eq(&src_other.oid, &null_oid) &&
 			    ren1->add_turned_into_rename) {
-				setup_rename_conflict_info(RENAME_DIR,
+				setup_rename_conflict_info(RENAME_VIA_DIR,
 							   ren1->pair,
 							   NULL,
 							   branch1,

@@ -3138,12 +3144,12 @@ static int process_entry(struct merge_options *o,
 							     b_oid, b_mode,
 							     conflict_info);
 			break;
-		case RENAME_DIR:
+		case RENAME_VIA_DIR:
 			clean_merge = 1;
-			if (conflict_rename_dir(o,
-						conflict_info->pair1,
-						conflict_info->branch1,
-						conflict_info->branch2))
+			if (conflict_rename_via_dir(o,
+						    conflict_info->pair1,
+						    conflict_info->branch1,
+						    conflict_info->branch2))
 				clean_merge = -1;
 			break;
 		case RENAME_DELETE:

Signature · 1 line

2.18.0.rc1.6.gffeb3ef585

Elijah Newren

author

2018-06-10 04:16 UTC #25 lore raw

PATCH v3 5/6 merge-recursive: rename conflict_rename_*() family of functions

In reply to Elijah Newren

Acked-by Johannes Schindelin <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 86 +++++++++++++++++++++++------------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

merge-recursive.c +43 ~~-43~~

index 910f0b70f0..7cf11dc04c 100644
@@ -1410,10 +1410,10 @@ static int merge_file_one(struct merge_options *o,
 	return merge_file_1(o, &one, &a, &b, path, branch1, branch2, mfi);
 }
 
-static int conflict_rename_via_dir(struct merge_options *o,
-				   struct diff_filepair *pair,
-				   const char *rename_branch,
-				   const char *other_branch)
+static int handle_rename_via_dir(struct merge_options *o,
+				 struct diff_filepair *pair,
+				 const char *rename_branch,
+				 const char *other_branch)
 {
 	/*
 	 * Handle file adds that need to be renamed due to directory rename

@@ -1515,10 +1515,10 @@ static int handle_change_delete(struct merge_options *o,
 	return ret;
 }
 
-static int conflict_rename_delete(struct merge_options *o,
-				  struct diff_filepair *pair,
-				  const char *rename_branch,
-				  const char *delete_branch)
+static int handle_rename_delete(struct merge_options *o,
+				struct diff_filepair *pair,
+				const char *rename_branch,
+				const char *delete_branch)
 {
 	const struct diff_filespec *orig = pair->one;
 	const struct diff_filespec *dest = pair->two;

@@ -1620,8 +1620,8 @@ static int handle_file(struct merge_options *o,
 	return ret;
 }
 
-static int conflict_rename_rename_1to2(struct merge_options *o,
-				       struct rename_conflict_info *ci)
+static int handle_rename_rename_1to2(struct merge_options *o,
+				     struct rename_conflict_info *ci)
 {
 	/* One file was renamed in both branches, but to different names. */
 	struct diff_filespec *one = ci->pair1->one;

@@ -1682,8 +1682,8 @@ static int conflict_rename_rename_1to2(struct merge_options *o,
 	return 0;
 }
 
-static int conflict_rename_rename_2to1(struct merge_options *o,
-				       struct rename_conflict_info *ci)
+static int handle_rename_rename_2to1(struct merge_options *o,
+				     struct rename_conflict_info *ci)
 {
 	/* Two files, a & b, were renamed to the same thing, c. */
 	struct diff_filespec *a = ci->pair1->one;

@@ -2425,7 +2425,7 @@ static void apply_directory_rename_modifications(struct merge_options *o,
 	 * "NOTE" in update_stages(), doing so will modify the current
 	 * in-memory index which will break calls to would_lose_untracked()
 	 * that we need to make.  Instead, we need to just make sure that
-	 * the various conflict_rename_*() functions update the index
+	 * the various handle_rename_*() functions update the index
 	 * explicitly rather than relying on unpack_trees() to have done it.
 	 */
 	get_tree_entry(&tree->object.oid,

@@ -2829,12 +2829,12 @@ static void initial_cleanup_rename(struct diff_queue_struct *pairs,
 	free(pairs);
 }
 
-static int handle_renames(struct merge_options *o,
-			  struct tree *common,
-			  struct tree *head,
-			  struct tree *merge,
-			  struct string_list *entries,
-			  struct rename_info *ri)
+static int detect_and_process_renames(struct merge_options *o,
+				      struct tree *common,
+				      struct tree *head,
+				      struct tree *merge,
+				      struct string_list *entries,
+				      struct rename_info *ri)
 {
 	struct diff_queue_struct *head_pairs, *merge_pairs;
 	struct hashmap *dir_re_head, *dir_re_merge;

@@ -3105,12 +3105,12 @@ static int merge_content(struct merge_options *o,
 	return !is_dirty && mfi.clean;
 }
 
-static int conflict_rename_normal(struct merge_options *o,
-				  const char *path,
-				  struct object_id *o_oid, unsigned int o_mode,
-				  struct object_id *a_oid, unsigned int a_mode,
-				  struct object_id *b_oid, unsigned int b_mode,
-				  struct rename_conflict_info *ci)
+static int handle_rename_normal(struct merge_options *o,
+				const char *path,
+				struct object_id *o_oid, unsigned int o_mode,
+				struct object_id *a_oid, unsigned int a_mode,
+				struct object_id *b_oid, unsigned int b_mode,
+				struct rename_conflict_info *ci)
 {
 	/* Merge the content and write it out */
 	return merge_content(o, path, was_dirty(o, path),

@@ -3137,37 +3137,37 @@ static int process_entry(struct merge_options *o,
 		switch (conflict_info->rename_type) {
 		case RENAME_NORMAL:
 		case RENAME_ONE_FILE_TO_ONE:
-			clean_merge = conflict_rename_normal(o,
-							     path,
-							     o_oid, o_mode,
-							     a_oid, a_mode,
-							     b_oid, b_mode,
-							     conflict_info);
+			clean_merge = handle_rename_normal(o,
+							   path,
+							   o_oid, o_mode,
+							   a_oid, a_mode,
+							   b_oid, b_mode,
+							   conflict_info);
 			break;
 		case RENAME_VIA_DIR:
 			clean_merge = 1;
-			if (conflict_rename_via_dir(o,
-						    conflict_info->pair1,
-						    conflict_info->branch1,
-						    conflict_info->branch2))
+			if (handle_rename_via_dir(o,
+						  conflict_info->pair1,
+						  conflict_info->branch1,
+						  conflict_info->branch2))
 				clean_merge = -1;
 			break;
 		case RENAME_DELETE:
 			clean_merge = 0;
-			if (conflict_rename_delete(o,
-						   conflict_info->pair1,
-						   conflict_info->branch1,
-						   conflict_info->branch2))
+			if (handle_rename_delete(o,
+						 conflict_info->pair1,
+						 conflict_info->branch1,
+						 conflict_info->branch2))
 				clean_merge = -1;
 			break;
 		case RENAME_ONE_FILE_TO_TWO:
 			clean_merge = 0;
-			if (conflict_rename_rename_1to2(o, conflict_info))
+			if (handle_rename_rename_1to2(o, conflict_info))
 				clean_merge = -1;
 			break;
 		case RENAME_TWO_FILES_TO_ONE:
 			clean_merge = 0;
-			if (conflict_rename_rename_2to1(o, conflict_info))
+			if (handle_rename_rename_2to1(o, conflict_info))
 				clean_merge = -1;
 			break;
 		default:

@@ -3307,8 +3307,8 @@ int merge_trees(struct merge_options *o,
 		get_files_dirs(o, merge);
 
 		entries = get_unmerged();
-		clean = handle_renames(o, common, head, merge, entries,
-				       &re_info);
+		clean = detect_and_process_renames(o, common, head, merge,
+						   entries, &re_info);
 		record_df_conflict_files(o, entries);
 		if (clean < 0)
 			goto cleanup;

Signature · 1 line

2.18.0.rc1.6.gffeb3ef585

Elijah Newren

author

2018-06-10 04:16 UTC #26 lore raw

PATCH v3 6/6 merge-recursive: add pointer about unduly complex looking code

In reply to Elijah Newren

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

merge-recursive.c +15 -0

index 7cf11dc04c..284b27d895 100644
@@ -1476,6 +1476,21 @@ static int handle_change_delete(struct merge_options *o,
 		if (!ret)
 			ret = update_file(o, 0, o_oid, o_mode, update_path);
 	} else {
+		/*
+		 * Despite the four nearly duplicate messages and argument
+		 * lists below and the ugliness of the nested if-statements,
+		 * having complete messages makes the job easier for
+		 * translators.
+		 *
+		 * The slight variance among the cases is due to the fact
+		 * that:
+		 *   1) directory/file conflicts (in effect if
+		 *      !alt_path) could cause us to need to write the
+		 *      file to a different path.
+		 *   2) renames (in effect if !old_path) could mean that
+		 *      there are two names for the path that the user
+		 *      may know the file by.
+		 */
 		if (!alt_path) {
 			if (!old_path) {
 				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "

Signature · 1 line

2.18.0.rc1.6.gffeb3ef585