threads / patch / 48520

patch, 5 partsmerge-recursive: fix miscellaneous grammar error in comment

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

## tl;dr

27 messages between May 19, 2018 and Jun 10, 2018. Diffs are folded; open one to read it.

replies: 26people: 3as markdown or json

Elijah Newren· May 19, 2018, 02:06 UTC · lore

[PATCH 0/5] 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(-)
-- 
2.17.0.847.g20b8963732
Elijah Newren· May 19, 2018, 02:06 UTC · re: Elijah Newren · lore
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to merge-recursive.c +1 −1
diff --git a/merge-recursive.c b/merge-recursive.c
index 35df695fa4..c430fd72bc 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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
-- 
2.17.0.847.g20b8963732
Elijah Newren· May 19, 2018, 02:07 UTC · re: Elijah Newren · lore

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

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 <newren@gmail.com>
---
 merge-recursive.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)
Show changes to merge-recursive.c +11 −25
diff --git a/merge-recursive.c b/merge-recursive.c
index 273ee79afa..3bd727995b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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
-- 
2.17.0.847.g20b8963732
Johannes Sixt· May 19, 2018, 07:32 UTC · re: Elijah Newren · lore

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

Am 19.05.2018 um 04:07 schrieb 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.

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.

Show 56 quoted 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 <newren@gmail.com>
> ---
>   merge-recursive.c | 36 +++++++++++-------------------------
>   1 file changed, 11 insertions(+), 25 deletions(-)
> 
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 273ee79afa..3bd727995b 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -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· May 19, 2018, 15:39 UTC · re: Johannes Sixt · lore

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

On Sat, May 19, 2018 at 12:32 AM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 8 quoted lines
> Am 19.05.2018 um 04:07 schrieb 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.
>
> 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· May 21, 2018, 13:41 UTC · re: Elijah Newren · lore

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

Hi Elijah,
On Sat, 19 May 2018, Elijah Newren wrote:
Show 12 quoted lines
> On Sat, May 19, 2018 at 12:32 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> > Am 19.05.2018 um 04:07 schrieb 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.
> >
> > 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· May 21, 2018, 17:22 UTC · re: Johannes Schindelin · lore

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

Hi Dscho,

On Mon, May 21, 2018 at 6:41 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> On Sat, 19 May 2018, Elijah Newren wrote:
>> On Sat, May 19, 2018 at 12:32 AM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 12 quoted 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?

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· May 19, 2018, 02:06 UTC · re: Elijah Newren · lore

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

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)
Show changes to merge-recursive.c +24 −23
diff --git a/merge-recursive.c b/merge-recursive.c
index c430fd72bc..01306c87eb 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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;
-- 
2.17.0.847.g20b8963732
Johannes Schindelin· May 21, 2018, 13:42 UTC · re: Elijah Newren · lore

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

Hi Elijah,
On Fri, 18 May 2018, Elijah Newren wrote:
> 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· May 21, 2018, 16:48 UTC · re: Johannes Schindelin · lore

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

Hi Dscho,

On Mon, May 21, 2018 at 6:42 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 9 quoted lines
> Hi Elijah,
>
> On Fri, 18 May 2018, Elijah Newren wrote:
>
>> 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.

> The patch looks obviously good to me.
Thanks for taking a look!
Elijah Newren· May 19, 2018, 02:06 UTC · re: Elijah Newren · lore

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

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 <newren@gmail.com>
---
 merge-recursive.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)
Show changes to merge-recursive.c +17 −11
diff --git a/merge-recursive.c b/merge-recursive.c
index 01306c87eb..d30085d9c7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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:
-- 
2.17.0.847.g20b8963732
Johannes Schindelin· May 21, 2018, 14:28 UTC · re: Elijah Newren · lore

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

Hi Elijah,
On Fri, 18 May 2018, Elijah Newren wrote:
Show 12 quoted lines
> 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 <newren@gmail.com>
> ---

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

Show 17 quoted lines
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 01306c87eb..d30085d9c7 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> -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
Technically, we do not move the path, but the file.
The rest of the diff looks obviously correct to me.

Thanks, Dscho

Elijah Newren· May 19, 2018, 02:06 UTC · re: Elijah Newren · lore

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

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 <newren@gmail.com>
---
 merge-recursive.c | 86 +++++++++++++++++++++++------------------------
 1 file changed, 43 insertions(+), 43 deletions(-)
Show changes to merge-recursive.c +43 −43
diff --git a/merge-recursive.c b/merge-recursive.c
index d30085d9c7..273ee79afa 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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;
-- 
2.17.0.847.g20b8963732
Johannes Schindelin· May 21, 2018, 14:30 UTC · re: Elijah Newren · lore

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

Hi Elijah,
On Fri, 18 May 2018, Elijah Newren wrote:
Show 23 quoted lines
> 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 <newren@gmail.com>
Makes sense, and the patch looks obviously correct to me.

Thanks, Dscho

Elijah Newren· May 22, 2018, 00:43 UTC · re: Elijah Newren · lore

[PATCH v2 0/5] 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 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)
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: add pointer about unduly complex looking code
 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 <Johannes.Schindelin@gmx.de>
         Signed-off-by: Elijah Newren <newren@gmail.com>
     
     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 <Johannes.Schindelin@gmx.de>
         Signed-off-by: Elijah Newren <newren@gmail.com>
     
     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 <Johannes.Schindelin@gmx.de>
         Signed-off-by: Elijah Newren <newren@gmail.com>
     
     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
-- 
2.17.0.847.g20b8963732
Elijah Newren· May 22, 2018, 00:43 UTC · re: Elijah Newren · lore

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

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

Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 86 +++++++++++++++++++++++------------------------
 1 file changed, 43 insertions(+), 43 deletions(-)
Show changes to merge-recursive.c +43 −43
diff --git a/merge-recursive.c b/merge-recursive.c
index e2b0db0645..270a4d2aad 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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;
-- 
2.17.0.847.g20b8963732
Elijah Newren· May 22, 2018, 00:43 UTC · re: Elijah Newren · lore

[PATCH v2 5/5] 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.

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 <newren@gmail.com>
---
 merge-recursive.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
Show changes to merge-recursive.c +15 −0
diff --git a/merge-recursive.c b/merge-recursive.c
index 270a4d2aad..28f39b3e9f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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 "
-- 
2.17.0.847.g20b8963732
Elijah Newren· May 22, 2018, 00:43 UTC · re: Elijah Newren · lore

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to merge-recursive.c +1 −1
diff --git a/merge-recursive.c b/merge-recursive.c
index 35df695fa4..c430fd72bc 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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
-- 
2.17.0.847.g20b8963732
Elijah Newren· May 22, 2018, 00:43 UTC · re: Elijah Newren · lore

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

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

Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)
Show changes to merge-recursive.c +24 −23
diff --git a/merge-recursive.c b/merge-recursive.c
index c430fd72bc..01306c87eb 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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;
-- 
2.17.0.847.g20b8963732
Elijah Newren· May 22, 2018, 00:43 UTC · re: Elijah Newren · lore

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

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

Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)
Show changes to merge-recursive.c +17 −11
diff --git a/merge-recursive.c b/merge-recursive.c
index 01306c87eb..e2b0db0645 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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:
-- 
2.17.0.847.g20b8963732
Elijah Newren· Jun 10, 2018, 04:16 UTC · re: Elijah Newren · lore

[PATCH v3 0/6] merge-recursive code cleanups

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
-- 
2.18.0.rc1.6.gffeb3ef585
Elijah Newren· Jun 10, 2018, 04:16 UTC · re: Elijah Newren · lore

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to merge-recursive.c +1 −1
diff --git a/merge-recursive.c b/merge-recursive.c
index ac27abbd4c..921f8e2d2d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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
-- 
2.18.0.rc1.6.gffeb3ef585
Elijah Newren· Jun 10, 2018, 04:16 UTC · re: Elijah Newren · lore

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

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

Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 75 ++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 37 deletions(-)
Show changes to merge-recursive.c +38 −37
diff --git a/merge-recursive.c b/merge-recursive.c
index 921f8e2d2d..09c249b93e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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;
-- 
2.18.0.rc1.6.gffeb3ef585
Elijah Newren· Jun 10, 2018, 04:16 UTC · re: Elijah Newren · lore

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

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

Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)
Show changes to merge-recursive.c +17 −11
diff --git a/merge-recursive.c b/merge-recursive.c
index 5538038178..910f0b70f0 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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:
-- 
2.18.0.rc1.6.gffeb3ef585
Elijah Newren· Jun 10, 2018, 04:16 UTC · re: Elijah Newren · lore

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to merge-recursive.c +3 −3
diff --git a/merge-recursive.c b/merge-recursive.c
index 09c249b93e..5538038178 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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);
-- 
2.18.0.rc1.6.gffeb3ef585
Elijah Newren· Jun 10, 2018, 04:16 UTC · re: Elijah Newren · lore

[PATCH v3 6/6] 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.

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 <newren@gmail.com>
---
 merge-recursive.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
Show changes to merge-recursive.c +15 −0
diff --git a/merge-recursive.c b/merge-recursive.c
index 7cf11dc04c..284b27d895 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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 "
-- 
2.18.0.rc1.6.gffeb3ef585
Elijah Newren· Jun 10, 2018, 04:16 UTC · re: Elijah Newren · lore

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

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

Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c | 86 +++++++++++++++++++++++------------------------
 1 file changed, 43 insertions(+), 43 deletions(-)
Show changes to merge-recursive.c +43 −43
diff --git a/merge-recursive.c b/merge-recursive.c
index 910f0b70f0..7cf11dc04c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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;
-- 
2.18.0.rc1.6.gffeb3ef585

← back to recent threads